mbox series

[0/9] drm/panel: simple: improve definition of display modes and add more panels

Message ID 1507721021-28174-1-git-send-email-LW@KARO-electronics.de
Headers show
Series drm/panel: simple: improve definition of display modes and add more panels | expand

Message

Lothar Waßmann Oct. 11, 2017, 11:23 a.m. UTC 
The first two patches of this patchset simplify the definition of
display modes in the driver and make it less error prone.

Patch 3 & 4 add support for overriding certain settings defined in the
panel definitions via DT entries to match up HW interfacing variants.

The remaining patches add support for various panels that may be used
with the Ka-Ro electronics TX module series.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Thierry Reding Oct. 17, 2017, 12:08 p.m. UTC | #1 
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@KARO-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.

Thierry
Thierry Reding Oct. 17, 2017, 12:09 p.m. UTC | #2 
On Wed, Oct 11, 2017 at 01:23:34PM +0200, Lothar Waßmann wrote:
> Use the newly defined macro to generate the display_mode data entries
> for all panels. This reduces the code size significantly and makes the
> code more readable.
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 799 ++++++-----------------------------
>  1 file changed, 134 insertions(+), 665 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index dec639d..fde9c41 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -89,6 +89,20 @@ struct panel_simple {
>  	struct gpio_desc *enable_gpio;
>  };
>  
> +#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,							\
> +}
> +
>  static inline struct panel_simple *to_panel_simple(struct drm_panel *panel)
>  {
>  	return container_of(panel, struct panel_simple, base);
[...]
> @@ -411,33 +415,9 @@ 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,							\
> -	}

Your first patch should put this in the right place to begin with so
that this patch is really just the conversion.

Again, I don't think this macro actually improves the way modes are
defined.

Thierry
Lothar Waßmann Oct. 17, 2017, 12:13 p.m. UTC | #3 
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@KARO-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.


Lothar Waßmann
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding Oct. 17, 2017, 12:50 p.m. UTC | #4 
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@KARO-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
Lothar Waßmann Oct. 17, 2017, 1:05 p.m. UTC | #5 
Hi,

On Tue, 17 Oct 2017 14:09:37 +0200 Thierry Reding wrote:
> On Wed, Oct 11, 2017 at 01:23:34PM +0200, Lothar Waßmann wrote:
> > Use the newly defined macro to generate the display_mode data entries
> > for all panels. This reduces the code size significantly and makes the
> > code more readable.
> > 
> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > ---
> >  drivers/gpu/drm/panel/panel-simple.c | 799 ++++++-----------------------------
> >  1 file changed, 134 insertions(+), 665 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > index dec639d..fde9c41 100644
> > --- a/drivers/gpu/drm/panel/panel-simple.c
> > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > @@ -89,6 +89,20 @@ struct panel_simple {
> >  	struct gpio_desc *enable_gpio;
> >  };
> >  
> > +#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,							\
> > +}
> > +
> >  static inline struct panel_simple *to_panel_simple(struct drm_panel *panel)
> >  {
> >  	return container_of(panel, struct panel_simple, base);
> [...]
> > @@ -411,33 +415,9 @@ 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,							\
> > -	}
> 
> Your first patch should put this in the right place to begin with so
> that this patch is really just the conversion.
> 
> Again, I don't think this macro actually improves the way modes are
> defined.
> 
I'm not happy with this panel driver stuff anyway. With the legacy
'display-timings' node that provided the timing data directly in the
DTB, every bootloader could pick up the timing data and feed it to
whatever driver it used for the display.
With the panel driver stuff the whole Linux driver has to be replicated
in the boot loader in order to be able to use the same DTB as Linux for
its HW configuration.
And adding a new panel involves recompiling the kernel and the boot
loader, rather than adding the timing data from the panel's datasheet
into the DTB.


