[v3] console: Cleanup computation of bytes per pixel and add missing cases

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 <balaton@eik.bme.hu>
---
  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

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 <balaton@eik.bme.hu>
> ---
>  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

On 23 August 2012 01:08, BALATON Zoltan <balaton@eik.bme.hu> 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 <balaton@eik.bme.hu>
> ---
>  console.c |  230
> +++++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 131 insertions(+), 99 deletions(-)

It's a bit hard to see what's actually been changed in this
patch -- can you split it up into a patch that fixes the
switch indent first (and which has only whitespace changes,
so 'git diff -w' produces no output for it), and then a patch
making the actual changes, please?

(A useful rule of thumb, incidentally, is that if you find yourself
writing "also" in a commit message you should consider splitting
it into two separate commits instead :-))

thanks
-- PMM

On Fri, 24 Aug 2012, Stefan Hajnoczi wrote:
>> +    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.

The v2 contained all my originally intended changes so it's fine with me. 
During review it was discovered that this part of code probably has more 
problems so I was trying to provide a fix for those in v3, and then 
checkpatch.pl complained about formatting too so I included a fix for that 
too. I can resend these broken up into too patches but as I wrote in the 
comment I have no way to test if these are correct. The above 
bits_per_pixel=16 for 15 bpp case was there already I just added a comment 
marking it as suspicious but I'm not sure how to resolve that.

Thanks,
BALATON Zoltan

On 24 August 2012 16:24, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> The above
> bits_per_pixel=16 for 15 bpp case was there already I just added a comment
> marking it as suspicious but I'm not sure how to resolve that.

It is certainly a bug somewhere, since ds_get_bits_per_pixel() just
returns the pixelformat bits_per_pixel field, and code like hw/pl110.c
assumes that that can be 15 and that that implies that the pixel
format is as per rgb_to_pixel15() in pixel_ops.h. So one end of
that or the other is wrong.

I think all this code needs careful auditing to (a) clearly define
what "bpp" means and what "depth" means (assuming we need to distinguish;
IME they are usually supposed to be synonyms!) and (b) make sure that
everything that accesses one or the other is actually using the
right one...

-- PMM

On Fri, 24 Aug 2012, Peter Maydell wrote:
> I think all this code needs careful auditing to (a) clearly define
> what "bpp" means and what "depth" means (assuming we need to distinguish;
> IME they are usually supposed to be synonyms!) and (b) make sure that
> everything that accesses one or the other is actually using the
> right one...

Right, and I don't feel like I can do that for parts I don't know at all 
so that's what I meant by not being sure about it and chose to add the 
comment as a hint for others who know better where these are used. I think 
I've mostly fixed it in vmware_vga which was the part I've looked at.

About depth vs. bpp, they are usually synonyms except for the 32 bits case 
where colour depth is actually 24 bits so distinguishing the two is 
proabably OK. (This was one of the things that the vmware_vga reported 
wrongly and the driver was patching it up with issuing a warning about 
it.)

Regards,
BALATON Zoltan