Re: [PATCH 1/3] drm/panel: simple: Add ability to override typical timing

[Sean Paul]

On Wed, Feb 07, 2018 at 11:41:49AM -0600, Rob Herring wrote:
> On Tue, Feb 6, 2018 at 3:48 PM, Sean Paul  wrote:
> > On Tue, Feb 06, 2018 at 02:19:34PM -0600, Rob Herring wrote:
> >> On Tue, Feb 6, 2018 at 10:56 AM, Sean Paul  wrote:
> >> > This patch adds the ability to override the typical display timing for a
> >> > given panel. This is useful for devices which have timing constraints
> >> > that do not apply across the entire display driver (eg: to avoid
> >> > crosstalk between panel and digitizer on certain laptops). The rules are
> >> > as follows:
> >> >
> >> > - panel must not specify fixed mode (since the override mode will
> >> >   either be the same as the fixed mode, or we'll be unable to
> >> >   check the bounds of the overried)
> >> > - panel must specify at least one display_timing range which will be
> >> >   used to ensure the override mode fits within its bounds
> >> >
> >> > Cc: Doug Anderson 
> >> > Cc: Heiko Stuebner 
> >> > Cc: Jeffy Chen 
> >> > Cc: Rob Herring 
> >> > Cc: Stéphane Marchesin 
> >> > Cc: Thierry Reding 
> >> > Cc: devicet...@vger.kernel.org
> >> > Cc: dri-devel@lists.freedesktop.org
> >> > Cc: linux-rockc...@lists.infradead.org
> >> > Signed-off-by: Sean Paul 
> >> > ---
> >> >  .../bindings/display/panel/simple-panel.txt| 20 +++
> >>
> >> The binding should be a separate patch.
> >>
> >
> > Ack, will split.
> >
> >
> >> >  drivers/gpu/drm/panel/panel-simple.c   | 69 
> >> > +-
> >> >  2 files changed, 88 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git 
> >> > a/Documentation/devicetree/bindings/display/panel/simple-panel.txt 
> >> > b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> >> > index 16d8ff088b7d..590bbff6fc90 100644
> >> > --- a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> >> > +++ b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> >> > @@ -7,6 +7,14 @@ Optional properties:
> >> >  - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
> >> >  - enable-gpios: GPIO pin to enable or disable the panel
> >> >  - backlight: phandle of the backlight device attached to the panel
> >> > +- override-mode: For devices which require a mode which differs from the
> >>
> >> This is not a property, but a node so it needs its own section.
> >>
> >> Also, it's not real clear from display-timing.txt, but the modes
> >> should be grouped under a display-timings node. Looks like we haven't
> >> been good at enforcing that as "panel-timing" is also common when
> >> there's a single mode. I'd rather not have another way. With a
> >> standard node name, we can validate the node more easily.
> >>
> >> We'd lose the fact that this is explicitly an override, but I'd doubt
> >> Thierry is going to start letting in panels with no timings.
> >>
> >
> > Yeah, I noticed that the timing subnode was specified as nestled in
> > display-timings. I figured since there can only be one override mode, and 
> > the
> > of_get_display_timing function was exported, it would be Ok to just reuse 
> > the
> > format of the subnode. I'll respin with the full thing.
> 
> TBC, I'm fine if you use panel-timing as that's already established
> for cases were only 1 mode exists.

Ah! So the dt change you were asking for was just s/override-mode/panel-timing/.
I'll respin a v3 with the improved documentation and reinstate the 
"panel-timing"
subnode.

Sean

> 
> Rob

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/panel: simple: Add ability to override typical timing

[Rob Herring]

