diff mbox

[3/8] hw/display/pxa2xx_lcd: Fix 16bpp+alpha and 18bpp+alpha palette formats

Message ID 1399574818-19349-4-git-send-email-peter.maydell@linaro.org
State Superseded
Headers show

Commit Message

Peter Maydell May 8, 2014, 6:46 p.m. UTC
The pxa2xx palette entry "16bpp plus transparency" format is
xxxxxxxTRRRRR000GGGGGG00BBBBB000, and "18bpp plus transparency" is
xxxxxxxTRRRRRR00GGGGGG00BBBBBB00.

Correct errors in the code for reading these and converting
them to the internal format. In particular, the buggy code
was attempting to mask out bit 24 of a uint16_t, which
Coverity spotted as an error.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/display/pxa2xx_lcd.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Peter Crosthwaite May 9, 2014, 11:35 p.m. UTC | #1
On Fri, May 9, 2014 at 4:46 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> The pxa2xx palette entry "16bpp plus transparency" format is
> xxxxxxxTRRRRR000GGGGGG00BBBBB000, and "18bpp plus transparency" is
> xxxxxxxTRRRRRR00GGGGGG00BBBBBB00.
>
> Correct errors in the code for reading these and converting
> them to the internal format. In particular, the buggy code
> was attempting to mask out bit 24 of a uint16_t, which
> Coverity spotted as an error.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/display/pxa2xx_lcd.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/hw/display/pxa2xx_lcd.c b/hw/display/pxa2xx_lcd.c
> index 09cdf17..fce013d 100644
> --- a/hw/display/pxa2xx_lcd.c
> +++ b/hw/display/pxa2xx_lcd.c
> @@ -620,13 +620,13 @@ static void pxa2xx_palette_parse(PXA2xxLCDState *s, int ch, int bpp)
>              src += 2;
>              break;
>          case 1: /* 16 bpp plus transparency */
> -            alpha = *(uint16_t *) src & (1 << 24);
> +            alpha = *(uint32_t *) src & (1 << 24);
>              if (s->control[0] & LCCR0_CMS)
> -                r = g = b = *(uint16_t *) src & 0xff;
> +                r = g = b = *(uint32_t *) src & 0xff;
>              else {
> -                r = (*(uint16_t *) src & 0xf800) >> 8;
> -                g = (*(uint16_t *) src & 0x07e0) >> 3;
> -                b = (*(uint16_t *) src & 0x001f) << 3;
> +                r = (*(uint32_t *) src & 0x7c0000) >> 15;

16BPP format (pasted from above with byte spacing)
xxxxxxxT RRRRR000 GGGGGG00 BBBBB000

So shouldn't r be 0xf80000 >> 16?

Regards,
Peter

> +                g = (*(uint32_t *) src & 0x00fc00) >> 8;
> +                b = (*(uint32_t *) src & 0x0000f8);
>              }
>              src += 2;
>              break;
> @@ -635,9 +635,9 @@ static void pxa2xx_palette_parse(PXA2xxLCDState *s, int ch, int bpp)
>              if (s->control[0] & LCCR0_CMS)
>                  r = g = b = *(uint32_t *) src & 0xff;
>              else {
> -                r = (*(uint32_t *) src & 0xf80000) >> 16;
> +                r = (*(uint32_t *) src & 0xfc0000) >> 16;
>                  g = (*(uint32_t *) src & 0x00fc00) >> 8;
> -                b = (*(uint32_t *) src & 0x0000f8);
> +                b = (*(uint32_t *) src & 0x0000fc);
>              }
>              src += 4;
>              break;
> --
> 1.9.2
>
>
Peter Maydell May 13, 2014, 3:23 p.m. UTC | #2
On 10 May 2014 00:35, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Fri, May 9, 2014 at 4:46 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> The pxa2xx palette entry "16bpp plus transparency" format is
>> xxxxxxxTRRRRR000GGGGGG00BBBBB000, and "18bpp plus transparency" is
>> xxxxxxxTRRRRRR00GGGGGG00BBBBBB00.
>>
>> Correct errors in the code for reading these and converting
>> them to the internal format. In particular, the buggy code
>> was attempting to mask out bit 24 of a uint16_t, which
>> Coverity spotted as an error.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  hw/display/pxa2xx_lcd.c | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/display/pxa2xx_lcd.c b/hw/display/pxa2xx_lcd.c
>> index 09cdf17..fce013d 100644
>> --- a/hw/display/pxa2xx_lcd.c
>> +++ b/hw/display/pxa2xx_lcd.c
>> @@ -620,13 +620,13 @@ static void pxa2xx_palette_parse(PXA2xxLCDState *s, int ch, int bpp)
>>              src += 2;
>>              break;
>>          case 1: /* 16 bpp plus transparency */
>> -            alpha = *(uint16_t *) src & (1 << 24);
>> +            alpha = *(uint32_t *) src & (1 << 24);
>>              if (s->control[0] & LCCR0_CMS)
>> -                r = g = b = *(uint16_t *) src & 0xff;
>> +                r = g = b = *(uint32_t *) src & 0xff;
>>              else {
>> -                r = (*(uint16_t *) src & 0xf800) >> 8;
>> -                g = (*(uint16_t *) src & 0x07e0) >> 3;
>> -                b = (*(uint16_t *) src & 0x001f) << 3;
>> +                r = (*(uint32_t *) src & 0x7c0000) >> 15;
>
> 16BPP format (pasted from above with byte spacing)
> xxxxxxxT RRRRR000 GGGGGG00 BBBBB000
>
> So shouldn't r be 0xf80000 >> 16?

Yep, you're right. My first attempt at this I read the wrong
diagram from the manual (the framebuffer pixel formats are
different from the palette formats) and failed to spot I
hadn't corrected the code completely.

Since this is the only patch in this set that needs a
respin, and they're all independent fixes, I'll put the
other 7 into target-arm.next and just resend this.

thanks
-- PMM
diff mbox

Patch

diff --git a/hw/display/pxa2xx_lcd.c b/hw/display/pxa2xx_lcd.c
index 09cdf17..fce013d 100644
--- a/hw/display/pxa2xx_lcd.c
+++ b/hw/display/pxa2xx_lcd.c
@@ -620,13 +620,13 @@  static void pxa2xx_palette_parse(PXA2xxLCDState *s, int ch, int bpp)
             src += 2;
             break;
         case 1: /* 16 bpp plus transparency */
-            alpha = *(uint16_t *) src & (1 << 24);
+            alpha = *(uint32_t *) src & (1 << 24);
             if (s->control[0] & LCCR0_CMS)
-                r = g = b = *(uint16_t *) src & 0xff;
+                r = g = b = *(uint32_t *) src & 0xff;
             else {
-                r = (*(uint16_t *) src & 0xf800) >> 8;
-                g = (*(uint16_t *) src & 0x07e0) >> 3;
-                b = (*(uint16_t *) src & 0x001f) << 3;
+                r = (*(uint32_t *) src & 0x7c0000) >> 15;
+                g = (*(uint32_t *) src & 0x00fc00) >> 8;
+                b = (*(uint32_t *) src & 0x0000f8);
             }
             src += 2;
             break;
@@ -635,9 +635,9 @@  static void pxa2xx_palette_parse(PXA2xxLCDState *s, int ch, int bpp)
             if (s->control[0] & LCCR0_CMS)
                 r = g = b = *(uint32_t *) src & 0xff;
             else {
-                r = (*(uint32_t *) src & 0xf80000) >> 16;
+                r = (*(uint32_t *) src & 0xfc0000) >> 16;
                 g = (*(uint32_t *) src & 0x00fc00) >> 8;
-                b = (*(uint32_t *) src & 0x0000f8);
+                b = (*(uint32_t *) src & 0x0000fc);
             }
             src += 4;
             break;