Re: [Qemu-trivial] [Qemu-devel] [PATCH v2] console: Correct computation of bytes per pixel from bits per pixel

[Jan Kiszka]

On 2012-08-22 18:44, Jan Kiszka wrote:
> On 2012-08-22 18:29, Stefan Weil wrote:
>> Am 22.08.2012 17:32, schrieb Jan Kiszka:
>>> On 2012-08-22 17:19, BALATON Zoltan wrote:
>>>> Division with round up is the correct way to compute this even if the
>>>> only case where division with round down gives incorrect result is
>>>> probably 15 bpp. This case was explicitely patched up in one of these
>>>> functions but was unhandled in the other. (I'm not sure about setting
>>>> 16 bpp for the 15bpp case either but I left that there for now.)
>>>>
>>>> Signed-off-by: BALATON Zoltan <address@hidden>
>>>> ---
>>>>   console.c |    5 ++---
>>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>>
>>>>   v2: Use DIV_ROUND_UP and extended commit message
>>>>
>>>> diff --git a/console.c b/console.c
>>>> index 4525cc7..9df1701 100644
>>>> --- a/console.c
>>>> +++ b/console.c
>>>> @@ -1612,7 +1612,7 @@ PixelFormat
>>>> qemu_different_endianness_pixelformat(int bpp)
>>>>       memset(&pf, 0x00, sizeof(PixelFormat));
>>>>
>>>>       pf.bits_per_pixel = bpp;
>>>> -    pf.bytes_per_pixel = bpp / 8;
>>>> +    pf.bytes_per_pixel = DIV_ROUND_UP(bpp, 8);
>>>>       pf.depth = bpp == 32 ? 24 : bpp;
>>>>
>>>>       switch (bpp) {
>>>> @@ -1661,13 +1661,12 @@ PixelFormat qemu_default_pixelformat(int bpp)
>>>>       memset(&pf, 0x00, sizeof(PixelFormat));
>>>>
>>>>       pf.bits_per_pixel = bpp;
>>>> -    pf.bytes_per_pixel = bpp / 8;
>>>> +    pf.bytes_per_pixel = DIV_ROUND_UP(bpp, 8);
>>>>       pf.depth = bpp == 32 ? 24 : bpp;
>>>>
>>>>       switch (bpp) {
>>>>           case 15:
>>>>               pf.bits_per_pixel = 16;
>>>> -            pf.bytes_per_pixel = 2;
>>> Removed unintentionally?
>>>
>>> Jan
>>
>> 15 bpp no longer needs special handling because
>> the computation above is fixed by the same patch,
>> so the removal is correct.
> 
> Ok, actually starting to look at this code: This is apparently a code
> cleanup, not a bug fix like the subject suggests. No valid input for
> could have actually triggered the round-up issue. Please declare it as
> such. Bonus for abort() on the invalid default cases.

Well, do not abort on bpp == 0, the curses case.

Also, qemu_different_endianness_pixelformat is currently only called
with 16 or 32 bpp (see vga.c), but only handles 24 and 32. Fishy...

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux