* [Qemu-devel] [PATCH v3] console: Cleanup computation of bytes per pixel and add missing cases
@ 2012-08-23 0:08 BALATON Zoltan
2012-08-24 11:26 ` Stefan Hajnoczi
2012-08-24 11:36 ` Peter Maydell
0 siblings, 2 replies; 6+ messages in thread
From: BALATON Zoltan @ 2012-08-23 0:08 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-trivial
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? */
+ 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;
}
--
1.7.10
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v3] console: Cleanup computation of bytes per pixel and add missing cases
2012-08-23 0:08 [Qemu-devel] [PATCH v3] console: Cleanup computation of bytes per pixel and add missing cases BALATON Zoltan
@ 2012-08-24 11:26 ` Stefan Hajnoczi
2012-08-24 15:24 ` BALATON Zoltan
2012-08-24 11:36 ` Peter Maydell
1 sibling, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2012-08-24 11:26 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-trivial, qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v3] console: Cleanup computation of bytes per pixel and add missing cases
2012-08-24 11:26 ` Stefan Hajnoczi
@ 2012-08-24 15:24 ` BALATON Zoltan
2012-08-24 15:45 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: BALATON Zoltan @ 2012-08-24 15:24 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-trivial, qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] console: Cleanup computation of bytes per pixel and add missing cases
2012-08-24 15:24 ` BALATON Zoltan
@ 2012-08-24 15:45 ` Peter Maydell
2012-08-24 18:48 ` BALATON Zoltan
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2012-08-24 15:45 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-trivial, Stefan Hajnoczi, qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] console: Cleanup computation of bytes per pixel and add missing cases
2012-08-24 15:45 ` Peter Maydell
@ 2012-08-24 18:48 ` BALATON Zoltan
0 siblings, 0 replies; 6+ messages in thread
From: BALATON Zoltan @ 2012-08-24 18:48 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-trivial, Stefan Hajnoczi, qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] console: Cleanup computation of bytes per pixel and add missing cases
2012-08-23 0:08 [Qemu-devel] [PATCH v3] console: Cleanup computation of bytes per pixel and add missing cases BALATON Zoltan
2012-08-24 11:26 ` Stefan Hajnoczi
@ 2012-08-24 11:36 ` Peter Maydell
1 sibling, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2012-08-24 11:36 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-trivial, qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-08-24 18:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-08-23 0:08 [Qemu-devel] [PATCH v3] console: Cleanup computation of bytes per pixel and add missing cases BALATON Zoltan
2012-08-24 11:26 ` Stefan Hajnoczi
2012-08-24 15:24 ` BALATON Zoltan
2012-08-24 15:45 ` Peter Maydell
2012-08-24 18:48 ` BALATON Zoltan
2012-08-24 11:36 ` Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).