[PATCH v2 0/5] Panel rotation patches

[PATCH v2 0/5] Panel rotation patches

[Derek Basehore]

This adds the plumbing for reading panel rotation from the devicetree
and sets up adding a panel property for the panel orientation on
Mediatek SoCs when a rotation is present.

v2 changes:
fixed build errors in i915

Derek Basehore (5):
  drm/panel: Add helper for reading DT rotation
  dt-bindings: display/panel: Expand rotation documentation
  drm/panel: Add attach/detach callbacks
  drm/connector: Split out orientation quirk detection
  drm/mtk: add panel orientation property

 .../bindings/display/panel/panel.txt          | 32 +++++++++++
 drivers/gpu/drm/drm_connector.c               | 16 ++----
 drivers/gpu/drm/drm_panel.c                   | 55 +++++++++++++++++++
 drivers/gpu/drm/i915/vlv_dsi.c                | 13 +++--
 drivers/gpu/drm/mediatek/mtk_dsi.c            |  8 +++
 include/drm/drm_connector.h                   |  2 +-
 include/drm/drm_panel.h                       | 11 ++++
 7 files changed, 120 insertions(+), 17 deletions(-)

-- 
2.22.0.rc2.383.gf4fbbf30c2-goog

[PATCH 1/5] drm/panel: Add helper for reading DT rotation

[Derek Basehore]

This adds a helper function for reading the rotation (panel
orientation) from the device tree.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 drivers/gpu/drm/drm_panel.c | 41 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_panel.h     |  7 +++++++
 2 files changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index dbd5b873e8f2..3b689ce4a51a 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -172,6 +172,47 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np)
 	return ERR_PTR(-EPROBE_DEFER);
 }
 EXPORT_SYMBOL(of_drm_find_panel);
+
+/**
+ * of_drm_get_panel_orientation - look up the rotation of the panel using a
+ * device tree node
+ * @np: device tree node of the panel
+ * @orientation: orientation enum to be filled in
+ *
+ * Looks up the rotation of a panel in the device tree. The rotation in the
+ * device tree is counter clockwise.
+ *
+ * Return: 0 when a valid rotation value (0, 90, 180, or 270) is read or the
+ * rotation property doesn't exist. -EERROR otherwise.
+ */
+int of_drm_get_panel_orientation(const struct device_node *np, int *orientation)
+{
+	int rotation, ret;
+
+	ret = of_property_read_u32(np, "rotation", &rotation);
+	if (ret == -EINVAL) {
+		/* Don't return an error if there's no rotation property. */
+		*orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
+		return 0;
+	}
+
+	if (ret < 0)
+		return ret;
+
+	if (rotation == 0)
+		*orientation = DRM_MODE_PANEL_ORIENTATION_NORMAL;
+	else if (rotation == 90)
+		*orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP;
+	else if (rotation == 180)
+		*orientation = DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP;
+	else if (rotation == 270)
+		*orientation = DRM_MODE_PANEL_ORIENTATION_LEFT_UP;
+	else
+		return -EINVAL;
+
+	return 0;
+}
+EXPORT_SYMBOL(of_drm_get_panel_orientation);
 #endif
 
 MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 8c738c0e6e9f..13631b2efbaa 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -197,11 +197,18 @@ int drm_panel_detach(struct drm_panel *panel);
 
 #if defined(CONFIG_OF) && defined(CONFIG_DRM_PANEL)
 struct drm_panel *of_drm_find_panel(const struct device_node *np);
+int of_drm_get_panel_orientation(const struct device_node *np,
+				 int *orientation);
 #else
 static inline struct drm_panel *of_drm_find_panel(const struct device_node *np)
 {
 	return ERR_PTR(-ENODEV);
 }
+int of_drm_get_panel_orientation(const struct device_node *np,
+				 int *orientation)
+{
+	return -ENODEV;
+}
 #endif
 
 #endif
-- 
2.22.0.rc2.383.gf4fbbf30c2-goog

Re: [PATCH 1/5] drm/panel: Add helper for reading DT rotation

[Sam Ravnborg]

Hi Derek.

On Mon, Jun 10, 2019 at 09:03:46PM -0700, Derek Basehore wrote:
> This adds a helper function for reading the rotation (panel
> orientation) from the device tree.
> 
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---
>  drivers/gpu/drm/drm_panel.c | 41 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_panel.h     |  7 +++++++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index dbd5b873e8f2..3b689ce4a51a 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -172,6 +172,47 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np)
>  	return ERR_PTR(-EPROBE_DEFER);
>  }
>  EXPORT_SYMBOL(of_drm_find_panel);
> +
> +/**
> + * of_drm_get_panel_orientation - look up the rotation of the panel using a
> + * device tree node
> + * @np: device tree node of the panel
> + * @orientation: orientation enum to be filled in
The comment says "enum" but the type used is an int.
Why not use enum drm_panel_orientation?

> + *
> + * Looks up the rotation of a panel in the device tree. The rotation in the
> + * device tree is counter clockwise.
> + *
> + * Return: 0 when a valid rotation value (0, 90, 180, or 270) is read or the
> + * rotation property doesn't exist. -EERROR otherwise.
> + */
Initially I read -EEROOR as a specific error code.
But I gues the semantic is to say that a negative error code is returned
if something was wrong.
As we do not use the "-EERROR" syntax anywhere else in drm, please
reword like we do in other places.


Also - it is worth to mention that the rotation returned is
DRM_MODE_PANEL_ORIENTATION_UNKNOWN if the property is not specified.
I wonder if this is correct, as no property could also been
interpretated as DRM_MODE_PANEL_ORIENTATION_NORMAL.
And in most cases the roation property is optional, so one could
assume that no property equals 0 degree.


	Sam

