From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c09::22a; helo=mail-wm0-x22a.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x22a.google.com (mail-wm0-x22a.google.com [IPv6:2a00:1450:400c:c09::22a]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id AE8522095D2CE for ; Tue, 17 Oct 2017 08:36:48 -0700 (PDT) Received: by mail-wm0-x22a.google.com with SMTP id t69so4753826wmt.2 for ; Tue, 17 Oct 2017 08:40:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=L86znHOWifEvSGTIfbUdFxrS5c50BymUxnJfVry/vtQ=; b=RIyJJzOOE+Z+6ur+1cHf3MAgPB9+LkHdIiNIpX7k9kz1t+g847cUBYt34gG+Vqb5kg qaTIawQWfs22VYaNByFeSzpWVG32ZuzvbXrTB80O/YaSxStJIdY4fyqJhLUbQ7VG+z8J xe/cIbgb24rLgKkTknKpXWCRdTcCSod8XikhU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=L86znHOWifEvSGTIfbUdFxrS5c50BymUxnJfVry/vtQ=; b=gK/bzXibs853VDHPK41eZqllcuV25Y1owssDtWHV8vZ9jpDVynnaDgm8xEy7fXETeS 9MRlMJiSmLncnFzUNz3YMpM30FQKgIpNJ0lYO7mO0+tKo+CO2y5XWjItTzIBFQrkUQCz QfL69Qda5iK8RhOfUK4oTl3ZSSMmD0mvW9p+TD+0HJg8Id63PgcWfX/3PRVOlt8njjfY FfompEJaErooSei1sCG4MbQAPAmGma+pETUp7bGOh0aAKbkuz1zqZqq+4+P6E/h46UKJ KX60iD4ELl1cybJhdmxSInaCWGV0AD5Bw6hntvIYV+x4lvYJgqX+uCHwi6DgZRVOktR8 ICcA== X-Gm-Message-State: AMCzsaWjUsx59ujGR/ghsqCprxDF5J1pImVckhXMsRP361K0p3LI05wj z9bv5XtVlKeNkBxLcMrt7VtXzqz2vQI= X-Google-Smtp-Source: ABhQp+TbGNmmUJYHsRWaLNOLgATrEgJllcA6cFeXfRf1gbg+C6y77oDSxcsjnqV6pSczVmW/kNYSWg== X-Received: by 10.28.74.89 with SMTP id x86mr3921724wma.57.1508254823198; Tue, 17 Oct 2017 08:40:23 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id e8sm9391035wmc.46.2017.10.17.08.40.21 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 17 Oct 2017 08:40:21 -0700 (PDT) Date: Tue, 17 Oct 2017 16:40:19 +0100 From: Leif Lindholm To: Evan Lloyd Cc: "edk2-devel@lists.01.org" Message-ID: <20171017154019.vcm7qgywa44i4zt6@bivouac.eciton.net> References: <20170926201529.11644-1-evan.lloyd@arm.com> <20170926201529.11644-14-evan.lloyd@arm.com> <20171013075303.crtv45upaupgscxx@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH 13/19] ArmPlatformPkg: HdLcd Remove redundant Bpp X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Oct 2017 15:36:49 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > > 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 > > > > > > 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 > > > Signed-off-by: Evan Lloyd > > > --- > > > > > > > > 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.