Submitted by BALATON Zoltan on Aug. 23, 2012, 12:08 a.m.

Message ID | Pine.GSO.4.64.1208230207120.26411@mono |
---|---|

State | New |

Headers | show |

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

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? */ + pf.rmask = 0x0000001F; + pf.gmask = 0x000003E0; + pf.bmask = 0x00007C00; + pf.rmax = 31; + pf.gmax = 31; + pf.bmax = 31; + pf.rshift = 0; + pf.gshift = 5; + pf.bshift = 10; + pf.rbits = 5; + pf.gbits = 5; + pf.bbits = 5; + break; + case 16: + pf.rmask = 0x0000001F; + pf.gmask = 0x000007E0; + pf.bmask = 0x0000F800; + pf.rmax = 31; + pf.gmax = 63; + pf.bmax = 31; + pf.rshift = 0; + pf.gshift = 5; + pf.bshift = 11; + pf.rbits = 5; + pf.gbits = 6; + pf.bbits = 5; + break; + 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: + abort(); } return pf; } @@ -1661,73 +1692,74 @@ 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; - pf.rmask = 0x00007c00; - pf.gmask = 0x000003E0; - pf.bmask = 0x0000001F; - pf.rmax = 31; - pf.gmax = 31; - pf.bmax = 31; - pf.rshift = 10; - pf.gshift = 5; - pf.bshift = 0; - pf.rbits = 5; - pf.gbits = 5; - pf.bbits = 5; - break; - case 16: - pf.rmask = 0x0000F800; - pf.gmask = 0x000007E0; - pf.bmask = 0x0000001F; - pf.rmax = 31; - pf.gmax = 63; - pf.bmax = 31; - pf.rshift = 11; - pf.gshift = 5; - pf.bshift = 0; - pf.rbits = 5; - pf.gbits = 6; - pf.bbits = 5; - break; - case 24: - pf.rmask = 0x00FF0000; - pf.gmask = 0x0000FF00; - pf.bmask = 0x000000FF; - pf.rmax = 255; - pf.gmax = 255; - pf.bmax = 255; - pf.rshift = 16; - pf.gshift = 8; - pf.bshift = 0; - pf.rbits = 8; - pf.gbits = 8; - pf.bbits = 8; - break; - case 32: - pf.rmask = 0x00FF0000; - pf.gmask = 0x0000FF00; - pf.bmask = 0x000000FF; - pf.amax = 255; - pf.rmax = 255; - pf.gmax = 255; - pf.bmax = 255; - pf.ashift = 24; - pf.rshift = 16; - pf.gshift = 8; - pf.bshift = 0; - pf.rbits = 8; - pf.gbits = 8; - pf.bbits = 8; - pf.abits = 8; - break; - default: - break; + case 0: /* Used by ui/curses */ + break; + case 15: + pf.bits_per_pixel = 16; /* Is this correct? */ + pf.rmask = 0x00007C00; + pf.gmask = 0x000003E0; + pf.bmask = 0x0000001F; + pf.rmax = 31; + pf.gmax = 31; + pf.bmax = 31; + pf.rshift = 10; + pf.gshift = 5; + pf.bshift = 0; + pf.rbits = 5; + pf.gbits = 5; + pf.bbits = 5; + break; + case 16: + pf.rmask = 0x0000F800; + pf.gmask = 0x000007E0; + pf.bmask = 0x0000001F; + pf.rmax = 31; + pf.gmax = 63; + pf.bmax = 31; + pf.rshift = 11; + pf.gshift = 5; + pf.bshift = 0; + pf.rbits = 5; + pf.gbits = 6; + pf.bbits = 5; + break; + case 24: + pf.rmask = 0x00FF0000; + pf.gmask = 0x0000FF00; + pf.bmask = 0x000000FF; + pf.rmax = 255; + pf.gmax = 255; + pf.bmax = 255; + pf.rshift = 16; + pf.gshift = 8; + pf.bshift = 0; + pf.rbits = 8; + pf.gbits = 8; + pf.bbits = 8; + break; + case 32: + pf.rmask = 0x00FF0000; + pf.gmask = 0x0000FF00; + pf.bmask = 0x000000FF; + pf.amax = 255; + pf.rmax = 255; + pf.gmax = 255; + pf.bmax = 255; + pf.ashift = 24; + pf.rshift = 16; + pf.gshift = 8; + pf.bshift = 0; + pf.rbits = 8; + pf.gbits = 8; + pf.bbits = 8; + pf.abits = 8; + break; + default: + abort(); } return pf; }

`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`