On Tue, Feb 6, 2018 at 3:48 PM, Sean Paul  wrote:
> On Tue, Feb 06, 2018 at 02:19:34PM -0600, Rob Herring wrote:
>> On Tue, Feb 6, 2018 at 10:56 AM, Sean Paul  wrote:
>> > This patch adds the ability to override the typical display timing for a
>> > given panel. This is useful for devices which have timing constraints
>> > that do not apply across the entire display driver (eg: to avoid
>> > crosstalk between panel and digitizer on certain laptops). The rules are
>> > as follows:
>> >
>> > - panel must not specify fixed mode (since the override mode will
>> >   either be the same as the fixed mode, or we'll be unable to
>> >   check the bounds of the overried)
>> > - panel must specify at least one display_timing range which will be
>> >   used to ensure the override mode fits within its bounds
>> >
>> > Cc: Doug Anderson 
>> > Cc: Heiko Stuebner 
>> > Cc: Jeffy Chen 
>> > Cc: Rob Herring 
>> > Cc: Stéphane Marchesin 
>> > Cc: Thierry Reding 
>> > Cc: devicet...@vger.kernel.org
>> > Cc: dri-devel@lists.freedesktop.org
>> > Cc: linux-rockc...@lists.infradead.org
>> > Signed-off-by: Sean Paul 
>> > ---
>> >  .../bindings/display/panel/simple-panel.txt| 20 +++
>>
>> The binding should be a separate patch.
>>
>
> Ack, will split.
>
>
>> >  drivers/gpu/drm/panel/panel-simple.c   | 69 
>> > +-
>> >  2 files changed, 88 insertions(+), 1 deletion(-)
>> >
>> > diff --git 
>> > a/Documentation/devicetree/bindings/display/panel/simple-panel.txt 
>> > b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
>> > index 16d8ff088b7d..590bbff6fc90 100644
>> > --- a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
>> > +++ b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
>> > @@ -7,6 +7,14 @@ Optional properties:
>> >  - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
>> >  - enable-gpios: GPIO pin to enable or disable the panel
>> >  - backlight: phandle of the backlight device attached to the panel
>> > +- override-mode: For devices which require a mode which differs from the
>>
>> This is not a property, but a node so it needs its own section.
>>
>> Also, it's not real clear from display-timing.txt, but the modes
>> should be grouped under a display-timings node. Looks like we haven't
>> been good at enforcing that as "panel-timing" is also common when
>> there's a single mode. I'd rather not have another way. With a
>> standard node name, we can validate the node more easily.
>>
>> We'd lose the fact that this is explicitly an override, but I'd doubt
>> Thierry is going to start letting in panels with no timings.
>>
>
> Yeah, I noticed that the timing subnode was specified as nestled in
> display-timings. I figured since there can only be one override mode, and the
> of_get_display_timing function was exported, it would be Ok to just reuse the
> format of the subnode. I'll respin with the full thing.

TBC, I'm fine if you use panel-timing as that's already established
for cases were only 1 mode exists.

Rob
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/panel: simple: Add ability to override typical timing

[Thierry Reding]

On Tue, Feb 06, 2018 at 02:19:34PM -0600, Rob Herring wrote:
> On Tue, Feb 6, 2018 at 10:56 AM, Sean Paul  wrote:
> > This patch adds the ability to override the typical display timing for a
> > given panel. This is useful for devices which have timing constraints
> > that do not apply across the entire display driver (eg: to avoid
> > crosstalk between panel and digitizer on certain laptops). The rules are
> > as follows:
> >
> > - panel must not specify fixed mode (since the override mode will
> >   either be the same as the fixed mode, or we'll be unable to
> >   check the bounds of the overried)
> > - panel must specify at least one display_timing range which will be
> >   used to ensure the override mode fits within its bounds
> >
> > Cc: Doug Anderson 
> > Cc: Heiko Stuebner 
> > Cc: Jeffy Chen 
> > Cc: Rob Herring 
> > Cc: Stéphane Marchesin 
> > Cc: Thierry Reding 
> > Cc: devicet...@vger.kernel.org
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linux-rockc...@lists.infradead.org
> > Signed-off-by: Sean Paul 
> > ---
> >  .../bindings/display/panel/simple-panel.txt| 20 +++
> 
> The binding should be a separate patch.
> 
> >  drivers/gpu/drm/panel/panel-simple.c   | 69 
> > +-
> >  2 files changed, 88 insertions(+), 1 deletion(-)
> >
> > diff --git 
> > a/Documentation/devicetree/bindings/display/panel/simple-panel.txt 
> > b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> > index 16d8ff088b7d..590bbff6fc90 100644
> > --- a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> > +++ b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> > @@ -7,6 +7,14 @@ Optional properties:
> >  - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
> >  - enable-gpios: GPIO pin to enable or disable the panel
> >  - backlight: phandle of the backlight device attached to the panel
> > +- override-mode: For devices which require a mode which differs from the
> 
> This is not a property, but a node so it needs its own section.
> 
> Also, it's not real clear from display-timing.txt, but the modes
> should be grouped under a display-timings node. Looks like we haven't
> been good at enforcing that as "panel-timing" is also common when
> there's a single mode. I'd rather not have another way. With a
> standard node name, we can validate the node more easily.
> 
> We'd lose the fact that this is explicitly an override, but I'd doubt
> Thierry is going to start letting in panels with no timings.

