Re: [Qemu-devel] [PATCH v2] hw/display/pxa2xx_lcd: Fix 16bpp+alpha and 18bpp+alpha palette formats

[Peter Crosthwaite]

On Fri, May 16, 2014 at 7:51 PM, Peter Maydell <address@hidden> 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 <address@hidden>

Reviewed-by: Peter Crosthwaite <address@hidden>

> ---
> This is the remaining uncommitted patch from the set of 8 random
> coverity fixes I sent a little while back.
> Changes v1->v2:
>  * use correct mask/shift for R component in 16bpp+transparency
>  * we need to add 4 to the src ptr, not 2, for the 16bpp+transparency
>    format, since each palette entry is 32 bits
>
>  hw/display/pxa2xx_lcd.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/hw/display/pxa2xx_lcd.c b/hw/display/pxa2xx_lcd.c
> index 80edb70..611fb17 100644
> --- a/hw/display/pxa2xx_lcd.c
> +++ b/hw/display/pxa2xx_lcd.c
> @@ -620,24 +620,24 @@ 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 & 0xf80000) >> 16;
> +                g = (*(uint32_t *) src & 0x00fc00) >> 8;
> +                b = (*(uint32_t *) src & 0x0000f8);
>              }
> -            src += 2;
> +            src += 4;
>              break;
>          case 2: /* 18 bpp plus transparency */
>              alpha = *(uint32_t *) src & (1 << 24);
>              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
>
>