Re: [Qemu-trivial] [Qemu-devel] [PATCH v3] console: Cleanup computation of bytes per pixel and add missing cases

[Stefan Hajnoczi]

On Thu, Aug 23, 2012 at 02:08:36AM +0200, 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. This patch also adds the
> missing cases and aborts for invalid unhandled cases. (I'm not sure
> about setting 16 bpp for the 15 bpp case so I left it there for now.)
> 
> Signed-off-by: BALATON Zoltan <address@hidden>
> ---
>  console.c |  230 
> +++++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 131 insertions(+), 99 deletions(-)
> 
>  v2: Use DIV_ROUND_UP and extended commit message
>  v3: Add missing cases to qemu_different_endianness_pixelformat (I
>      have no way to test if this is correct though),
>      abort() for the default: case
>      also reindented as suggested by scripts/checkpatch.pl
> 
> diff --git a/console.c b/console.c
> index 4525cc7..21843cd 100644
> --- a/console.c
> +++ b/console.c
> @@ -1612,44 +1612,75 @@ 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) {
> -        case 24:
> -            pf.rmask = 0x000000FF;
> -            pf.gmask = 0x0000FF00;
> -            pf.bmask = 0x00FF0000;
> -            pf.rmax = 255;
> -            pf.gmax = 255;
> -            pf.bmax = 255;
> -            pf.rshift = 0;
> -            pf.gshift = 8;
> -            pf.bshift = 16;
> -            pf.rbits = 8;
> -            pf.gbits = 8;
> -            pf.bbits = 8;
> -            break;
> -        case 32:
> -            pf.rmask = 0x0000FF00;
> -            pf.gmask = 0x00FF0000;
> -            pf.bmask = 0xFF000000;
> -            pf.amask = 0x00000000;
> -            pf.amax = 255;
> -            pf.rmax = 255;
> -            pf.gmax = 255;
> -            pf.bmax = 255;
> -            pf.ashift = 0;
> -            pf.rshift = 8;
> -            pf.gshift = 16;
> -            pf.bshift = 24;
> -            pf.rbits = 8;
> -            pf.gbits = 8;
> -            pf.bbits = 8;
> -            pf.abits = 8;
> -            break;
> -        default:
> -            break;
> +    case 0:
> +        break;
> +    case 15:
> +        pf.bits_per_pixel = 16; /* Is this correct? */

A trivial patch can't have "Is this correct?" parts :).

I already applied the simple and correct v2 patch.  Feel free to follow
up with additional cleanups but with resolved "Is this correct?" parts.

Stefan