I was actually going to suggest the same change. I don't mind if the
name isn't explicit about this being an override. The important thing is
that we document this as being an override.

> Finally, since this is an override, is it valid to only override the
> parameters that need overriding? I don't really have an opinion either
> way. It just needs to be explicitly documented.

I've thought about that, but I think that'd be putting too much of a
burden on the panel drivers and/or display drivers. In the simplest case
it may be sufficient to restrict the pixel clock, in which case it would
be fairly easy for the driver to adjust the other parameters.

But what if you also need to restrict the porches. At some point it'll
become very difficult for the driver to automatically determine which of
the other parameters to adjust and how. Chances are that whoever needs
to deal with the restrictions already knows how to adjust the parameters
to fixup the mode.

I think this gives us a nice balance of simplicity when people don't
care (they'd just use the typical timings) and flexibility when they do
(adjust the mode to take into account display driver restrictions and
completely specify the mode if the restrictions are too complicated for
code to realistically apply them).

Thierry


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/panel: simple: Add ability to override typical timing

[Sean Paul]

On Tue, Feb 06, 2018 at 02:19:34PM -0600, Rob Herring wrote:
> On Tue, Feb 6, 2018 at 10:56 AM, Sean Paul  wrote:
> > This patch adds the ability to override the typical display timing for a
> > given panel. This is useful for devices which have timing constraints
> > that do not apply across the entire display driver (eg: to avoid
> > crosstalk between panel and digitizer on certain laptops). The rules are
> > as follows:
> >
> > - panel must not specify fixed mode (since the override mode will
> >   either be the same as the fixed mode, or we'll be unable to
> >   check the bounds of the overried)
> > - panel must specify at least one display_timing range which will be
> >   used to ensure the override mode fits within its bounds
> >
> > Cc: Doug Anderson 
> > Cc: Heiko Stuebner 
> > Cc: Jeffy Chen 
> > Cc: Rob Herring 
> > Cc: Stéphane Marchesin 
> > Cc: Thierry Reding 
> > Cc: devicet...@vger.kernel.org
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linux-rockc...@lists.infradead.org
> > Signed-off-by: Sean Paul 
> > ---
> >  .../bindings/display/panel/simple-panel.txt| 20 +++
> 
> The binding should be a separate patch.
> 

Ack, will split.


> >  drivers/gpu/drm/panel/panel-simple.c   | 69 
> > +-
> >  2 files changed, 88 insertions(+), 1 deletion(-)
> >
> > diff --git 
> > a/Documentation/devicetree/bindings/display/panel/simple-panel.txt 
> > b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> > index 16d8ff088b7d..590bbff6fc90 100644
> > --- a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> > +++ b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> > @@ -7,6 +7,14 @@ Optional properties:
> >  - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
> >  - enable-gpios: GPIO pin to enable or disable the panel
> >  - backlight: phandle of the backlight device attached to the panel
> > +- override-mode: For devices which require a mode which differs from the
> 
> This is not a property, but a node so it needs its own section.
> 
> Also, it's not real clear from display-timing.txt, but the modes
> should be grouped under a display-timings node. Looks like we haven't
> been good at enforcing that as "panel-timing" is also common when
> there's a single mode. I'd rather not have another way. With a
> standard node name, we can validate the node more easily.
> 
> We'd lose the fact that this is explicitly an override, but I'd doubt
> Thierry is going to start letting in panels with no timings.
> 

Yeah, I noticed that the timing subnode was specified as nestled in
display-timings. I figured since there can only be one override mode, and the
of_get_display_timing function was exported, it would be Ok to just reuse the
format of the subnode. I'll respin with the full thing.


> Finally, since this is an override, is it valid to only override the
> parameters that need overriding? I don't really have an opinion either
> way. It just needs to be explicitly documented.

I'll pimp the documentation. My gut reaction is to specify everything, since
this should be a very conscious decision, and having to fully specify the mode
seems like the right thing to do.

Thanks for your review!

Sean

> 
> > +display_timing's "typical" mode, and whose restrictions 
> > cannot
> > +be applied across the entire platform. The mode must fall
> > +within the bounds of the panel's specified display_timing, 
> > and
> > +cannot be used if the panel already specifies a single 
> > fixed
> > +mode. The format is specified under "timing subnode" in
> > +display-timing.txt
> > +
> >
> >  Example:
> >
> > @@ -18,4 +26,16 @@ Example:
> > enable-gpios = < 90 0>;
> >
> > backlight = <>;
> > +
> > +   override-mode {
> > +   clock-frequency = <266604720>;
> > +   hactive = <2400>;
> > +   hfront-porch = <48>;
> > +   hback-porch = <84>;
> > +   hsync-len = <32>;
> > +   vactive = <1600>;
> > +   vfront-porch = <3>;
> > +   vback-porch = <120>;
> > +   vsync-len = <10>;
> > +   }
> > };

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/panel: simple: Add ability to override typical timing

