Re: [PATCH 13/19] ArmPlatformPkg: HdLcd Remove redundant Bpp

[Evan Lloyd <Evan.Lloyd@arm.com>]

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: 13 October 2017 08:53
> To: Evan Lloyd <Evan.Lloyd@arm.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [PATCH 13/19] ArmPlatformPkg: HdLcd Remove redundant Bpp
>
> On Tue, Sep 26, 2017 at 09:15:23PM +0100, evan.lloyd@arm.com wrote:
> > From: EvanLloyd <evan.lloyd@arm.com>
> >
> > Because of copy/paste effects, HdLcdArmVExpress.c contained a table
> > entry "LCD_BPP Bpp;" specifying the Bits per Pixel for each mode.
> > However, all modes are LCD_BITS_PER_PIXEL_24.
> >
> > This change removes the table entry and related use of the field.
>
> Since it looks to be modifying some of the same lines, I would prefer to see
> this patch before the previous one. Or am I misreading that?

[[Evan Lloyd]] It does modify some shared lines.  In practice this change became obvious once the previous one had been made.  I'm not sure I understand what benefit you see in changing the order.  I am, however, sure of the cost of rework and re-test.  Do you really need this order change?  How would you feel about our rolling them both together?

Evan

>
> /
>     Leif
>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Girish Pathak <girish.pathak@arm.com>
> > Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> > ---
> >
> >
> ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArm
> VExp
> > ress.c | 42 ++++++--------------
> >  1 file changed, 13 insertions(+), 29 deletions(-)
> >
> > diff --git
> >
> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdAr
> mVE
> > xpress.c
> >
> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdAr
> mVE
> > xpress.c index
> >
> dc2c5fb89923304c46d137ec8eaefc9418548d06..2401cdb30cb7252964ce1f36
> 3aa2
> > 6d99933c09be 100644
> > ---
> >
> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdAr
> mVE
> > xpress.c
> > +++
> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdA
> > +++ rmVExpress.c
> > @@ -30,7 +30,6 @@
> >
> >  typedef struct {
> >    UINT32                     Mode;
> > -  LCD_BPP                    Bpp;
> >    UINT32                     OscFreq;
> >
> >    // These are used by HDLCD
> > @@ -42,37 +41,37 @@ typedef struct {
> >  **/
> >  STATIC CONST DISPLAY_MODE mDisplayModes[] = {
> >    { // Mode 0 : VGA : 640 x 480 x 24 bpp
> > -    VGA, LCD_BITS_PER_PIXEL_24,
> > +    VGA,
> >      VGA_OSC_FREQUENCY,
> >      {VGA_H_RES_PIXELS, VGA_H_SYNC, VGA_H_BACK_PORCH,
> VGA_H_FRONT_PORCH},
> >      {VGA_V_RES_PIXELS, VGA_V_SYNC, VGA_V_BACK_PORCH,
> VGA_V_FRONT_PORCH}
> >    },
> >    { // Mode 1 : SVGA : 800 x 600 x 24 bpp
> > -    SVGA, LCD_BITS_PER_PIXEL_24,
> > +    SVGA,
> >      SVGA_OSC_FREQUENCY,
> >      {SVGA_H_RES_PIXELS, SVGA_H_SYNC, SVGA_H_BACK_PORCH,
> SVGA_H_FRONT_PORCH},
> >      {SVGA_V_RES_PIXELS, SVGA_V_SYNC, SVGA_V_BACK_PORCH,
> SVGA_V_FRONT_PORCH}
> >    },
> >    { // Mode 2 : XGA : 1024 x 768 x 24 bpp
> > -    XGA, LCD_BITS_PER_PIXEL_24,
> > +    XGA,
> >      XGA_OSC_FREQUENCY,
> >      {XGA_H_RES_PIXELS, XGA_H_SYNC, XGA_H_BACK_PORCH,
> XGA_H_FRONT_PORCH},
> >      {XGA_V_RES_PIXELS, XGA_V_SYNC, XGA_V_BACK_PORCH,
> XGA_V_FRONT_PORCH}
> >    },
> >    { // Mode 3 : SXGA : 1280 x 1024 x 24 bpp
> > -    SXGA, LCD_BITS_PER_PIXEL_24,
> > +    SXGA,
> >      (SXGA_OSC_FREQUENCY/2),
> >      {SXGA_H_RES_PIXELS, SXGA_H_SYNC, SXGA_H_BACK_PORCH,
> SXGA_H_FRONT_PORCH},
> >      {SXGA_V_RES_PIXELS, SXGA_V_SYNC, SXGA_V_BACK_PORCH,
> SXGA_V_FRONT_PORCH}
> >    },
> >    { // Mode 4 : UXGA : 1600 x 1200 x 24 bpp
> > -    UXGA, LCD_BITS_PER_PIXEL_24,
> > +    UXGA,
> >      (UXGA_OSC_FREQUENCY/2),
> >      {UXGA_H_RES_PIXELS, UXGA_H_SYNC, UXGA_H_BACK_PORCH,
> UXGA_H_FRONT_PORCH},
> >      {UXGA_V_RES_PIXELS, UXGA_V_SYNC, UXGA_V_BACK_PORCH,
> UXGA_V_FRONT_PORCH}
> >    },
> >    { // Mode 5 : HD : 1920 x 1080 x 24 bpp
> > -    HD, LCD_BITS_PER_PIXEL_24,
> > +    HD,
> >      (HD_OSC_FREQUENCY/2),
> >      {HD_H_RES_PIXELS, HD_H_SYNC, HD_H_BACK_PORCH,
> HD_H_FRONT_PORCH},
> >      {HD_V_RES_PIXELS, HD_V_SYNC, HD_V_BACK_PORCH,
> HD_V_FRONT_PORCH}
> > @@ -283,27 +282,12 @@ LcdPlatformQueryMode (
> >    Info->VerticalResolution =
> mDisplayModes[ModeNumber].Vertical.Resolution;
> >    Info->PixelsPerScanLine =
> > mDisplayModes[ModeNumber].Horizontal.Resolution;
> >
> > -  switch (mDisplayModes[ModeNumber].Bpp) {
> > -    case LCD_BITS_PER_PIXEL_24:
> > -      Info->PixelFormat                   =
> PixelRedGreenBlueReserved8BitPerColor;
> > -      Info->PixelInformation.RedMask      = LCD_24BPP_RED_MASK;
> > -      Info->PixelInformation.GreenMask    = LCD_24BPP_GREEN_MASK;
> > -      Info->PixelInformation.BlueMask     = LCD_24BPP_BLUE_MASK;
> > -      Info->PixelInformation.ReservedMask = LCD_24BPP_RESERVED_MASK;
> > -      break;
> > -
> > -    case LCD_BITS_PER_PIXEL_16_555:
> > -    case LCD_BITS_PER_PIXEL_16_565:
> > -    case LCD_BITS_PER_PIXEL_12_444:
> > -    case LCD_BITS_PER_PIXEL_8:
> > -    case LCD_BITS_PER_PIXEL_4:
> > -    case LCD_BITS_PER_PIXEL_2:
> > -    case LCD_BITS_PER_PIXEL_1:
> > -    default:
> > -      // These are not supported
> > -      ASSERT (FALSE);
> > -      break;
> > -  }
> > +  /* Bits per Pixel is always LCD_BITS_PER_PIXEL_24 */
> > +  Info->PixelFormat                   = PixelRedGreenBlueReserved8BitPerColor;
> > +  Info->PixelInformation.RedMask      = LCD_24BPP_RED_MASK;
> > +  Info->PixelInformation.GreenMask    = LCD_24BPP_GREEN_MASK;
> > +  Info->PixelInformation.BlueMask     = LCD_24BPP_BLUE_MASK;
> > +  Info->PixelInformation.ReservedMask = LCD_24BPP_RESERVED_MASK;
> >
> >    return EFI_SUCCESS;
> >  }
> > @@ -365,7 +349,7 @@ LcdPlatformGetBpp (
> >      return EFI_INVALID_PARAMETER;
> >    }
> >
> > -  *Bpp = mDisplayModes[ModeNumber].Bpp;
> > +  *Bpp = LCD_BITS_PER_PIXEL_24;
> >
> >    return EFI_SUCCESS;
> >  }
> > --
> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> >
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.