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

[Leif Lindholm <leif.lindholm@linaro.org>]

On Tue, Oct 17, 2017 at 02:32:05PM +0000, Evan Lloyd wrote:
> 
> 
> > -----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.

This rework is minor. If you or your team can use some help with the
git trickery that makes it simple, I'm happy to assist.

If you trust me, you can pick the result from
https://git.linaro.org/people/leif.lindholm/edk2.git/log/?h=166_gop_v1
(but the whole point of this is that I don't understand the changeset
up front, so I may have bungled something)

If your project schedule does not include the time to rework and
re-test patchsets, I'll have a strong word with your product manager
:)

> Do you really need
> this order change?  How would you feel about our rolling them both
> together?

Not ideal. One is trivial cleanup, the other is an API change -
squashing them together makes it a lot less clear which is which, and
hence a lot harder to perform any sensible level of code review.

Also a heads up: when you rebase this series to edk2/master, you'll
encounter a breakage caused by
gArmPlatformTokenSpaceGuid.PcdArmLcdDdrFrameBufferSize being given the
same TokenValue as (now upstream)
gArmPlatformTokenSpaceGuid.PcdWatchdogCount.

/
    Leif

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