[Rob Herring]

On Tue, Feb 6, 2018 at 10:56 AM, Sean Paul  wrote:
> This patch adds the ability to override the typical display timing for a
> given panel. This is useful for devices which have timing constraints
> that do not apply across the entire display driver (eg: to avoid
> crosstalk between panel and digitizer on certain laptops). The rules are
> as follows:
>
> - panel must not specify fixed mode (since the override mode will
>   either be the same as the fixed mode, or we'll be unable to
>   check the bounds of the overried)
> - panel must specify at least one display_timing range which will be
>   used to ensure the override mode fits within its bounds
>
> Cc: Doug Anderson 
> Cc: Heiko Stuebner 
> Cc: Jeffy Chen 
> Cc: Rob Herring 
> Cc: Stéphane Marchesin 
> Cc: Thierry Reding 
> Cc: devicet...@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-rockc...@lists.infradead.org
> Signed-off-by: Sean Paul 
> ---
>  .../bindings/display/panel/simple-panel.txt| 20 +++

The binding should be a separate patch.

>  drivers/gpu/drm/panel/panel-simple.c   | 69 
> +-
>  2 files changed, 88 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/display/panel/simple-panel.txt 
> b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> index 16d8ff088b7d..590bbff6fc90 100644
> --- a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> +++ b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> @@ -7,6 +7,14 @@ Optional properties:
>  - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
>  - enable-gpios: GPIO pin to enable or disable the panel
>  - backlight: phandle of the backlight device attached to the panel
> +- override-mode: For devices which require a mode which differs from the

This is not a property, but a node so it needs its own section.

Also, it's not real clear from display-timing.txt, but the modes
should be grouped under a display-timings node. Looks like we haven't
been good at enforcing that as "panel-timing" is also common when
there's a single mode. I'd rather not have another way. With a
standard node name, we can validate the node more easily.

We'd lose the fact that this is explicitly an override, but I'd doubt
Thierry is going to start letting in panels with no timings.

Finally, since this is an override, is it valid to only override the
parameters that need overriding? I don't really have an opinion either
way. It just needs to be explicitly documented.

> +display_timing's "typical" mode, and whose restrictions 
> cannot
> +be applied across the entire platform. The mode must fall
> +within the bounds of the panel's specified display_timing, 
> and
> +cannot be used if the panel already specifies a single fixed
> +mode. The format is specified under "timing subnode" in
> +display-timing.txt
> +
>
>  Example:
>
> @@ -18,4 +26,16 @@ Example:
> enable-gpios = < 90 0>;
>
> backlight = <>;
> +
> +   override-mode {
> +   clock-frequency = <266604720>;
> +   hactive = <2400>;
> +   hfront-porch = <48>;
> +   hback-porch = <84>;
> +   hsync-len = <32>;
> +   vactive = <1600>;
> +   vfront-porch = <3>;
> +   vback-porch = <120>;
> +   vsync-len = <10>;
> +   }
> };
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm/panel: simple: Add ability to override typical timing

[Sean Paul]

