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

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(-)

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

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