> +int of_drm_get_panel_orientation(const struct device_node *np, int *orientation)
> +{
> +	int rotation, ret;
> +
> +	ret = of_property_read_u32(np, "rotation", &rotation);
> +	if (ret == -EINVAL) {
> +		/* Don't return an error if there's no rotation property. */
> +		*orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> +		return 0;
> +	}
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	if (rotation == 0)
> +		*orientation = DRM_MODE_PANEL_ORIENTATION_NORMAL;
> +	else if (rotation == 90)
> +		*orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP;
> +	else if (rotation == 180)
> +		*orientation = DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP;
> +	else if (rotation == 270)
> +		*orientation = DRM_MODE_PANEL_ORIENTATION_LEFT_UP;
> +	else
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(of_drm_get_panel_orientation);
>  #endif
>  
>  MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 8c738c0e6e9f..13631b2efbaa 100644
> --- a/include/drm/drm_panel.h
> +++ b/include/drm/drm_panel.h
> @@ -197,11 +197,18 @@ int drm_panel_detach(struct drm_panel *panel);
>  
>  #if defined(CONFIG_OF) && defined(CONFIG_DRM_PANEL)
>  struct drm_panel *of_drm_find_panel(const struct device_node *np);
> +int of_drm_get_panel_orientation(const struct device_node *np,
> +				 int *orientation);
>  #else
>  static inline struct drm_panel *of_drm_find_panel(const struct device_node *np)
>  {
>  	return ERR_PTR(-ENODEV);
>  }
> +int of_drm_get_panel_orientation(const struct device_node *np,
> +				 int *orientation)
> +{
> +	return -ENODEV;
> +}
>  #endif
>  
>  #endif
> -- 
> 2.22.0.rc2.383.gf4fbbf30c2-goog

Re: [PATCH 1/5] drm/panel: Add helper for reading DT rotation

[dbasehore .]

On Wed, Jun 12, 2019 at 2:18 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Derek.
>
> On Mon, Jun 10, 2019 at 09:03:46PM -0700, Derek Basehore wrote:
> > This adds a helper function for reading the rotation (panel
> > orientation) from the device tree.
> >
> > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > ---
> >  drivers/gpu/drm/drm_panel.c | 41 +++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_panel.h     |  7 +++++++
> >  2 files changed, 48 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> > index dbd5b873e8f2..3b689ce4a51a 100644
> > --- a/drivers/gpu/drm/drm_panel.c
> > +++ b/drivers/gpu/drm/drm_panel.c
> > @@ -172,6 +172,47 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np)
> >       return ERR_PTR(-EPROBE_DEFER);
> >  }
> >  EXPORT_SYMBOL(of_drm_find_panel);
> > +
> > +/**
> > + * of_drm_get_panel_orientation - look up the rotation of the panel using a
> > + * device tree node
> > + * @np: device tree node of the panel
> > + * @orientation: orientation enum to be filled in
> The comment says "enum" but the type used is an int.
> Why not use enum drm_panel_orientation?
>

The binding is just an int value, but I can change it to the enum.

> > + *
> > + * Looks up the rotation of a panel in the device tree. The rotation in the
> > + * device tree is counter clockwise.
> > + *
> > + * Return: 0 when a valid rotation value (0, 90, 180, or 270) is read or the
> > + * rotation property doesn't exist. -EERROR otherwise.
> > + */
> Initially I read -EEROOR as a specific error code.
> But I gues the semantic is to say that a negative error code is returned
> if something was wrong.
> As we do not use the "-EERROR" syntax anywhere else in drm, please
> reword like we do in other places.
>
>
> Also - it is worth to mention that the rotation returned is
> DRM_MODE_PANEL_ORIENTATION_UNKNOWN if the property is not specified.
> I wonder if this is correct, as no property could also been
> interpretated as DRM_MODE_PANEL_ORIENTATION_NORMAL.
> And in most cases the roation property is optional, so one could
> assume that no property equals 0 degree.
>
>
>         Sam
>
> > +int of_drm_get_panel_orientation(const struct device_node *np, int *orientation)
> > +{
> > +     int rotation, ret;
> > +
> > +     ret = of_property_read_u32(np, "rotation", &rotation);
> > +     if (ret == -EINVAL) {
> > +             /* Don't return an error if there's no rotation property. */
> > +             *orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> > +             return 0;
> > +     }
> > +
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     if (rotation == 0)
> > +             *orientation = DRM_MODE_PANEL_ORIENTATION_NORMAL;
> > +     else if (rotation == 90)
> > +             *orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP;
> > +     else if (rotation == 180)
> > +             *orientation = DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP;
> > +     else if (rotation == 270)
> > +             *orientation = DRM_MODE_PANEL_ORIENTATION_LEFT_UP;
> > +     else
> > +             return -EINVAL;
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(of_drm_get_panel_orientation);
> >  #endif
> >
> >  MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");
> > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> > index 8c738c0e6e9f..13631b2efbaa 100644
> > --- a/include/drm/drm_panel.h
> > +++ b/include/drm/drm_panel.h
> > @@ -197,11 +197,18 @@ int drm_panel_detach(struct drm_panel *panel);
> >
> >  #if defined(CONFIG_OF) && defined(CONFIG_DRM_PANEL)
> >  struct drm_panel *of_drm_find_panel(const struct device_node *np);
> > +int of_drm_get_panel_orientation(const struct device_node *np,
> > +                              int *orientation);
> >  #else
> >  static inline struct drm_panel *of_drm_find_panel(const struct device_node *np)
> >  {
> >       return ERR_PTR(-ENODEV);
> >  }
> > +int of_drm_get_panel_orientation(const struct device_node *np,
> > +                              int *orientation)
> > +{
> > +     return -ENODEV;
> > +}
> >  #endif
> >
> >  #endif
> > --
> > 2.22.0.rc2.383.gf4fbbf30c2-goog

Re: [PATCH 1/5] drm/panel: Add helper for reading DT rotation

[dbasehore .]

On Fri, Jun 14, 2019 at 5:43 PM dbasehore . <dbasehore@chromium.org> wrote:
>
> On Wed, Jun 12, 2019 at 2:18 PM Sam Ravnborg <sam@ravnborg.org> wrote:
> >
> > Hi Derek.
> >
> > On Mon, Jun 10, 2019 at 09:03:46PM -0700, Derek Basehore wrote:
> > > This adds a helper function for reading the rotation (panel
> > > orientation) from the device tree.
> > >
> > > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > > ---
> > >  drivers/gpu/drm/drm_panel.c | 41 +++++++++++++++++++++++++++++++++++++
> > >  include/drm/drm_panel.h     |  7 +++++++
> > >  2 files changed, 48 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> > > index dbd5b873e8f2..3b689ce4a51a 100644
> > > --- a/drivers/gpu/drm/drm_panel.c
> > > +++ b/drivers/gpu/drm/drm_panel.c
> > > @@ -172,6 +172,47 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np)
> > >       return ERR_PTR(-EPROBE_DEFER);
> > >  }
> > >  EXPORT_SYMBOL(of_drm_find_panel);
> > > +
> > > +/**
> > > + * of_drm_get_panel_orientation - look up the rotation of the panel using a
> > > + * device tree node
> > > + * @np: device tree node of the panel
> > > + * @orientation: orientation enum to be filled in
> > The comment says "enum" but the type used is an int.
> > Why not use enum drm_panel_orientation?
> >
>
> The binding is just an int value, but I can change it to the enum.

Oops, I see what happened here. The way I wrote it should actually use the enum

>
> > > + *
> > > + * Looks up the rotation of a panel in the device tree. The rotation in the
> > > + * device tree is counter clockwise.
> > > + *
> > > + * Return: 0 when a valid rotation value (0, 90, 180, or 270) is read or the
> > > + * rotation property doesn't exist. -EERROR otherwise.
> > > + */
> > Initially I read -EEROOR as a specific error code.
> > But I gues the semantic is to say that a negative error code is returned
> > if something was wrong.
> > As we do not use the "-EERROR" syntax anywhere else in drm, please
> > reword like we do in other places.
> >
> >
> > Also - it is worth to mention that the rotation returned is
> > DRM_MODE_PANEL_ORIENTATION_UNKNOWN if the property is not specified.
> > I wonder if this is correct, as no property could also been
> > interpretated as DRM_MODE_PANEL_ORIENTATION_NORMAL.
> > And in most cases the roation property is optional, so one could
> > assume that no property equals 0 degree.
> >
> >
> >         Sam
> >
> > > +int of_drm_get_panel_orientation(const struct device_node *np, int *orientation)
> > > +{
> > > +     int rotation, ret;
> > > +
> > > +     ret = of_property_read_u32(np, "rotation", &rotation);
> > > +     if (ret == -EINVAL) {
> > > +             /* Don't return an error if there's no rotation property. */
> > > +             *orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> > > +             return 0;
> > > +     }
> > > +
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     if (rotation == 0)
> > > +             *orientation = DRM_MODE_PANEL_ORIENTATION_NORMAL;
> > > +     else if (rotation == 90)
> > > +             *orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP;
> > > +     else if (rotation == 180)
> > > +             *orientation = DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP;
> > > +     else if (rotation == 270)
> > > +             *orientation = DRM_MODE_PANEL_ORIENTATION_LEFT_UP;
> > > +     else
> > > +             return -EINVAL;
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL(of_drm_get_panel_orientation);
> > >  #endif
> > >
> > >  MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");
> > > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> > > index 8c738c0e6e9f..13631b2efbaa 100644
> > > --- a/include/drm/drm_panel.h
> > > +++ b/include/drm/drm_panel.h
> > > @@ -197,11 +197,18 @@ int drm_panel_detach(struct drm_panel *panel);
> > >
> > >  #if defined(CONFIG_OF) && defined(CONFIG_DRM_PANEL)
> > >  struct drm_panel *of_drm_find_panel(const struct device_node *np);
> > > +int of_drm_get_panel_orientation(const struct device_node *np,
> > > +                              int *orientation);
> > >  #else
> > >  static inline struct drm_panel *of_drm_find_panel(const struct device_node *np)
> > >  {
> > >       return ERR_PTR(-ENODEV);
> > >  }
> > > +int of_drm_get_panel_orientation(const struct device_node *np,
> > > +                              int *orientation)
> > > +{
> > > +     return -ENODEV;
> > > +}
> > >  #endif
> > >
> > >  #endif
> > > --
> > > 2.22.0.rc2.383.gf4fbbf30c2-goog

Re: [PATCH 1/5] drm/panel: Add helper for reading DT rotation

[Sam Ravnborg]

Hi Derek.

On Mon, Jun 10, 2019 at 09:03:46PM -0700, Derek Basehore wrote:
> This adds a helper function for reading the rotation (panel
> orientation) from the device tree.
> 
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---
>  drivers/gpu/drm/drm_panel.c | 41 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_panel.h     |  7 +++++++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index dbd5b873e8f2..3b689ce4a51a 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -172,6 +172,47 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np)
>  	return ERR_PTR(-EPROBE_DEFER);
>  }
>  EXPORT_SYMBOL(of_drm_find_panel);
> +
> +/**
> + * of_drm_get_panel_orientation - look up the rotation of the panel using a
> + * device tree node
> + * @np: device tree node of the panel
> + * @orientation: orientation enum to be filled in
> + *
> + * Looks up the rotation of a panel in the device tree. The rotation in the
> + * device tree is counter clockwise.
> + *
> + * Return: 0 when a valid rotation value (0, 90, 180, or 270) is read or the
> + * rotation property doesn't exist. -EERROR otherwise.
> + */
> +int of_drm_get_panel_orientation(const struct device_node *np, int *orientation)
> +{
> +	int rotation, ret;
> +
> +	ret = of_property_read_u32(np, "rotation", &rotation);

I just noticed that everywhere this code talks about orientation,
but the property that it reads are rotation.
To my best understanding these are not the same.

	Sam

Re: [PATCH 1/5] drm/panel: Add helper for reading DT rotation

[dbasehore .]

On Wed, Jun 12, 2019 at 2:20 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Derek.
>
> On Mon, Jun 10, 2019 at 09:03:46PM -0700, Derek Basehore wrote:
> > This adds a helper function for reading the rotation (panel
> > orientation) from the device tree.
> >
> > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > ---
> >  drivers/gpu/drm/drm_panel.c | 41 +++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_panel.h     |  7 +++++++
> >  2 files changed, 48 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> > index dbd5b873e8f2..3b689ce4a51a 100644
> > --- a/drivers/gpu/drm/drm_panel.c
> > +++ b/drivers/gpu/drm/drm_panel.c
> > @@ -172,6 +172,47 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np)
> >       return ERR_PTR(-EPROBE_DEFER);
> >  }
> >  EXPORT_SYMBOL(of_drm_find_panel);
> > +
> > +/**
> > + * of_drm_get_panel_orientation - look up the rotation of the panel using a
> > + * device tree node
> > + * @np: device tree node of the panel
> > + * @orientation: orientation enum to be filled in
> > + *
> > + * Looks up the rotation of a panel in the device tree. The rotation in the
> > + * device tree is counter clockwise.
> > + *
> > + * Return: 0 when a valid rotation value (0, 90, 180, or 270) is read or the
> > + * rotation property doesn't exist. -EERROR otherwise.
> > + */
> > +int of_drm_get_panel_orientation(const struct device_node *np, int *orientation)
> > +{
> > +     int rotation, ret;
> > +
> > +     ret = of_property_read_u32(np, "rotation", &rotation);
>
> I just noticed that everywhere this code talks about orientation,
> but the property that it reads are rotation.
> To my best understanding these are not the same.

This is because both were previously defined in the kernel. Rotation
was defined as a binding in the devicetree for panels (which is where
we wanted to put this property) and orientation already exists as a
DRM property.

If we want to change one, I would suggest the rotation binding since
it doesn't have any upstream users yet.

>
>         Sam

[PATCH 2/5] dt-bindings: display/panel: Expand rotation documentation

[Derek Basehore]

This adds to the rotation documentation to explain how drivers should
use the property and gives an example of the property in a devicetree
node.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 .../bindings/display/panel/panel.txt          | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/panel/panel.txt b/Documentation/devicetree/bindings/display/panel/panel.txt
index e2e6867852b8..f35d62d933fc 100644
--- a/Documentation/devicetree/bindings/display/panel/panel.txt
+++ b/Documentation/devicetree/bindings/display/panel/panel.txt
@@ -2,3 +2,35 @@ Common display properties
 -------------------------
 
 - rotation:	Display rotation in degrees counter clockwise (0,90,180,270)
+
+Property read from the device tree using of of_drm_get_panel_orientation
+
+The panel driver may apply the rotation at the TCON level, which will
+make the panel look like it isn't rotated to the kernel and any other
+software.
+
+If not, a panel orientation property should be added through the SoC
+vendor DRM code using the drm_connector_init_panel_orientation_property
+function.
+
+Example:
+	panel: panel@0 {
+		compatible = "boe,himax8279d8p";
+		reg = <0>;
+		enable-gpios = <&pio 45 0>;
+		pp33-gpios = <&pio 35 0>;
+		pp18-gpios = <&pio 36 0>;
+		pinctrl-names = "default", "state_3300mv", "state_1800mv";
+		pinctrl-0 = <&panel_pins_default>;
+		pinctrl-1 = <&panel_pins_3300mv>;
+		pinctrl-2 = <&panel_pins_1800mv>;
+		backlight = <&backlight_lcd0>;
+		rotation = <180>;
+		status = "okay";
+
+		port {
+			panel_in: endpoint {
+				remote-endpoint = <&dsi_out>;
+			};
+		};
+	};
-- 
2.22.0.rc2.383.gf4fbbf30c2-goog

Re: [PATCH 2/5] dt-bindings: display/panel: Expand rotation documentation

[Rob Herring]

On Mon, Jun 10, 2019 at 10:03 PM Derek Basehore <dbasehore@chromium.org> wrote:
>
> This adds to the rotation documentation to explain how drivers should
> use the property and gives an example of the property in a devicetree
> node.
>
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---
>  .../bindings/display/panel/panel.txt          | 32 +++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/panel/panel.txt b/Documentation/devicetree/bindings/display/panel/panel.txt
> index e2e6867852b8..f35d62d933fc 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel.txt
> +++ b/Documentation/devicetree/bindings/display/panel/panel.txt
> @@ -2,3 +2,35 @@ Common display properties
>  -------------------------
>
>  - rotation:    Display rotation in degrees counter clockwise (0,90,180,270)
> +
> +Property read from the device tree using of of_drm_get_panel_orientation

Don't put kernel specifics into bindings.

> +
> +The panel driver may apply the rotation at the TCON level, which will

What's TCON? Something Mediatek specific IIRC.

> +make the panel look like it isn't rotated to the kernel and any other
> +software.
> +
> +If not, a panel orientation property should be added through the SoC
> +vendor DRM code using the drm_connector_init_panel_orientation_property
> +function.

The 'rotation' property should be defined purely based on how the
panel is mounted relative to a device's orientation. If the display
pipeline has some ability to handle rotation, that's a feature of the
display pipeline and not the panel.

> +
> +Example:

This file is a collection of common properties. It shouldn't have an
example especially as this example is mostly non-common properties.

> +       panel: panel@0 {
> +               compatible = "boe,himax8279d8p";
> +               reg = <0>;
> +               enable-gpios = <&pio 45 0>;

> +               pp33-gpios = <&pio 35 0>;
> +               pp18-gpios = <&pio 36 0>;

BTW, are these upstream because they look like GPIO controlled
supplies which we model with gpio-regulator binding typically.

> +               pinctrl-names = "default", "state_3300mv", "state_1800mv";
> +               pinctrl-0 = <&panel_pins_default>;
> +               pinctrl-1 = <&panel_pins_3300mv>;
> +               pinctrl-2 = <&panel_pins_1800mv>;
> +               backlight = <&backlight_lcd0>;
> +               rotation = <180>;
> +               status = "okay";
> +
> +               port {
> +                       panel_in: endpoint {
> +                               remote-endpoint = <&dsi_out>;
> +                       };
> +               };
> +       };
> --
> 2.22.0.rc2.383.gf4fbbf30c2-goog
>

Re: [PATCH 2/5] dt-bindings: display/panel: Expand rotation documentation

[dbasehore .]

On Tue, Jun 11, 2019 at 8:25 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Mon, Jun 10, 2019 at 10:03 PM Derek Basehore <dbasehore@chromium.org> wrote:
> >
> > This adds to the rotation documentation to explain how drivers should
> > use the property and gives an example of the property in a devicetree
> > node.
> >
> > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > ---
> >  .../bindings/display/panel/panel.txt          | 32 +++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/panel/panel.txt b/Documentation/devicetree/bindings/display/panel/panel.txt
> > index e2e6867852b8..f35d62d933fc 100644
> > --- a/Documentation/devicetree/bindings/display/panel/panel.txt
> > +++ b/Documentation/devicetree/bindings/display/panel/panel.txt
> > @@ -2,3 +2,35 @@ Common display properties
> >  -------------------------
> >
> >  - rotation:    Display rotation in degrees counter clockwise (0,90,180,270)
> > +
> > +Property read from the device tree using of of_drm_get_panel_orientation
>
> Don't put kernel specifics into bindings.

Will remove that. I'll clean up the documentation to indicate that
this binding creates a panel orientation property unless the rotation
is handled in the Timing Controller on the panel if that sounds fine.

>
> > +
> > +The panel driver may apply the rotation at the TCON level, which will
>
> What's TCON? Something Mediatek specific IIRC.

The TCON is the Timing controller, which is on the panel. Every panel
has one. I'll add to the doc that the TCON is in the panel, etc.

>
> > +make the panel look like it isn't rotated to the kernel and any other
> > +software.
> > +
> > +If not, a panel orientation property should be added through the SoC
> > +vendor DRM code using the drm_connector_init_panel_orientation_property
> > +function.
>
> The 'rotation' property should be defined purely based on how the
> panel is mounted relative to a device's orientation. If the display
> pipeline has some ability to handle rotation, that's a feature of the
> display pipeline and not the panel.

This is how the panel orientation property is already handled in the
kernel. See drivers/gpu/drm/i915/vlv_dsi.c for more details.

>
> > +
> > +Example:
>
> This file is a collection of common properties. It shouldn't have an
> example especially as this example is mostly non-common properties.

Just copied one of our DTS entries that uses the property. I'll remove
everything under compatible except for rotation and status.

>
> > +       panel: panel@0 {
> > +               compatible = "boe,himax8279d8p";
> > +               reg = <0>;
> > +               enable-gpios = <&pio 45 0>;
>
> > +               pp33-gpios = <&pio 35 0>;
> > +               pp18-gpios = <&pio 36 0>;
>
> BTW, are these upstream because they look like GPIO controlled
> supplies which we model with gpio-regulator binding typically.

The boe,himax8279 driver was sent upstream, but it doesn't appear to
be merged. I'll look into it on that thread.

>
> > +               pinctrl-names = "default", "state_3300mv", "state_1800mv";
> > +               pinctrl-0 = <&panel_pins_default>;
> > +               pinctrl-1 = <&panel_pins_3300mv>;
> > +               pinctrl-2 = <&panel_pins_1800mv>;
> > +               backlight = <&backlight_lcd0>;
> > +               rotation = <180>;
> > +               status = "okay";
> > +
> > +               port {
> > +                       panel_in: endpoint {
> > +                               remote-endpoint = <&dsi_out>;
> > +                       };
> > +               };
> > +       };
> > --
> > 2.22.0.rc2.383.gf4fbbf30c2-goog
> >

Re: [PATCH 2/5] dt-bindings: display/panel: Expand rotation documentation

[Rob Herring]

On Tue, Jun 11, 2019 at 4:02 PM dbasehore . <dbasehore@chromium.org> wrote:
>
> On Tue, Jun 11, 2019 at 8:25 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Mon, Jun 10, 2019 at 10:03 PM Derek Basehore <dbasehore@chromium.org> wrote:
> > >
> > > This adds to the rotation documentation to explain how drivers should
> > > use the property and gives an example of the property in a devicetree
> > > node.
> > >
> > > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > > ---
> > >  .../bindings/display/panel/panel.txt          | 32 +++++++++++++++++++
> > >  1 file changed, 32 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/panel/panel.txt b/Documentation/devicetree/bindings/display/panel/panel.txt
> > > index e2e6867852b8..f35d62d933fc 100644
> > > --- a/Documentation/devicetree/bindings/display/panel/panel.txt
> > > +++ b/Documentation/devicetree/bindings/display/panel/panel.txt
> > > @@ -2,3 +2,35 @@ Common display properties
> > >  -------------------------
> > >
> > >  - rotation:    Display rotation in degrees counter clockwise (0,90,180,270)
> > > +
> > > +Property read from the device tree using of of_drm_get_panel_orientation
> >
> > Don't put kernel specifics into bindings.
>
> Will remove that. I'll clean up the documentation to indicate that
> this binding creates a panel orientation property unless the rotation
> is handled in the Timing Controller on the panel if that sounds fine.

Even if the timing ctrlr handles it, don't you still need to know what
the native orientation is?

> > > +
> > > +The panel driver may apply the rotation at the TCON level, which will
> >
> > What's TCON? Something Mediatek specific IIRC.
>
> The TCON is the Timing controller, which is on the panel. Every panel
> has one. I'll add to the doc that the TCON is in the panel, etc.
>
> >
> > > +make the panel look like it isn't rotated to the kernel and any other
> > > +software.
> > > +
> > > +If not, a panel orientation property should be added through the SoC
> > > +vendor DRM code using the drm_connector_init_panel_orientation_property
> > > +function.
> >
> > The 'rotation' property should be defined purely based on how the
> > panel is mounted relative to a device's orientation. If the display
> > pipeline has some ability to handle rotation, that's a feature of the
> > display pipeline and not the panel.
>
> This is how the panel orientation property is already handled in the
> kernel. See drivers/gpu/drm/i915/vlv_dsi.c for more details.

The point is your description is all about the kernel. This is a
binding which is not kernel specific.

> > > +
> > > +Example:
> >
> > This file is a collection of common properties. It shouldn't have an
> > example especially as this example is mostly non-common properties.
>
> Just copied one of our DTS entries that uses the property. I'll remove
> everything under compatible except for rotation and status.

Just remove the example or add what you want to the "boe,himax8279d8p"
binding doc. We are moving towards examples being compiled and
validated, so incomplete ones won't work.

Rob

Re: [PATCH 2/5] dt-bindings: display/panel: Expand rotation documentation

[dbasehore .]

On Thu, Jun 13, 2019 at 5:52 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Tue, Jun 11, 2019 at 4:02 PM dbasehore . <dbasehore@chromium.org> wrote:
> >
> > On Tue, Jun 11, 2019 at 8:25 AM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Mon, Jun 10, 2019 at 10:03 PM Derek Basehore <dbasehore@chromium.org> wrote:
> > > >
> > > > This adds to the rotation documentation to explain how drivers should
> > > > use the property and gives an example of the property in a devicetree
> > > > node.
> > > >
> > > > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > > > ---
> > > >  .../bindings/display/panel/panel.txt          | 32 +++++++++++++++++++
> > > >  1 file changed, 32 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/display/panel/panel.txt b/Documentation/devicetree/bindings/display/panel/panel.txt
> > > > index e2e6867852b8..f35d62d933fc 100644
> > > > --- a/Documentation/devicetree/bindings/display/panel/panel.txt
> > > > +++ b/Documentation/devicetree/bindings/display/panel/panel.txt
> > > > @@ -2,3 +2,35 @@ Common display properties
> > > >  -------------------------
> > > >
> > > >  - rotation:    Display rotation in degrees counter clockwise (0,90,180,270)
> > > > +
> > > > +Property read from the device tree using of of_drm_get_panel_orientation
> > >
> > > Don't put kernel specifics into bindings.
> >
> > Will remove that. I'll clean up the documentation to indicate that
> > this binding creates a panel orientation property unless the rotation
> > is handled in the Timing Controller on the panel if that sounds fine.
>
> Even if the timing ctrlr handles it, don't you still need to know what
> the native orientation is?

Not really. For all intents and purposes, the orientation of the panel
has changed.

>
> > > > +
> > > > +The panel driver may apply the rotation at the TCON level, which will
> > >
> > > What's TCON? Something Mediatek specific IIRC.
> >
> > The TCON is the Timing controller, which is on the panel. Every panel
> > has one. I'll add to the doc that the TCON is in the panel, etc.
> >
> > >
> > > > +make the panel look like it isn't rotated to the kernel and any other
> > > > +software.
> > > > +
> > > > +If not, a panel orientation property should be added through the SoC
> > > > +vendor DRM code using the drm_connector_init_panel_orientation_property
> > > > +function.
> > >
> > > The 'rotation' property should be defined purely based on how the
> > > panel is mounted relative to a device's orientation. If the display
> > > pipeline has some ability to handle rotation, that's a feature of the
> > > display pipeline and not the panel.
> >
> > This is how the panel orientation property is already handled in the
> > kernel. See drivers/gpu/drm/i915/vlv_dsi.c for more details.
>
> The point is your description is all about the kernel. This is a
> binding which is not kernel specific.

Ah, I see. I thought you were saying what the implementation should be.

>
> > > > +
> > > > +Example:
> > >
> > > This file is a collection of common properties. It shouldn't have an
> > > example especially as this example is mostly non-common properties.
> >
> > Just copied one of our DTS entries that uses the property. I'll remove
> > everything under compatible except for rotation and status.
>
> Just remove the example or add what you want to the "boe,himax8279d8p"
> binding doc. We are moving towards examples being compiled and
> validated, so incomplete ones won't work.

Ok, will do.

>
> Rob

Thanks for the quick reviews.

[PATCH 3/5] drm/panel: Add attach/detach callbacks

[Derek Basehore]

This adds the attach/detach callbacks. These are for setting up
internal state for the connector/panel pair that can't be done at
probe (since the connector doesn't exist) and which don't need to be
repeatedly done for every get/modes, prepare, or enable callback.
Values such as the panel orientation, and display size can be filled
in for the connector.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 drivers/gpu/drm/drm_panel.c | 14 ++++++++++++++
 include/drm/drm_panel.h     |  4 ++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 3b689ce4a51a..72f67678d9d5 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -104,12 +104,23 @@ EXPORT_SYMBOL(drm_panel_remove);
  */
 int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
 {
+	int ret;
+
 	if (panel->connector)
 		return -EBUSY;
 
 	panel->connector = connector;
 	panel->drm = connector->dev;
 
+	if (panel->funcs->attach) {
+		ret = panel->funcs->attach(panel);
+		if (ret < 0) {
+			panel->connector = NULL;
+			panel->drm = NULL;
+			return ret;
+		}
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_panel_attach);
@@ -128,6 +139,9 @@ EXPORT_SYMBOL(drm_panel_attach);
  */
 int drm_panel_detach(struct drm_panel *panel)
 {
+	if (panel->funcs->detach)
+		panel->funcs->detach(panel);
+
 	panel->connector = NULL;
 	panel->drm = NULL;
 
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 13631b2efbaa..e136e3a3c996 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -37,6 +37,8 @@ struct display_timing;
  * struct drm_panel_funcs - perform operations on a given panel
  * @disable: disable panel (turn off back light, etc.)
  * @unprepare: turn off panel
+ * @detach: detach panel->connector (clear internal state, etc.)
+ * @attach: attach panel->connector (update internal state, etc.)
  * @prepare: turn on panel and perform set up
  * @enable: enable panel (turn on back light, etc.)
  * @get_modes: add modes to the connector that the panel is attached to and
@@ -70,6 +72,8 @@ struct display_timing;
 struct drm_panel_funcs {
 	int (*disable)(struct drm_panel *panel);
 	int (*unprepare)(struct drm_panel *panel);
+	void (*detach)(struct drm_panel *panel);
+	int (*attach)(struct drm_panel *panel);
 	int (*prepare)(struct drm_panel *panel);
 	int (*enable)(struct drm_panel *panel);
 	int (*get_modes)(struct drm_panel *panel);
-- 
2.22.0.rc2.383.gf4fbbf30c2-goog

Re: [PATCH 3/5] drm/panel: Add attach/detach callbacks

[Daniel Vetter]

On Mon, Jun 10, 2019 at 09:03:48PM -0700, Derek Basehore wrote:
> This adds the attach/detach callbacks. These are for setting up
> internal state for the connector/panel pair that can't be done at
> probe (since the connector doesn't exist) and which don't need to be
> repeatedly done for every get/modes, prepare, or enable callback.
> Values such as the panel orientation, and display size can be filled
> in for the connector.
> 
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---
>  drivers/gpu/drm/drm_panel.c | 14 ++++++++++++++
>  include/drm/drm_panel.h     |  4 ++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index 3b689ce4a51a..72f67678d9d5 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -104,12 +104,23 @@ EXPORT_SYMBOL(drm_panel_remove);
>   */
>  int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
>  {
> +	int ret;
> +
>  	if (panel->connector)
>  		return -EBUSY;
>  
>  	panel->connector = connector;
>  	panel->drm = connector->dev;
>  
> +	if (panel->funcs->attach) {
> +		ret = panel->funcs->attach(panel);
> +		if (ret < 0) {
> +			panel->connector = NULL;
> +			panel->drm = NULL;
> +			return ret;
> +		}
> +	}

Why can't we just implement this in the drm helpers for everyone, by e.g.
storing a dt node in drm_panel? Feels a bit overkill to have these new
hooks here.

Also, my understanding is that this dt stuff is supposed to be
standardized, so this should work.
-Daniel

> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_panel_attach);
> @@ -128,6 +139,9 @@ EXPORT_SYMBOL(drm_panel_attach);
>   */
>  int drm_panel_detach(struct drm_panel *panel)
>  {
> +	if (panel->funcs->detach)
> +		panel->funcs->detach(panel);
> +
>  	panel->connector = NULL;
>  	panel->drm = NULL;
>  
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 13631b2efbaa..e136e3a3c996 100644
> --- a/include/drm/drm_panel.h
> +++ b/include/drm/drm_panel.h
> @@ -37,6 +37,8 @@ struct display_timing;
>   * struct drm_panel_funcs - perform operations on a given panel
>   * @disable: disable panel (turn off back light, etc.)
>   * @unprepare: turn off panel
> + * @detach: detach panel->connector (clear internal state, etc.)
> + * @attach: attach panel->connector (update internal state, etc.)
>   * @prepare: turn on panel and perform set up
>   * @enable: enable panel (turn on back light, etc.)
>   * @get_modes: add modes to the connector that the panel is attached to and
> @@ -70,6 +72,8 @@ struct display_timing;
>  struct drm_panel_funcs {
>  	int (*disable)(struct drm_panel *panel);
>  	int (*unprepare)(struct drm_panel *panel);
> +	void (*detach)(struct drm_panel *panel);
> +	int (*attach)(struct drm_panel *panel);
>  	int (*prepare)(struct drm_panel *panel);
>  	int (*enable)(struct drm_panel *panel);
>  	int (*get_modes)(struct drm_panel *panel);
> -- 
> 2.22.0.rc2.383.gf4fbbf30c2-goog
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 3/5] drm/panel: Add attach/detach callbacks

[dbasehore .]

On Tue, Jun 11, 2019 at 1:57 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Jun 10, 2019 at 09:03:48PM -0700, Derek Basehore wrote:
> > This adds the attach/detach callbacks. These are for setting up
> > internal state for the connector/panel pair that can't be done at
> > probe (since the connector doesn't exist) and which don't need to be
> > repeatedly done for every get/modes, prepare, or enable callback.
> > Values such as the panel orientation, and display size can be filled
> > in for the connector.
> >
> > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > ---
> >  drivers/gpu/drm/drm_panel.c | 14 ++++++++++++++
> >  include/drm/drm_panel.h     |  4 ++++
> >  2 files changed, 18 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> > index 3b689ce4a51a..72f67678d9d5 100644
> > --- a/drivers/gpu/drm/drm_panel.c
> > +++ b/drivers/gpu/drm/drm_panel.c
> > @@ -104,12 +104,23 @@ EXPORT_SYMBOL(drm_panel_remove);
> >   */
> >  int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
> >  {
> > +     int ret;
> > +
> >       if (panel->connector)
> >               return -EBUSY;
> >
> >       panel->connector = connector;
> >       panel->drm = connector->dev;
> >
> > +     if (panel->funcs->attach) {
> > +             ret = panel->funcs->attach(panel);
> > +             if (ret < 0) {
> > +                     panel->connector = NULL;
> > +                     panel->drm = NULL;
> > +                     return ret;
> > +             }
> > +     }
>
> Why can't we just implement this in the drm helpers for everyone, by e.g.
> storing a dt node in drm_panel? Feels a bit overkill to have these new
> hooks here.
>
> Also, my understanding is that this dt stuff is supposed to be
> standardized, so this should work.

So do you want all of this information added to the drm_panel struct?
If we do that, we don't necessarily even need the drm helper function.
We could just copy the values over here in the drm_panel_attach
function (and clear them in drm_panel_detach).

> -Daniel
>
> > +
> >       return 0;
> >  }
> >  EXPORT_SYMBOL(drm_panel_attach);
> > @@ -128,6 +139,9 @@ EXPORT_SYMBOL(drm_panel_attach);
> >   */
> >  int drm_panel_detach(struct drm_panel *panel)
> >  {
> > +     if (panel->funcs->detach)
> > +             panel->funcs->detach(panel);
> > +
> >       panel->connector = NULL;
> >       panel->drm = NULL;
> >
> > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> > index 13631b2efbaa..e136e3a3c996 100644
> > --- a/include/drm/drm_panel.h
> > +++ b/include/drm/drm_panel.h
> > @@ -37,6 +37,8 @@ struct display_timing;
> >   * struct drm_panel_funcs - perform operations on a given panel
> >   * @disable: disable panel (turn off back light, etc.)
> >   * @unprepare: turn off panel
> > + * @detach: detach panel->connector (clear internal state, etc.)
> > + * @attach: attach panel->connector (update internal state, etc.)
> >   * @prepare: turn on panel and perform set up
> >   * @enable: enable panel (turn on back light, etc.)
> >   * @get_modes: add modes to the connector that the panel is attached to and
> > @@ -70,6 +72,8 @@ struct display_timing;
> >  struct drm_panel_funcs {
> >       int (*disable)(struct drm_panel *panel);
> >       int (*unprepare)(struct drm_panel *panel);
> > +     void (*detach)(struct drm_panel *panel);
> > +     int (*attach)(struct drm_panel *panel);
> >       int (*prepare)(struct drm_panel *panel);
> >       int (*enable)(struct drm_panel *panel);
> >       int (*get_modes)(struct drm_panel *panel);
> > --
> > 2.22.0.rc2.383.gf4fbbf30c2-goog
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH 3/5] drm/panel: Add attach/detach callbacks

[dbasehore .]

If we want to query the device tree outside of the panel code in
helper functions, we can do this with the struct as is. There's
already a device struct pointer in drm_panel, so I think we can pull
from that.

On Tue, Jun 11, 2019 at 5:25 PM dbasehore . <dbasehore@chromium.org> wrote:
>
> On Tue, Jun 11, 2019 at 1:57 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Mon, Jun 10, 2019 at 09:03:48PM -0700, Derek Basehore wrote:
> > > This adds the attach/detach callbacks. These are for setting up
> > > internal state for the connector/panel pair that can't be done at
> > > probe (since the connector doesn't exist) and which don't need to be
> > > repeatedly done for every get/modes, prepare, or enable callback.
> > > Values such as the panel orientation, and display size can be filled
> > > in for the connector.
> > >
> > > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > > ---
> > >  drivers/gpu/drm/drm_panel.c | 14 ++++++++++++++
> > >  include/drm/drm_panel.h     |  4 ++++
> > >  2 files changed, 18 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> > > index 3b689ce4a51a..72f67678d9d5 100644
> > > --- a/drivers/gpu/drm/drm_panel.c
> > > +++ b/drivers/gpu/drm/drm_panel.c
> > > @@ -104,12 +104,23 @@ EXPORT_SYMBOL(drm_panel_remove);
> > >   */
> > >  int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
> > >  {
> > > +     int ret;
> > > +
> > >       if (panel->connector)
> > >               return -EBUSY;
> > >
> > >       panel->connector = connector;
> > >       panel->drm = connector->dev;
> > >
> > > +     if (panel->funcs->attach) {
> > > +             ret = panel->funcs->attach(panel);
> > > +             if (ret < 0) {
> > > +                     panel->connector = NULL;
> > > +                     panel->drm = NULL;
> > > +                     return ret;
> > > +             }
> > > +     }
> >
> > Why can't we just implement this in the drm helpers for everyone, by e.g.
> > storing a dt node in drm_panel? Feels a bit overkill to have these new
> > hooks here.
> >
> > Also, my understanding is that this dt stuff is supposed to be
> > standardized, so this should work.
>
> So do you want all of this information added to the drm_panel struct?
> If we do that, we don't necessarily even need the drm helper function.
> We could just copy the values over here in the drm_panel_attach
> function (and clear them in drm_panel_detach).
>
> > -Daniel
> >
> > > +
> > >       return 0;
> > >  }
> > >  EXPORT_SYMBOL(drm_panel_attach);
> > > @@ -128,6 +139,9 @@ EXPORT_SYMBOL(drm_panel_attach);
> > >   */
> > >  int drm_panel_detach(struct drm_panel *panel)
> > >  {
> > > +     if (panel->funcs->detach)
> > > +             panel->funcs->detach(panel);
> > > +
> > >       panel->connector = NULL;
> > >       panel->drm = NULL;
> > >
> > > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> > > index 13631b2efbaa..e136e3a3c996 100644
> > > --- a/include/drm/drm_panel.h
> > > +++ b/include/drm/drm_panel.h
> > > @@ -37,6 +37,8 @@ struct display_timing;
> > >   * struct drm_panel_funcs - perform operations on a given panel
> > >   * @disable: disable panel (turn off back light, etc.)
> > >   * @unprepare: turn off panel
> > > + * @detach: detach panel->connector (clear internal state, etc.)
> > > + * @attach: attach panel->connector (update internal state, etc.)
> > >   * @prepare: turn on panel and perform set up
> > >   * @enable: enable panel (turn on back light, etc.)
> > >   * @get_modes: add modes to the connector that the panel is attached to and
> > > @@ -70,6 +72,8 @@ struct display_timing;
> > >  struct drm_panel_funcs {
> > >       int (*disable)(struct drm_panel *panel);
> > >       int (*unprepare)(struct drm_panel *panel);
> > > +     void (*detach)(struct drm_panel *panel);
> > > +     int (*attach)(struct drm_panel *panel);
> > >       int (*prepare)(struct drm_panel *panel);
> > >       int (*enable)(struct drm_panel *panel);
> > >       int (*get_modes)(struct drm_panel *panel);
> > > --
> > > 2.22.0.rc2.383.gf4fbbf30c2-goog
> > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

Re: [PATCH 3/5] drm/panel: Add attach/detach callbacks

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2817 bytes --]

On Tue, Jun 11, 2019 at 05:25:47PM -0700, dbasehore . wrote:
> On Tue, Jun 11, 2019 at 1:57 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Mon, Jun 10, 2019 at 09:03:48PM -0700, Derek Basehore wrote:
> > > This adds the attach/detach callbacks. These are for setting up
> > > internal state for the connector/panel pair that can't be done at
> > > probe (since the connector doesn't exist) and which don't need to be
> > > repeatedly done for every get/modes, prepare, or enable callback.
> > > Values such as the panel orientation, and display size can be filled
> > > in for the connector.
> > >
> > > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > > ---
> > >  drivers/gpu/drm/drm_panel.c | 14 ++++++++++++++
> > >  include/drm/drm_panel.h     |  4 ++++
> > >  2 files changed, 18 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> > > index 3b689ce4a51a..72f67678d9d5 100644
> > > --- a/drivers/gpu/drm/drm_panel.c
> > > +++ b/drivers/gpu/drm/drm_panel.c
> > > @@ -104,12 +104,23 @@ EXPORT_SYMBOL(drm_panel_remove);
> > >   */
> > >  int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
> > >  {
> > > +     int ret;
> > > +
> > >       if (panel->connector)
> > >               return -EBUSY;
> > >
> > >       panel->connector = connector;
> > >       panel->drm = connector->dev;
> > >
> > > +     if (panel->funcs->attach) {
> > > +             ret = panel->funcs->attach(panel);
> > > +             if (ret < 0) {
> > > +                     panel->connector = NULL;
> > > +                     panel->drm = NULL;
> > > +                     return ret;
> > > +             }
> > > +     }
> >
> > Why can't we just implement this in the drm helpers for everyone, by e.g.
> > storing a dt node in drm_panel? Feels a bit overkill to have these new
> > hooks here.
> >
> > Also, my understanding is that this dt stuff is supposed to be
> > standardized, so this should work.
> 
> So do you want all of this information added to the drm_panel struct?
> If we do that, we don't necessarily even need the drm helper function.
> We could just copy the values over here in the drm_panel_attach
> function (and clear them in drm_panel_detach).

Yeah, I think we should have all this extra information in the struct
drm_panel. However, I think we need to more carefully split things such
that the DT parsing happens at panel probe time. That way we can catch
errors in DT, or missing entries/resources when we can still do
something about it.

If we start parsing DT and encounter failures, it's going to be very
confusing if that's at panel attach time where code will usually just
assume that everything is already validated and can't fail anymore.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/5] drm/panel: Add attach/detach callbacks

[dbasehore .]

On Fri, Jun 21, 2019 at 2:19 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Tue, Jun 11, 2019 at 05:25:47PM -0700, dbasehore . wrote:
> > On Tue, Jun 11, 2019 at 1:57 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Mon, Jun 10, 2019 at 09:03:48PM -0700, Derek Basehore wrote:
> > > > This adds the attach/detach callbacks. These are for setting up
> > > > internal state for the connector/panel pair that can't be done at
> > > > probe (since the connector doesn't exist) and which don't need to be
> > > > repeatedly done for every get/modes, prepare, or enable callback.
> > > > Values such as the panel orientation, and display size can be filled
> > > > in for the connector.
> > > >
> > > > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > > > ---
> > > >  drivers/gpu/drm/drm_panel.c | 14 ++++++++++++++
> > > >  include/drm/drm_panel.h     |  4 ++++
> > > >  2 files changed, 18 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> > > > index 3b689ce4a51a..72f67678d9d5 100644
> > > > --- a/drivers/gpu/drm/drm_panel.c
> > > > +++ b/drivers/gpu/drm/drm_panel.c
> > > > @@ -104,12 +104,23 @@ EXPORT_SYMBOL(drm_panel_remove);
> > > >   */
> > > >  int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
> > > >  {
> > > > +     int ret;
> > > > +
> > > >       if (panel->connector)
> > > >               return -EBUSY;
> > > >
> > > >       panel->connector = connector;
> > > >       panel->drm = connector->dev;
> > > >
> > > > +     if (panel->funcs->attach) {
> > > > +             ret = panel->funcs->attach(panel);
> > > > +             if (ret < 0) {
> > > > +                     panel->connector = NULL;
> > > > +                     panel->drm = NULL;
> > > > +                     return ret;
> > > > +             }
> > > > +     }
> > >
> > > Why can't we just implement this in the drm helpers for everyone, by e.g.
> > > storing a dt node in drm_panel? Feels a bit overkill to have these new
> > > hooks here.
> > >
> > > Also, my understanding is that this dt stuff is supposed to be
> > > standardized, so this should work.
> >
> > So do you want all of this information added to the drm_panel struct?
> > If we do that, we don't necessarily even need the drm helper function.
> > We could just copy the values over here in the drm_panel_attach
> > function (and clear them in drm_panel_detach).
>
> Yeah, I think we should have all this extra information in the struct
> drm_panel. However, I think we need to more carefully split things such
> that the DT parsing happens at panel probe time. That way we can catch
> errors in DT, or missing entries/resources when we can still do
> something about it.

For now, I'll just put width, height, bpc, orientation, bus_flags, and
bus_formats in the drm_panel struct. Those are pretty consistently set
from either get_modes or prepare. The other thing those all have in
common is that the values don't change.

We could just add an entire drm_display_info struct inside drm_panel,
but I don't know if we can just copy that over or set a pointer
without breaking something else, since some of the values in the
drm_display_info struct are not set by the panel (but maybe set by
something else).

>
> If we start parsing DT and encounter failures, it's going to be very
> confusing if that's at panel attach time where code will usually just
> assume that everything is already validated and can't fail anymore.
>
> Thierry

Thanks for the review

[PATCH 4/5] drm/connector: Split out orientation quirk detection

[Derek Basehore]

This removes the orientation quirk detection from the code to add
an orientation property to a panel. This is used only for legacy x86
systems, yet we'd like to start using this on devicetree systems where
quirk detection like this is not needed.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 drivers/gpu/drm/drm_connector.c | 16 ++++------------
 drivers/gpu/drm/i915/intel_dp.c | 14 +++++++++++---
 drivers/gpu/drm/i915/vlv_dsi.c  | 14 ++++++++++----
 include/drm/drm_connector.h     |  2 +-
 4 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index e17586aaa80f..58a09b65028b 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1894,31 +1894,23 @@ EXPORT_SYMBOL(drm_connector_set_vrr_capable_property);
  * drm_connector_init_panel_orientation_property -
  *	initialize the connecters panel_orientation property
  * @connector: connector for which to init the panel-orientation property.
- * @width: width in pixels of the panel, used for panel quirk detection
- * @height: height in pixels of the panel, used for panel quirk detection
  *
  * This function should only be called for built-in panels, after setting
  * connector->display_info.panel_orientation first (if known).
  *
- * This function will check for platform specific (e.g. DMI based) quirks
- * overriding display_info.panel_orientation first, then if panel_orientation
- * is not DRM_MODE_PANEL_ORIENTATION_UNKNOWN it will attach the
- * "panel orientation" property to the connector.
+ * This function will check if the panel_orientation is not
+ * DRM_MODE_PANEL_ORIENTATION_UNKNOWN. If not, it will attach the "panel
+ * orientation" property to the connector.
  *
  * Returns:
  * Zero on success, negative errno on failure.
  */
 int drm_connector_init_panel_orientation_property(
-	struct drm_connector *connector, int width, int height)
+	struct drm_connector *connector)
 {
 	struct drm_device *dev = connector->dev;
 	struct drm_display_info *info = &connector->display_info;
 	struct drm_property *prop;
-	int orientation_quirk;
-
-	orientation_quirk = drm_get_panel_orientation_quirk(width, height);
-	if (orientation_quirk != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
-		info->panel_orientation = orientation_quirk;
 
 	if (info->panel_orientation == DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
 		return 0;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b099a9dc28fd..72ab090ea97a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -40,6 +40,7 @@
 #include <drm/drm_edid.h>
 #include <drm/drm_hdcp.h>
 #include <drm/drm_probe_helper.h>
+#include <drm/drm_utils.h>
 #include <drm/i915_drm.h>
 
 #include "i915_debugfs.h"
@@ -7281,9 +7282,16 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	intel_connector->panel.backlight.power = intel_edp_backlight_power;
 	intel_panel_setup_backlight(connector, pipe);
 
-	if (fixed_mode)
-		drm_connector_init_panel_orientation_property(
-			connector, fixed_mode->hdisplay, fixed_mode->vdisplay);
+	if (fixed_mode) {
+		int orientation = drm_get_panel_orientation_quirk(
+				fixed_mode->hdisplay, fixed_mode->vdisplay);
+
+		if (orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
+			connector->display_info.panel_orientation =
+				orientation;
+
+		drm_connector_init_panel_orientation_property(connector);
+	}
 
 	return true;
 
diff --git a/drivers/gpu/drm/i915/vlv_dsi.c b/drivers/gpu/drm/i915/vlv_dsi.c
index bfe2891eac37..27f86a787f60 100644
--- a/drivers/gpu/drm/i915/vlv_dsi.c
+++ b/drivers/gpu/drm/i915/vlv_dsi.c
@@ -30,6 +30,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_mipi_dsi.h>
+#include <drm/drm_utils.h>
 
 #include "i915_drv.h"
 #include "intel_atomic.h"
@@ -1650,6 +1651,7 @@ static void intel_dsi_add_properties(struct intel_connector *connector)
 
 	if (connector->panel.fixed_mode) {
 		u32 allowed_scalers;
+		int orientation;
 
 		allowed_scalers = BIT(DRM_MODE_SCALE_ASPECT) | BIT(DRM_MODE_SCALE_FULLSCREEN);
 		if (!HAS_GMCH(dev_priv))
@@ -1660,12 +1662,16 @@ static void intel_dsi_add_properties(struct intel_connector *connector)
 
 		connector->base.state->scaling_mode = DRM_MODE_SCALE_ASPECT;
 
-		connector->base.display_info.panel_orientation =
-			vlv_dsi_get_panel_orientation(connector);
-		drm_connector_init_panel_orientation_property(
-				&connector->base,
+		orientation = drm_get_panel_orientation_quirk(
 				connector->panel.fixed_mode->hdisplay,
 				connector->panel.fixed_mode->vdisplay);
+		if (orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
+			connector->base.display_info.panel_orientation = orientation;
+		else
+			connector->base.display_info.panel_orientation =
+				vlv_dsi_get_panel_orientation(connector);
+
+		drm_connector_init_panel_orientation_property(&connector->base);
 	}
 }
 
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 47e749b74e5f..c2992f7a0dd5 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1370,7 +1370,7 @@ void drm_connector_set_link_status_property(struct drm_connector *connector,
 void drm_connector_set_vrr_capable_property(
 		struct drm_connector *connector, bool capable);
 int drm_connector_init_panel_orientation_property(
-	struct drm_connector *connector, int width, int height);
+	struct drm_connector *connector);
 int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
 					  int min, int max);
 
-- 
2.22.0.rc2.383.gf4fbbf30c2-goog

Re: [PATCH 4/5] drm/connector: Split out orientation quirk detection

[Jani Nikula]

On Mon, 10 Jun 2019, Derek Basehore <dbasehore@chromium.org> wrote:
> This removes the orientation quirk detection from the code to add
> an orientation property to a panel. This is used only for legacy x86
> systems, yet we'd like to start using this on devicetree systems where
> quirk detection like this is not needed.

Not needed, but no harm done either, right?

I guess I'll defer judgement on this to Hans and Ville (Cc'd).

Side note, I'm about to apply some (minor) conflicting changes in our
-next as soon as I get CI results on it.


BR,
Jani.


>
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---
>  drivers/gpu/drm/drm_connector.c | 16 ++++------------
>  drivers/gpu/drm/i915/intel_dp.c | 14 +++++++++++---
>  drivers/gpu/drm/i915/vlv_dsi.c  | 14 ++++++++++----
>  include/drm/drm_connector.h     |  2 +-
>  4 files changed, 26 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index e17586aaa80f..58a09b65028b 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1894,31 +1894,23 @@ EXPORT_SYMBOL(drm_connector_set_vrr_capable_property);
>   * drm_connector_init_panel_orientation_property -
>   *	initialize the connecters panel_orientation property
>   * @connector: connector for which to init the panel-orientation property.
> - * @width: width in pixels of the panel, used for panel quirk detection
> - * @height: height in pixels of the panel, used for panel quirk detection
>   *
>   * This function should only be called for built-in panels, after setting
>   * connector->display_info.panel_orientation first (if known).
>   *
> - * This function will check for platform specific (e.g. DMI based) quirks
> - * overriding display_info.panel_orientation first, then if panel_orientation
> - * is not DRM_MODE_PANEL_ORIENTATION_UNKNOWN it will attach the
> - * "panel orientation" property to the connector.
> + * This function will check if the panel_orientation is not
> + * DRM_MODE_PANEL_ORIENTATION_UNKNOWN. If not, it will attach the "panel
> + * orientation" property to the connector.
>   *
>   * Returns:
>   * Zero on success, negative errno on failure.
>   */
>  int drm_connector_init_panel_orientation_property(
> -	struct drm_connector *connector, int width, int height)
> +	struct drm_connector *connector)
>  {
>  	struct drm_device *dev = connector->dev;
>  	struct drm_display_info *info = &connector->display_info;
>  	struct drm_property *prop;
> -	int orientation_quirk;
> -
> -	orientation_quirk = drm_get_panel_orientation_quirk(width, height);
> -	if (orientation_quirk != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
> -		info->panel_orientation = orientation_quirk;
>  
>  	if (info->panel_orientation == DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
>  		return 0;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b099a9dc28fd..72ab090ea97a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -40,6 +40,7 @@
>  #include <drm/drm_edid.h>
>  #include <drm/drm_hdcp.h>
>  #include <drm/drm_probe_helper.h>
> +#include <drm/drm_utils.h>
>  #include <drm/i915_drm.h>
>  
>  #include "i915_debugfs.h"
> @@ -7281,9 +7282,16 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	intel_connector->panel.backlight.power = intel_edp_backlight_power;
>  	intel_panel_setup_backlight(connector, pipe);
>  
> -	if (fixed_mode)
> -		drm_connector_init_panel_orientation_property(
> -			connector, fixed_mode->hdisplay, fixed_mode->vdisplay);
> +	if (fixed_mode) {
> +		int orientation = drm_get_panel_orientation_quirk(
> +				fixed_mode->hdisplay, fixed_mode->vdisplay);
> +
> +		if (orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
> +			connector->display_info.panel_orientation =
> +				orientation;
> +
> +		drm_connector_init_panel_orientation_property(connector);
> +	}
>  
>  	return true;
>  
> diff --git a/drivers/gpu/drm/i915/vlv_dsi.c b/drivers/gpu/drm/i915/vlv_dsi.c
> index bfe2891eac37..27f86a787f60 100644
> --- a/drivers/gpu/drm/i915/vlv_dsi.c
> +++ b/drivers/gpu/drm/i915/vlv_dsi.c
> @@ -30,6 +30,7 @@
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_utils.h>
>  
>  #include "i915_drv.h"
>  #include "intel_atomic.h"
> @@ -1650,6 +1651,7 @@ static void intel_dsi_add_properties(struct intel_connector *connector)
>  
>  	if (connector->panel.fixed_mode) {
>  		u32 allowed_scalers;
> +		int orientation;
>  
>  		allowed_scalers = BIT(DRM_MODE_SCALE_ASPECT) | BIT(DRM_MODE_SCALE_FULLSCREEN);
>  		if (!HAS_GMCH(dev_priv))
> @@ -1660,12 +1662,16 @@ static void intel_dsi_add_properties(struct intel_connector *connector)
>  
>  		connector->base.state->scaling_mode = DRM_MODE_SCALE_ASPECT;
>  
> -		connector->base.display_info.panel_orientation =
> -			vlv_dsi_get_panel_orientation(connector);
> -		drm_connector_init_panel_orientation_property(
> -				&connector->base,
> +		orientation = drm_get_panel_orientation_quirk(
>  				connector->panel.fixed_mode->hdisplay,
>  				connector->panel.fixed_mode->vdisplay);
> +		if (orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
> +			connector->base.display_info.panel_orientation = orientation;
> +		else
> +			connector->base.display_info.panel_orientation =
> +				vlv_dsi_get_panel_orientation(connector);
> +
> +		drm_connector_init_panel_orientation_property(&connector->base);
>  	}
>  }
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 47e749b74e5f..c2992f7a0dd5 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1370,7 +1370,7 @@ void drm_connector_set_link_status_property(struct drm_connector *connector,
>  void drm_connector_set_vrr_capable_property(
>  		struct drm_connector *connector, bool capable);
>  int drm_connector_init_panel_orientation_property(
> -	struct drm_connector *connector, int width, int height);
> +	struct drm_connector *connector);
>  int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>  					  int min, int max);

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 4/5] drm/connector: Split out orientation quirk detection

[Hans de Goede]

Hi,

On 11-06-19 10:08, Jani Nikula wrote:
> On Mon, 10 Jun 2019, Derek Basehore <dbasehore@chromium.org> wrote:
>> This removes the orientation quirk detection from the code to add
>> an orientation property to a panel. This is used only for legacy x86
>> systems, yet we'd like to start using this on devicetree systems where
>> quirk detection like this is not needed.
> 
> Not needed, but no harm done either, right?
> 
> I guess I'll defer judgement on this to Hans and Ville (Cc'd).

Hmm, I'm not big fan of this change. It adds code duplication and as
other models with the same issue using a different driver or panel-type
show up we will get more code duplication.

Also I'm not convinced that devicetree based platforms will not need
this. The whole devicetree as an ABI thing, which means that all
devicetree bindings need to be set in stone before things are merged
into the mainline, is done solely so that we can get vendors to ship
hardware with the dtb files included in the firmware.

I'm 100% sure that there is e.g. ARM hardware out there which uses
non upright mounted LCD panels (I used to have a few Allwinner
tablets which did this). And given my experience with the quality
of firmware bundled tables like ACPI tables I'm quite sure that
if we ever move to firmware included dtb files that we will need
quirks for those too.

Also calling this "used only for legacy x86 systems" is a bit
unfair IMHO, new UEFI models are still being added to the quirk list
today, for hardware which does not even ship yet. Actually 99%
of the models in the quirk list are UEFI only models, which do
not even support classic PC BIOS booting, so those systems are
anything but legacy.

I believe the only reason we have only had to deal with this on x86
so far is because the OOTB support for most ARM systems is less polished
then that for x86 systems and on ARM systems there are still more
important issues to tackle first.

Regards,

Hans






> 
> Side note, I'm about to apply some (minor) conflicting changes in our
> -next as soon as I get CI results on it.
> 
> 
> BR,
> Jani.
> 
> 
>>
>> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
>> ---
>>   drivers/gpu/drm/drm_connector.c | 16 ++++------------
>>   drivers/gpu/drm/i915/intel_dp.c | 14 +++++++++++---
>>   drivers/gpu/drm/i915/vlv_dsi.c  | 14 ++++++++++----
>>   include/drm/drm_connector.h     |  2 +-
>>   4 files changed, 26 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index e17586aaa80f..58a09b65028b 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1894,31 +1894,23 @@ EXPORT_SYMBOL(drm_connector_set_vrr_capable_property);
>>    * drm_connector_init_panel_orientation_property -
>>    *	initialize the connecters panel_orientation property
>>    * @connector: connector for which to init the panel-orientation property.
>> - * @width: width in pixels of the panel, used for panel quirk detection
>> - * @height: height in pixels of the panel, used for panel quirk detection
>>    *
>>    * This function should only be called for built-in panels, after setting
>>    * connector->display_info.panel_orientation first (if known).
>>    *
>> - * This function will check for platform specific (e.g. DMI based) quirks
>> - * overriding display_info.panel_orientation first, then if panel_orientation
>> - * is not DRM_MODE_PANEL_ORIENTATION_UNKNOWN it will attach the
>> - * "panel orientation" property to the connector.
>> + * This function will check if the panel_orientation is not
>> + * DRM_MODE_PANEL_ORIENTATION_UNKNOWN. If not, it will attach the "panel
>> + * orientation" property to the connector.
>>    *
>>    * Returns:
>>    * Zero on success, negative errno on failure.
>>    */
>>   int drm_connector_init_panel_orientation_property(
>> -	struct drm_connector *connector, int width, int height)
>> +	struct drm_connector *connector)
>>   {
>>   	struct drm_device *dev = connector->dev;
>>   	struct drm_display_info *info = &connector->display_info;
>>   	struct drm_property *prop;
>> -	int orientation_quirk;
>> -
>> -	orientation_quirk = drm_get_panel_orientation_quirk(width, height);
>> -	if (orientation_quirk != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
>> -		info->panel_orientation = orientation_quirk;
>>   
>>   	if (info->panel_orientation == DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
>>   		return 0;
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index b099a9dc28fd..72ab090ea97a 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -40,6 +40,7 @@
>>   #include <drm/drm_edid.h>
>>   #include <drm/drm_hdcp.h>
>>   #include <drm/drm_probe_helper.h>
>> +#include <drm/drm_utils.h>
>>   #include <drm/i915_drm.h>
>>   
>>   #include "i915_debugfs.h"
>> @@ -7281,9 +7282,16 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>>   	intel_connector->panel.backlight.power = intel_edp_backlight_power;
>>   	intel_panel_setup_backlight(connector, pipe);
>>   
>> -	if (fixed_mode)
>> -		drm_connector_init_panel_orientation_property(
>> -			connector, fixed_mode->hdisplay, fixed_mode->vdisplay);
>> +	if (fixed_mode) {
>> +		int orientation = drm_get_panel_orientation_quirk(
>> +				fixed_mode->hdisplay, fixed_mode->vdisplay);
>> +
>> +		if (orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
>> +			connector->display_info.panel_orientation =
>> +				orientation;
>> +
>> +		drm_connector_init_panel_orientation_property(connector);
>> +	}
>>   
>>   	return true;
>>   
>> diff --git a/drivers/gpu/drm/i915/vlv_dsi.c b/drivers/gpu/drm/i915/vlv_dsi.c
>> index bfe2891eac37..27f86a787f60 100644
>> --- a/drivers/gpu/drm/i915/vlv_dsi.c
>> +++ b/drivers/gpu/drm/i915/vlv_dsi.c
>> @@ -30,6 +30,7 @@
>>   #include <drm/drm_crtc.h>
>>   #include <drm/drm_edid.h>
>>   #include <drm/drm_mipi_dsi.h>
>> +#include <drm/drm_utils.h>
>>   
>>   #include "i915_drv.h"
>>   #include "intel_atomic.h"
>> @@ -1650,6 +1651,7 @@ static void intel_dsi_add_properties(struct intel_connector *connector)
>>   
>>   	if (connector->panel.fixed_mode) {
>>   		u32 allowed_scalers;
>> +		int orientation;
>>   
>>   		allowed_scalers = BIT(DRM_MODE_SCALE_ASPECT) | BIT(DRM_MODE_SCALE_FULLSCREEN);
>>   		if (!HAS_GMCH(dev_priv))
>> @@ -1660,12 +1662,16 @@ static void intel_dsi_add_properties(struct intel_connector *connector)
>>   
>>   		connector->base.state->scaling_mode = DRM_MODE_SCALE_ASPECT;
>>   
>> -		connector->base.display_info.panel_orientation =
>> -			vlv_dsi_get_panel_orientation(connector);
>> -		drm_connector_init_panel_orientation_property(
>> -				&connector->base,
>> +		orientation = drm_get_panel_orientation_quirk(
>>   				connector->panel.fixed_mode->hdisplay,
>>   				connector->panel.fixed_mode->vdisplay);
>> +		if (orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
>> +			connector->base.display_info.panel_orientation = orientation;
>> +		else
>> +			connector->base.display_info.panel_orientation =
>> +				vlv_dsi_get_panel_orientation(connector);
>> +
>> +		drm_connector_init_panel_orientation_property(&connector->base);
>>   	}
>>   }
>>   
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 47e749b74e5f..c2992f7a0dd5 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -1370,7 +1370,7 @@ void drm_connector_set_link_status_property(struct drm_connector *connector,
>>   void drm_connector_set_vrr_capable_property(
>>   		struct drm_connector *connector, bool capable);
>>   int drm_connector_init_panel_orientation_property(
>> -	struct drm_connector *connector, int width, int height);
>> +	struct drm_connector *connector);
>>   int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>>   					  int min, int max);
> 

Re: [PATCH 4/5] drm/connector: Split out orientation quirk detection

[dbasehore .]

On Tue, Jun 11, 2019 at 1:54 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 11-06-19 10:08, Jani Nikula wrote:
> > On Mon, 10 Jun 2019, Derek Basehore <dbasehore@chromium.org> wrote:
> >> This removes the orientation quirk detection from the code to add
> >> an orientation property to a panel. This is used only for legacy x86
> >> systems, yet we'd like to start using this on devicetree systems where
> >> quirk detection like this is not needed.
> >
> > Not needed, but no harm done either, right?
> >
> > I guess I'll defer judgement on this to Hans and Ville (Cc'd).
>
> Hmm, I'm not big fan of this change. It adds code duplication and as
> other models with the same issue using a different driver or panel-type
> show up we will get more code duplication.
>
> Also I'm not convinced that devicetree based platforms will not need
> this. The whole devicetree as an ABI thing, which means that all
> devicetree bindings need to be set in stone before things are merged
> into the mainline, is done solely so that we can get vendors to ship
> hardware with the dtb files included in the firmware.

We've posted fixes to the devicetree well after the initial merge into
mainline before, so I don't see what you mean about the bindings being
set in stone. I also don't really see the point. The devicetree is in
the kernel. If there's some setting in the devicetree that we want to
change, it's effectively the same to make the change in the devicetree
versus some quirk setting. The only difference seems to be that making
the change in the devicetree is cleaner.

>
> I'm 100% sure that there is e.g. ARM hardware out there which uses
> non upright mounted LCD panels (I used to have a few Allwinner
> tablets which did this). And given my experience with the quality
> of firmware bundled tables like ACPI tables I'm quite sure that
> if we ever move to firmware included dtb files that we will need
> quirks for those too.

Is there a timeline to start using firmware bundled tables? Since the
quirk code only uses DMI, it will need to be changed anyways for
firmware bundled devicetree files anyways.

We could consolidate the duplicated code into another function that
calls drm_get_panel_orientation_quirks too. The only reason it's like
it is is because I initially only had the call to
drm_get_panel_orientation_quirk once in the code.


>
> Also calling this "used only for legacy x86 systems" is a bit
> unfair IMHO, new UEFI models are still being added to the quirk list
> today, for hardware which does not even ship yet. Actually 99%
> of the models in the quirk list are UEFI only models, which do
> not even support classic PC BIOS booting, so those systems are
> anything but legacy.
>
> I believe the only reason we have only had to deal with this on x86
> so far is because the OOTB support for most ARM systems is less polished
> then that for x86 systems and on ARM systems there are still more
> important issues to tackle first.

ARM just handles it in a different way. I'm not exactly sure how more
of the Android tablets do this, but it might just be handled entirely
in userspace with rotated splash screens on boot (so a device with a
180 degree panel has an upside down splash screen).

>
> Regards,
>
> Hans
>
>
>
>
>
>
> >
> > Side note, I'm about to apply some (minor) conflicting changes in our
> > -next as soon as I get CI results on it.
> >
> >
> > BR,
> > Jani.
> >
> >
> >>
> >> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> >> ---
> >>   drivers/gpu/drm/drm_connector.c | 16 ++++------------
> >>   drivers/gpu/drm/i915/intel_dp.c | 14 +++++++++++---
> >>   drivers/gpu/drm/i915/vlv_dsi.c  | 14 ++++++++++----
> >>   include/drm/drm_connector.h     |  2 +-
> >>   4 files changed, 26 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> >> index e17586aaa80f..58a09b65028b 100644
> >> --- a/drivers/gpu/drm/drm_connector.c
> >> +++ b/drivers/gpu/drm/drm_connector.c
> >> @@ -1894,31 +1894,23 @@ EXPORT_SYMBOL(drm_connector_set_vrr_capable_property);
> >>    * drm_connector_init_panel_orientation_property -
> >>    * initialize the connecters panel_orientation property
> >>    * @connector: connector for which to init the panel-orientation property.
> >> - * @width: width in pixels of the panel, used for panel quirk detection
> >> - * @height: height in pixels of the panel, used for panel quirk detection
> >>    *
> >>    * This function should only be called for built-in panels, after setting
> >>    * connector->display_info.panel_orientation first (if known).
> >>    *
> >> - * This function will check for platform specific (e.g. DMI based) quirks
> >> - * overriding display_info.panel_orientation first, then if panel_orientation
> >> - * is not DRM_MODE_PANEL_ORIENTATION_UNKNOWN it will attach the
> >> - * "panel orientation" property to the connector.
> >> + * This function will check if the panel_orientation is not
> >> + * DRM_MODE_PANEL_ORIENTATION_UNKNOWN. If not, it will attach the "panel
> >> + * orientation" property to the connector.
> >>    *
> >>    * Returns:
> >>    * Zero on success, negative errno on failure.
> >>    */
> >>   int drm_connector_init_panel_orientation_property(
> >> -    struct drm_connector *connector, int width, int height)
> >> +    struct drm_connector *connector)
> >>   {
> >>      struct drm_device *dev = connector->dev;
> >>      struct drm_display_info *info = &connector->display_info;
> >>      struct drm_property *prop;
> >> -    int orientation_quirk;
> >> -
> >> -    orientation_quirk = drm_get_panel_orientation_quirk(width, height);
> >> -    if (orientation_quirk != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
> >> -            info->panel_orientation = orientation_quirk;
> >>
> >>      if (info->panel_orientation == DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
> >>              return 0;
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index b099a9dc28fd..72ab090ea97a 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -40,6 +40,7 @@
> >>   #include <drm/drm_edid.h>
> >>   #include <drm/drm_hdcp.h>
> >>   #include <drm/drm_probe_helper.h>
> >> +#include <drm/drm_utils.h>
> >>   #include <drm/i915_drm.h>
> >>
> >>   #include "i915_debugfs.h"
> >> @@ -7281,9 +7282,16 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> >>      intel_connector->panel.backlight.power = intel_edp_backlight_power;
> >>      intel_panel_setup_backlight(connector, pipe);
> >>
> >> -    if (fixed_mode)
> >> -            drm_connector_init_panel_orientation_property(
> >> -                    connector, fixed_mode->hdisplay, fixed_mode->vdisplay);
> >> +    if (fixed_mode) {
> >> +            int orientation = drm_get_panel_orientation_quirk(
> >> +                            fixed_mode->hdisplay, fixed_mode->vdisplay);
> >> +
> >> +            if (orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
> >> +                    connector->display_info.panel_orientation =
> >> +                            orientation;
> >> +
> >> +            drm_connector_init_panel_orientation_property(connector);
> >> +    }
> >>
> >>      return true;
> >>
> >> diff --git a/drivers/gpu/drm/i915/vlv_dsi.c b/drivers/gpu/drm/i915/vlv_dsi.c
> >> index bfe2891eac37..27f86a787f60 100644
> >> --- a/drivers/gpu/drm/i915/vlv_dsi.c
> >> +++ b/drivers/gpu/drm/i915/vlv_dsi.c
> >> @@ -30,6 +30,7 @@
> >>   #include <drm/drm_crtc.h>
> >>   #include <drm/drm_edid.h>
> >>   #include <drm/drm_mipi_dsi.h>
> >> +#include <drm/drm_utils.h>
> >>
> >>   #include "i915_drv.h"
> >>   #include "intel_atomic.h"
> >> @@ -1650,6 +1651,7 @@ static void intel_dsi_add_properties(struct intel_connector *connector)
> >>
> >>      if (connector->panel.fixed_mode) {
> >>              u32 allowed_scalers;
> >> +            int orientation;
> >>
> >>              allowed_scalers = BIT(DRM_MODE_SCALE_ASPECT) | BIT(DRM_MODE_SCALE_FULLSCREEN);
> >>              if (!HAS_GMCH(dev_priv))
> >> @@ -1660,12 +1662,16 @@ static void intel_dsi_add_properties(struct intel_connector *connector)
> >>
> >>              connector->base.state->scaling_mode = DRM_MODE_SCALE_ASPECT;
> >>
> >> -            connector->base.display_info.panel_orientation =
> >> -                    vlv_dsi_get_panel_orientation(connector);
> >> -            drm_connector_init_panel_orientation_property(
> >> -                            &connector->base,
> >> +            orientation = drm_get_panel_orientation_quirk(
> >>                              connector->panel.fixed_mode->hdisplay,
> >>                              connector->panel.fixed_mode->vdisplay);
> >> +            if (orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
> >> +                    connector->base.display_info.panel_orientation = orientation;
> >> +            else
> >> +                    connector->base.display_info.panel_orientation =
> >> +                            vlv_dsi_get_panel_orientation(connector);
> >> +
> >> +            drm_connector_init_panel_orientation_property(&connector->base);
> >>      }
> >>   }
> >>
> >> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> >> index 47e749b74e5f..c2992f7a0dd5 100644
> >> --- a/include/drm/drm_connector.h
> >> +++ b/include/drm/drm_connector.h
> >> @@ -1370,7 +1370,7 @@ void drm_connector_set_link_status_property(struct drm_connector *connector,
> >>   void drm_connector_set_vrr_capable_property(
> >>              struct drm_connector *connector, bool capable);
> >>   int drm_connector_init_panel_orientation_property(
> >> -    struct drm_connector *connector, int width, int height);
> >> +    struct drm_connector *connector);
> >>   int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
> >>                                        int min, int max);
> >

Re: [PATCH 4/5] drm/connector: Split out orientation quirk detection

[Hans de Goede]

Hi,

On 12-06-19 02:16, dbasehore . wrote:
> On Tue, Jun 11, 2019 at 1:54 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 11-06-19 10:08, Jani Nikula wrote:
>>> On Mon, 10 Jun 2019, Derek Basehore <dbasehore@chromium.org> wrote:
>>>> This removes the orientation quirk detection from the code to add
>>>> an orientation property to a panel. This is used only for legacy x86
>>>> systems, yet we'd like to start using this on devicetree systems where
>>>> quirk detection like this is not needed.
>>>
>>> Not needed, but no harm done either, right?
>>>
>>> I guess I'll defer judgement on this to Hans and Ville (Cc'd).
>>
>> Hmm, I'm not big fan of this change. It adds code duplication and as
>> other models with the same issue using a different driver or panel-type
>> show up we will get more code duplication.
>>
>> Also I'm not convinced that devicetree based platforms will not need
>> this. The whole devicetree as an ABI thing, which means that all
>> devicetree bindings need to be set in stone before things are merged
>> into the mainline, is done solely so that we can get vendors to ship
>> hardware with the dtb files included in the firmware.
> 
> We've posted fixes to the devicetree well after the initial merge into
> mainline before, so I don't see what you mean about the bindings being
> set in stone.

That was just me repeating the official party line about devicetree.

> I also don't really see the point. The devicetree is in
> the kernel. If there's some setting in the devicetree that we want to
> change, it's effectively the same to make the change in the devicetree
> versus some quirk setting. The only difference seems to be that making
> the change in the devicetree is cleaner.

I agree with you that devicetree in practice is easy to update after
shipping. But at least whenever I tried to get new bindings reviewed
I was always told that I was not allowed to count on that.

>> I'm 100% sure that there is e.g. ARM hardware out there which uses
>> non upright mounted LCD panels (I used to have a few Allwinner
>> tablets which did this). And given my experience with the quality
>> of firmware bundled tables like ACPI tables I'm quite sure that
>> if we ever move to firmware included dtb files that we will need
>> quirks for those too.
> 
> Is there a timeline to start using firmware bundled tables?

Nope, as I said "if we ever move to ...".

> Since the
> quirk code only uses DMI, it will need to be changed anyways for
> firmware bundled devicetree files anyways.
> 
> We could consolidate the duplicated code into another function that
> calls drm_get_panel_orientation_quirks too. The only reason it's like
> it is is because I initially only had the call to
> drm_get_panel_orientation_quirk once in the code.

Yes if you can add a new helper for the current callers, then
I'm fine with dropping the quirk handling from
drm_connector_init_panel_orientation_property()

Regards,

Hans

Re: [PATCH 4/5] drm/connector: Split out orientation quirk detection

[dbasehore .]

On Wed, Jun 12, 2019 at 5:33 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 12-06-19 02:16, dbasehore . wrote:
> > On Tue, Jun 11, 2019 at 1:54 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> On 11-06-19 10:08, Jani Nikula wrote:
> >>> On Mon, 10 Jun 2019, Derek Basehore <dbasehore@chromium.org> wrote:
> >>>> This removes the orientation quirk detection from the code to add
> >>>> an orientation property to a panel. This is used only for legacy x86
> >>>> systems, yet we'd like to start using this on devicetree systems where
> >>>> quirk detection like this is not needed.
> >>>
> >>> Not needed, but no harm done either, right?
> >>>
> >>> I guess I'll defer judgement on this to Hans and Ville (Cc'd).
> >>
> >> Hmm, I'm not big fan of this change. It adds code duplication and as
> >> other models with the same issue using a different driver or panel-type
> >> show up we will get more code duplication.
> >>
> >> Also I'm not convinced that devicetree based platforms will not need
> >> this. The whole devicetree as an ABI thing, which means that all
> >> devicetree bindings need to be set in stone before things are merged
> >> into the mainline, is done solely so that we can get vendors to ship
> >> hardware with the dtb files included in the firmware.
> >
> > We've posted fixes to the devicetree well after the initial merge into
> > mainline before, so I don't see what you mean about the bindings being
> > set in stone.
>
> That was just me repeating the official party line about devicetree.
>
> > I also don't really see the point. The devicetree is in
> > the kernel. If there's some setting in the devicetree that we want to
> > change, it's effectively the same to make the change in the devicetree
> > versus some quirk setting. The only difference seems to be that making
> > the change in the devicetree is cleaner.
>
> I agree with you that devicetree in practice is easy to update after
> shipping. But at least whenever I tried to get new bindings reviewed
> I was always told that I was not allowed to count on that.
>
> >> I'm 100% sure that there is e.g. ARM hardware out there which uses
> >> non upright mounted LCD panels (I used to have a few Allwinner
> >> tablets which did this). And given my experience with the quality
> >> of firmware bundled tables like ACPI tables I'm quite sure that
> >> if we ever move to firmware included dtb files that we will need
> >> quirks for those too.
> >
> > Is there a timeline to start using firmware bundled tables?
>
> Nope, as I said "if we ever move to ...".
>
> > Since the
> > quirk code only uses DMI, it will need to be changed anyways for
> > firmware bundled devicetree files anyways.
> >
> > We could consolidate the duplicated code into another function that
> > calls drm_get_panel_orientation_quirks too. The only reason it's like
> > it is is because I initially only had the call to
> > drm_get_panel_orientation_quirk once in the code.
>
> Yes if you can add a new helper for the current callers, then
> I'm fine with dropping the quirk handling from
> drm_connector_init_panel_orientation_property()
>

Ok, it sounds like having a special callback for quirks in the panel
orientation property is the best way to go. The handles the duplicate
code and devicetree bundles. If we need to fix something that's
specified in a devicetree, and we aren't willing to change it, we can
write the quirk code for that later.

> Regards,
>
> Hans

Thanks for the feedback

[PATCH 5/5] drm/mtk: add panel orientation property

[Derek Basehore]

This inits the panel orientation property for the mediatek dsi driver
if the panel orientation (connector.display_info.panel_orientation) is
not DRM_MODE_PANEL_ORIENTATION_UNKNOWN.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 4a0b9150a7bb..08ffdc7526dd 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -782,10 +782,18 @@ static int mtk_dsi_create_connector(struct drm_device *drm, struct mtk_dsi *dsi)
 			DRM_ERROR("Failed to attach panel to drm\n");
 			goto err_connector_cleanup;
 		}
+
+		ret = drm_connector_init_panel_orientation_property(&dsi->conn);
+		if (ret) {
+			DRM_ERROR("Failed to init panel orientation\n");
+			goto err_panel_detach;
+		}
 	}
 
 	return 0;
 
+err_panel_detach:
+	drm_panel_detach(dsi->panel);
 err_connector_cleanup:
 	drm_connector_cleanup(&dsi->conn);
 	return ret;
-- 
2.22.0.rc2.383.gf4fbbf30c2-goog

[PATCH 0/5] Panel rotation patches

[Derek Basehore]

This adds the plumbing for reading panel rotation from the devicetree
and sets up adding a panel property for the panel orientation on
Mediatek SoCs when a rotation is present.

Derek Basehore (5):
  drm/panel: Add helper for reading DT rotation
  dt-bindings: display/panel: Expand rotation documentation
  drm/panel: Add attach/detach callbacks
  drm/connector: Split out orientation quirk detection
  drm/mtk: add panel orientation property

 .../bindings/display/panel/panel.txt          | 32 +++++++++++
 drivers/gpu/drm/drm_connector.c               | 16 ++----
 drivers/gpu/drm/drm_panel.c                   | 55 +++++++++++++++++++
 drivers/gpu/drm/i915/vlv_dsi.c                | 13 +++--
 drivers/gpu/drm/mediatek/mtk_dsi.c            |  8 +++
 include/drm/drm_connector.h                   |  2 +-
 include/drm/drm_panel.h                       | 11 ++++
 7 files changed, 120 insertions(+), 17 deletions(-)

-- 
2.22.0.rc2.383.gf4fbbf30c2-goog

[PATCH 4/5] drm/connector: Split out orientation quirk detection

[Derek Basehore]

This removes the orientation quirk detection from the code to add
an orientation property to a panel. This is used only for legacy x86
systems, yet we'd like to start using this on device tree systems
where quirk detection like this is not needed.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 drivers/gpu/drm/drm_connector.c | 16 ++++------------
 drivers/gpu/drm/i915/vlv_dsi.c  | 13 +++++++++----
 include/drm/drm_connector.h     |  2 +-
 3 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index e17586aaa80f..58a09b65028b 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1894,31 +1894,23 @@ EXPORT_SYMBOL(drm_connector_set_vrr_capable_property);
  * drm_connector_init_panel_orientation_property -
  *	initialize the connecters panel_orientation property
  * @connector: connector for which to init the panel-orientation property.
- * @width: width in pixels of the panel, used for panel quirk detection
- * @height: height in pixels of the panel, used for panel quirk detection
  *
  * This function should only be called for built-in panels, after setting
  * connector->display_info.panel_orientation first (if known).
  *
- * This function will check for platform specific (e.g. DMI based) quirks
- * overriding display_info.panel_orientation first, then if panel_orientation
- * is not DRM_MODE_PANEL_ORIENTATION_UNKNOWN it will attach the
- * "panel orientation" property to the connector.
+ * This function will check if the panel_orientation is not
+ * DRM_MODE_PANEL_ORIENTATION_UNKNOWN. If not, it will attach the "panel
+ * orientation" property to the connector.
  *
  * Returns:
  * Zero on success, negative errno on failure.
  */
 int drm_connector_init_panel_orientation_property(
-	struct drm_connector *connector, int width, int height)
+	struct drm_connector *connector)
 {
 	struct drm_device *dev = connector->dev;
 	struct drm_display_info *info = &connector->display_info;
 	struct drm_property *prop;
-	int orientation_quirk;
-
-	orientation_quirk = drm_get_panel_orientation_quirk(width, height);
-	if (orientation_quirk != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
-		info->panel_orientation = orientation_quirk;
 
 	if (info->panel_orientation == DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
 		return 0;
diff --git a/drivers/gpu/drm/i915/vlv_dsi.c b/drivers/gpu/drm/i915/vlv_dsi.c
index bfe2891eac37..113129996530 100644
--- a/drivers/gpu/drm/i915/vlv_dsi.c
+++ b/drivers/gpu/drm/i915/vlv_dsi.c
@@ -1650,6 +1650,7 @@ static void intel_dsi_add_properties(struct intel_connector *connector)
 
 	if (connector->panel.fixed_mode) {
 		u32 allowed_scalers;
+		int orientation;
 
 		allowed_scalers = BIT(DRM_MODE_SCALE_ASPECT) | BIT(DRM_MODE_SCALE_FULLSCREEN);
 		if (!HAS_GMCH(dev_priv))
@@ -1660,12 +1661,16 @@ static void intel_dsi_add_properties(struct intel_connector *connector)
 
 		connector->base.state->scaling_mode = DRM_MODE_SCALE_ASPECT;
 
-		connector->base.display_info.panel_orientation =
-			vlv_dsi_get_panel_orientation(connector);
-		drm_connector_init_panel_orientation_property(
-				&connector->base,
+		orientation = drm_get_panel_orientation_quirk(
 				connector->panel.fixed_mode->hdisplay,
 				connector->panel.fixed_mode->vdisplay);
+		if (orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
+			connector->display_info.panel_orientation = orientation;
+		else
+			connector->display_info.panel_orientation =
+				intel_dsi_get_panel_orientation(connector);
+
+		drm_connector_init_panel_orientation_property(&connector->base);
 	}
 }
 
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 47e749b74e5f..c2992f7a0dd5 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1370,7 +1370,7 @@ void drm_connector_set_link_status_property(struct drm_connector *connector,
 void drm_connector_set_vrr_capable_property(
 		struct drm_connector *connector, bool capable);
 int drm_connector_init_panel_orientation_property(
-	struct drm_connector *connector, int width, int height);
+	struct drm_connector *connector);
 int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
 					  int min, int max);
 
-- 
2.22.0.rc2.383.gf4fbbf30c2-goog