This patch adds the ability to override the typical display timing for a
given panel. This is useful for devices which have timing constraints
that do not apply across the entire display driver (eg: to avoid
crosstalk between panel and digitizer on certain laptops). The rules are
as follows:

- panel must not specify fixed mode (since the override mode will
  either be the same as the fixed mode, or we'll be unable to
  check the bounds of the overried)
- panel must specify at least one display_timing range which will be
  used to ensure the override mode fits within its bounds

Cc: Doug Anderson 
Cc: Heiko Stuebner 
Cc: Jeffy Chen 
Cc: Rob Herring 
Cc: Stéphane Marchesin 
Cc: Thierry Reding 
Cc: devicet...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-rockc...@lists.infradead.org
Signed-off-by: Sean Paul 
---
 .../bindings/display/panel/simple-panel.txt| 20 +++
 drivers/gpu/drm/panel/panel-simple.c   | 69 +-
 2 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/panel/simple-panel.txt 
b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
index 16d8ff088b7d..590bbff6fc90 100644
--- a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
+++ b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
@@ -7,6 +7,14 @@ Optional properties:
 - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
 - enable-gpios: GPIO pin to enable or disable the panel
 - backlight: phandle of the backlight device attached to the panel
+- override-mode: For devices which require a mode which differs from the
+display_timing's "typical" mode, and whose restrictions cannot
+be applied across the entire platform. The mode must fall
+within the bounds of the panel's specified display_timing, and
+cannot be used if the panel already specifies a single fixed
+mode. The format is specified under "timing subnode" in
+display-timing.txt
+
 
 Example:
 
@@ -18,4 +26,16 @@ Example:
enable-gpios = < 90 0>;
 
backlight = <>;
+
+   override-mode {
+   clock-frequency = <266604720>;
+   hactive = <2400>;
+   hfront-porch = <48>;
+   hback-porch = <84>;
+   hsync-len = <32>;
+   vactive = <1600>;
+   vfront-porch = <3>;
+   vback-porch = <120>;
+   vsync-len = <10>;
+   }
};
diff --git a/drivers/gpu/drm/panel/panel-simple.c 
b/drivers/gpu/drm/panel/panel-simple.c
index 5591984a392b..b774365f3635 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -34,6 +34,7 @@
 #include 
 
 #include 
+#include 
 #include 
 
 struct panel_desc {
@@ -87,6 +88,8 @@ struct panel_simple {
struct i2c_adapter *ddc;
 
struct gpio_desc *enable_gpio;
+
+   struct drm_display_mode override_mode;
 };
 
 static inline struct panel_simple *to_panel_simple(struct drm_panel *panel)
@@ -99,11 +102,22 @@ static int panel_simple_get_fixed_modes(struct 
panel_simple *panel)
struct drm_connector *connector = panel->base.connector;
struct drm_device *drm = panel->base.drm;
struct drm_display_mode *mode;
+   bool has_override = panel->override_mode.type;
unsigned int i, num = 0;
 
if (!panel->desc)
return 0;
 
+   if (has_override) {
+   mode = drm_mode_duplicate(drm, >override_mode);
+   if (mode) {
+   drm_mode_probed_add(connector, mode);
+   num++;
+   } else {
+   dev_err(drm->dev, "failed to add override mode\n");
+   }
+   }
+
for (i = 0; i < panel->desc->num_timings; i++) {
const struct display_timing *dt = >desc->timings[i];
struct videomode vm;
@@ -120,7 +134,7 @@ static int panel_simple_get_fixed_modes(struct panel_simple 
*panel)
 
mode->type |= DRM_MODE_TYPE_DRIVER;
 
-   if (panel->desc->num_timings == 1)
+   if (panel->desc->num_timings == 1 && !has_override)
mode->type |= DRM_MODE_TYPE_PREFERRED;
 
drm_mode_probed_add(connector, mode);
@@ -291,10 +305,58 @@ static const struct drm_panel_funcs panel_simple_funcs = {
.get_timings = panel_simple_get_timings,
 };
 
+#define PANEL_SIMPLE_BOUNDS_CHECK(to_check, bounds, field) \
+   (to_check->field.typ >= bounds->field.min && \
+to_check->field.typ <= bounds->field.max)
+static void