linux-kernel - Re: [PATCH 1/9] drm/panel: simple: add a macro for defining display modes in a simpler and less error prone way

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171017125045.GD27983@ulmo>
Date:   Tue, 17 Oct 2017 14:50:45 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Lothar Waßmann <LW@...O-electronics.de>
Cc:     David Airlie <airlied@...ux.ie>,
        Mark Rutland <mark.rutland@....com>,
        Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/9] drm/panel: simple: add a macro for defining display
 modes in a simpler and less error prone way

On Tue, Oct 17, 2017 at 02:13:37PM +0200, Lothar Waßmann wrote:
> Hi,
> 
> On Tue, 17 Oct 2017 14:08:18 +0200 Thierry Reding wrote:
> > On Wed, Oct 11, 2017 at 01:23:33PM +0200, Lothar Waßmann wrote:
> > > Create a macro that eases the definition of display mode parameters by
> > > accecpting the parameters:
> > > freq, hactive, hfront-porch, hsynclen, hback-porch,
> > > vactive, vfront-porch, vsynclen, vback-porch, vrefresh
> > > that can be usually directly taken from an LCD datasheet.
> > > 
> > > Put the calculations that are now open coded repeating the same
> > > parameters multiple times into the macro expansion.
> > > 
> > > Signed-off-by: Lothar Waßmann <LW@...O-electronics.de>
> > > ---
> > >  drivers/gpu/drm/panel/panel-simple.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > > index 474fa75..dec639d 100644
> > > --- a/drivers/gpu/drm/panel/panel-simple.c
> > > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > > @@ -411,6 +411,20 @@ static const struct panel_desc ampire_am_480272h3tmqw_t01h = {
> > >  	.bus_format = MEDIA_BUS_FMT_RGB888_1X24,
> > >  };
> > >  
> > > +#define SP_DISPLAY_MODE(freq, ha, hfp, hs, hbp, va, vfp, vs, vbp, vr, flgs) { \
> > > +	.clock = freq,							\
> > > +	.hdisplay = ha,							\
> > > +	.hsync_start = (ha) + (hfp),					\
> > > +	.hsync_end = (ha) + (hfp) + (hs),				\
> > > +	.htotal = (ha) + (hfp) + (hs) + (hbp),				\
> > > +	.vdisplay = (va),						\
> > > +	.vsync_start = (va) + (vfp),					\
> > > +	.vsync_end = (va) + (vfp) + (vs),				\
> > > +	.vtotal = (va) + (vfp) + (vs) + (vbp),				\
> > > +	.vrefresh = vr,							\
> > > +	.flags = flgs,							\
> > > +	}
> > 
> > I don't think this simplifies anything. It's now completely non-obvious
> > which parameter is which, so you actually have to go look at the macro
> > definition when you add a new mode to make sure you get them right.
> > 
> In the original code you have to repeat the same parameters (e.g.
> vertical front porch (vfp)) in multiple places which is error prone.

I don't think this is an issue. The definitions initialize the fields in
the natural order, so the repetitions are where very localized and quite
difficult to actually get wrong. I've never seen anyone get it wrong for
any panel.

Also note that the above notation is only out of convenience to make it
clearer what the active, front-porch, sync and back-porch values are.
You can easily just collapse them into the sum if you're worried about
the repetition.

Alternatively, are you aware of struct display_timing? It's a different
way to describe the timings which doesn't require repeating the values.
simple-panel already supports those as an alternative to the struct
drm_display_mode.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)