Lothar Waßmann
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding Oct. 17, 2017, 1:08 p.m. UTC | #6 
On Tue, Oct 17, 2017 at 03:05:16PM +0200, Lothar Waßmann wrote:
> Hi,
> 
> On Tue, 17 Oct 2017 14:09:37 +0200 Thierry Reding wrote:
> > On Wed, Oct 11, 2017 at 01:23:34PM +0200, Lothar Waßmann wrote:
> > > Use the newly defined macro to generate the display_mode data entries
> > > for all panels. This reduces the code size significantly and makes the
> > > code more readable.
> > > 
> > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > > ---
> > >  drivers/gpu/drm/panel/panel-simple.c | 799 ++++++-----------------------------
> > >  1 file changed, 134 insertions(+), 665 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > > index dec639d..fde9c41 100644
> > > --- a/drivers/gpu/drm/panel/panel-simple.c
> > > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > > @@ -89,6 +89,20 @@ struct panel_simple {
> > >  	struct gpio_desc *enable_gpio;
> > >  };
> > >  
> > > +#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,							\
> > > +}
> > > +
> > >  static inline struct panel_simple *to_panel_simple(struct drm_panel *panel)
> > >  {
> > >  	return container_of(panel, struct panel_simple, base);
> > [...]
> > > @@ -411,33 +415,9 @@ 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,							\
> > > -	}
> > 
> > Your first patch should put this in the right place to begin with so
> > that this patch is really just the conversion.
> > 
> > Again, I don't think this macro actually improves the way modes are
> > defined.
> > 
> I'm not happy with this panel driver stuff anyway. With the legacy
> 'display-timings' node that provided the timing data directly in the
> DTB, every bootloader could pick up the timing data and feed it to
> whatever driver it used for the display.
> With the panel driver stuff the whole Linux driver has to be replicated
> in the boot loader in order to be able to use the same DTB as Linux for
> its HW configuration.
> And adding a new panel involves recompiling the kernel and the boot
> loader, rather than adding the timing data from the panel's datasheet
> into the DTB.

This isn't the first time I've heard this. Please read this for more
background information on why the situation is what it is:

	http://sietch-tagr.blogspot.de/2016/04/display-panels-are-not-special.html

Thierry
Lothar Waßmann Oct. 17, 2017, 1:17 p.m. UTC | #7 
Hi,

On Tue, 17 Oct 2017 15:08:31 +0200 Thierry Reding wrote:
> On Tue, Oct 17, 2017 at 03:05:16PM +0200, Lothar Waßmann wrote:
> > Hi,
> > 
> > On Tue, 17 Oct 2017 14:09:37 +0200 Thierry Reding wrote:
> > > On Wed, Oct 11, 2017 at 01:23:34PM +0200, Lothar Waßmann wrote:
> > > > Use the newly defined macro to generate the display_mode data entries
> > > > for all panels. This reduces the code size significantly and makes the
> > > > code more readable.
> > > > 
> > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > > > ---
> > > >  drivers/gpu/drm/panel/panel-simple.c | 799 ++++++-----------------------------
> > > >  1 file changed, 134 insertions(+), 665 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > > > index dec639d..fde9c41 100644
> > > > --- a/drivers/gpu/drm/panel/panel-simple.c
> > > > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > > > @@ -89,6 +89,20 @@ struct panel_simple {
> > > >  	struct gpio_desc *enable_gpio;
> > > >  };
> > > >  
> > > > +#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,							\
> > > > +}
> > > > +
> > > >  static inline struct panel_simple *to_panel_simple(struct drm_panel *panel)
> > > >  {
> > > >  	return container_of(panel, struct panel_simple, base);
> > > [...]
> > > > @@ -411,33 +415,9 @@ 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,							\
> > > > -	}
> > > 
> > > Your first patch should put this in the right place to begin with so
> > > that this patch is really just the conversion.
> > > 
> > > Again, I don't think this macro actually improves the way modes are
> > > defined.
> > > 
> > I'm not happy with this panel driver stuff anyway. With the legacy
> > 'display-timings' node that provided the timing data directly in the
> > DTB, every bootloader could pick up the timing data and feed it to
> > whatever driver it used for the display.
> > With the panel driver stuff the whole Linux driver has to be replicated
> > in the boot loader in order to be able to use the same DTB as Linux for
> > its HW configuration.
> > And adding a new panel involves recompiling the kernel and the boot
> > loader, rather than adding the timing data from the panel's datasheet
> > into the DTB.
> 
> This isn't the first time I've heard this. Please read this for more
> background information on why the situation is what it is:
> 
> 	http://sietch-tagr.blogspot.de/2016/04/display-panels-are-not-special.html
> 
Thanks for the link. That's what I have been looking for already for
some time but couldn't find any references to.


Lothar Waßmann
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html