Re: [PATCH v10 0/2] Panel rotation patches
On Tue, Apr 14, 2020 at 2:18 PM Dmitry Osipenko wrote: > > 14.04.2020 22:32, dbasehore . пишет: > > Hi Dmitry, sorry for the late reply. > > > > On Sun, Mar 8, 2020 at 12:25 PM Dmitry Osipenko wrote: > >> > >> 06.03.2020 03:21, 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. > >> > >> Hello Derek and everyone, > >> > >> I'm looking at adding display rotation support to NVIDIA Tegra DRM > >> driver because some devices have display panel physically mounted > >> upside-down, and thus, display controller's scan-out needs to be rotated > >> by 180° in this case. > >> > >> Derek, yours panel-rotation patches add support for assigning panel's > >> orientation to the connector, but then only primary display plane > >> receives rotation value in [1], while rotation needs to be applied to > >> all available overlay/cursor planes and this should happen in other > >> places than [1] as well. > > > > This is intended. We don't correct the output in the kernel. We > > instead rely on notifying userspace that the panel is rotated, then we > > handle it there. > > > >> > >> [1] drm_client_modeset_commit_atomic() > >> > >> Please also note that in a case of the scan-out rotation, plane's > >> coordinates need to be changed in accordance to the display's rotation. > >> > >> I looked briefly through the DRM code and my understanding that the DRM > >> core currently doesn't support use-case where scan-out needs to rotated > >> based on a panel's orientation, correct? Is it the use-case you're > >> working on for the Mediatek driver? > > > > Yes, we rely on userspace to rotate the output. The major reason for > > this is because there may not be a "free" hardware rotation that can > > be applied to the overlay. Sean Paul and others also preferred that > > userspace control what is output to the screen instead of the kernel > > taking care of it. This code just adds the drm property to the panel. > > > > Could you please explain what that userspace is? This was added for Chrome OS, which uses its own graphics stack, Ozone, instead of Xorg. > > AFAIK, things like Xorg modesetting don't support that orientation property. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v10 0/2] Panel rotation patches
Hi Dmitry, sorry for the late reply. On Sun, Mar 8, 2020 at 12:25 PM Dmitry Osipenko wrote: > > 06.03.2020 03:21, 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. > > Hello Derek and everyone, > > I'm looking at adding display rotation support to NVIDIA Tegra DRM > driver because some devices have display panel physically mounted > upside-down, and thus, display controller's scan-out needs to be rotated > by 180° in this case. > > Derek, yours panel-rotation patches add support for assigning panel's > orientation to the connector, but then only primary display plane > receives rotation value in [1], while rotation needs to be applied to > all available overlay/cursor planes and this should happen in other > places than [1] as well. This is intended. We don't correct the output in the kernel. We instead rely on notifying userspace that the panel is rotated, then we handle it there. > > [1] drm_client_modeset_commit_atomic() > > Please also note that in a case of the scan-out rotation, plane's > coordinates need to be changed in accordance to the display's rotation. > > I looked briefly through the DRM code and my understanding that the DRM > core currently doesn't support use-case where scan-out needs to rotated > based on a panel's orientation, correct? Is it the use-case you're > working on for the Mediatek driver? Yes, we rely on userspace to rotate the output. The major reason for this is because there may not be a "free" hardware rotation that can be applied to the overlay. Sean Paul and others also preferred that userspace control what is output to the screen instead of the kernel taking care of it. This code just adds the drm property to the panel. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v8 1/4] drm/panel: Add helper for reading DT rotation
On Mon, Oct 7, 2019 at 9:38 AM Sean Paul wrote:
>
> On Wed, Sep 25, 2019 at 03:58:30PM -0700, Derek Basehore wrote:
> > This adds a helper function for reading the rotation (panel
> > orientation) from the device tree.
> >
> > Signed-off-by: Derek Basehore
> > Reviewed-by: Sam Ravnborg
>
> The patch LGTM, but I don't see it used anywhere later in the patch? Is there
> a
> panel driver incoming?
Yeah, the boe-tv101wum-nl6 panel will use it. It's not in the patch
currently sent upstream since I don't want to step on their toes, but
I plan on sending a patch to add it as soon as that is merged.
I could hold back on this patch until that panel driver is merged too.
Another alternative is to throw this into the generic drm_panel code,
but there's no obvious place to put it since DRM seems to leave
reading the DTS up to the panel drivers themselves.
>
> Sean
>
> > ---
> > drivers/gpu/drm/drm_panel.c | 43 +
> > include/drm/drm_panel.h | 9
> > 2 files changed, 52 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> > index 6b0bf42039cf..0909b53b74e6 100644
> > --- a/drivers/gpu/drm/drm_panel.c
> > +++ b/drivers/gpu/drm/drm_panel.c
> > @@ -264,6 +264,49 @@ 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 orientation of the panel
> > through
> > + * the "rotation" binding from 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 orientation of
> > the
> > + * panel is expressed as a property name "rotation" 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,
> > + enum drm_panel_orientation *orientation)
> > +{
> > + int rotation, ret;
> > +
> > + ret = of_property_read_u32(np, "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 ");
> > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> > index 624bd15ecfab..d16158deacdc 100644
> > --- a/include/drm/drm_panel.h
> > +++ b/include/drm/drm_panel.h
> > @@ -34,6 +34,8 @@ struct drm_device;
> > struct drm_panel;
> > struct display_timing;
> >
> > +enum drm_panel_orientation;
> > +
> > /**
> > * struct drm_panel_funcs - perform operations on a given panel
> > *
> > @@ -165,11 +167,18 @@ int drm_panel_get_modes(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,
> > + enum drm_panel_orientation *orientation);
> > #else
> > static inline struct drm_panel *of_drm_find_panel(const struct device_node
> > *np)
> > {
> > return ERR_PTR(-ENODEV);
> > }
> > +static inline int of_drm_get_panel_orientation(const struct device_node
> > *np,
> > + enum drm_panel_orientation *orientation)
> > +{
> > + return -ENODEV;
> > +}
> > #endif
> >
> > #endif
> > --
> > 2.23.0.351.gc4317032e6-goog
> >
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS
Re: [v8,2/4] drm/panel: set display info in panel attach
On Mon, Oct 7, 2019 at 9:44 AM Sean Paul wrote:
>
> On Mon, Sep 30, 2019 at 04:14:54PM -0700, dbasehore . wrote:
> > On Sat, Sep 28, 2019 at 10:23 PM james qian wang (Arm Technology
> > China) wrote:
> > >
> > > On Wed, Sep 25, 2019 at 03:58:31PM -0700, Derek Basehore wrote:
> > > > Devicetree systems can set panel orientation via a panel binding, but
> > > > there's no way, as is, to propagate this setting to the connector,
> > > > where the property need to be added.
> > > > To address this, this patch sets orientation, as well as other fixed
> > > > values for the panel, in the drm_panel_attach function. These values
> > > > are stored from probe in the drm_panel struct.
> > > >
> > > > Signed-off-by: Derek Basehore
> > > > Reviewed-by: Sam Ravnborg
> > > > ---
> > > > drivers/gpu/drm/drm_panel.c | 28 +
> > > > include/drm/drm_panel.h | 50 +
> > > > 2 files changed, 78 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> > > > index 0909b53b74e6..1cd2b56c9fe6 100644
> > > > --- a/drivers/gpu/drm/drm_panel.c
> > > > +++ b/drivers/gpu/drm/drm_panel.c
> > > > @@ -104,11 +104,23 @@ EXPORT_SYMBOL(drm_panel_remove);
> > > > */
> > > > int drm_panel_attach(struct drm_panel *panel, struct drm_connector
> > > > *connector)
> > > > {
> > > > + struct drm_display_info *info;
> > > > +
> > > > if (panel->connector)
> > > > return -EBUSY;
> > > >
> > > > panel->connector = connector;
> > > > panel->drm = connector->dev;
> > > > + info = >display_info;
> > > > + info->width_mm = panel->width_mm;
> > > > + info->height_mm = panel->height_mm;
> > > > + info->bpc = panel->bpc;
> > > > + info->panel_orientation = panel->orientation;
> > > > + info->bus_flags = panel->bus_flags;
> > > > + if (panel->bus_formats)
> > > > + drm_display_info_set_bus_formats(>display_info,
> > > > + panel->bus_formats,
> > > > + panel->num_bus_formats);
> > > >
> > > > return 0;
> > > > }
> > > > @@ -126,6 +138,22 @@ EXPORT_SYMBOL(drm_panel_attach);
> > > > */
> > > > void drm_panel_detach(struct drm_panel *panel)
> > > > {
> > > > + struct drm_display_info *info;
> > > > +
> > > > + if (!panel->connector)
> > > > + goto out;
> > > > +
> > > > + info = >connector->display_info;
> > > > + info->width_mm = 0;
> > > > + info->height_mm = 0;
> > > > + info->bpc = 0;
> > > > + info->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> > > > + info->bus_flags = 0;
> > > > + kfree(info->bus_formats);
> > > > + info->bus_formats = NULL;
> > > > + info->num_bus_formats = 0;
> > > > +
> > > > +out:
> > > > panel->connector = NULL;
> > > > panel->drm = NULL;
> > > > }
> > > > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> > > > index d16158deacdc..f3587a54b8ac 100644
> > > > --- a/include/drm/drm_panel.h
> > > > +++ b/include/drm/drm_panel.h
> > > > @@ -141,6 +141,56 @@ struct drm_panel {
> > > >*/
> > > > const struct drm_panel_funcs *funcs;
> > > >
> > >
> > > All these new added members seems dupliated with drm_display_info,
> > > So I think, can we add a new drm_plane_funcs func:
> > >
> > > int (*set_display_info)(struct drm_panel *panel,
> > > struct drm_display_info *info);
> >
> > I don't disagree personally, since I originally wrote it this way, but
> > 2 maintainers, Daniel Vetter and Thierry Reding, requested that it be
> > changed. Unless that decision is reversed, I won't be changing this.
> >
>
> Reading back the feedback on v1, I don't think anyone said they were against
> storing a dr
Re: [v8,2/4] drm/panel: set display info in panel attach
On Sat, Sep 28, 2019 at 10:23 PM james qian wang (Arm Technology
China) wrote:
>
> On Wed, Sep 25, 2019 at 03:58:31PM -0700, Derek Basehore wrote:
> > Devicetree systems can set panel orientation via a panel binding, but
> > there's no way, as is, to propagate this setting to the connector,
> > where the property need to be added.
> > To address this, this patch sets orientation, as well as other fixed
> > values for the panel, in the drm_panel_attach function. These values
> > are stored from probe in the drm_panel struct.
> >
> > Signed-off-by: Derek Basehore
> > Reviewed-by: Sam Ravnborg
> > ---
> > drivers/gpu/drm/drm_panel.c | 28 +
> > include/drm/drm_panel.h | 50 +
> > 2 files changed, 78 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> > index 0909b53b74e6..1cd2b56c9fe6 100644
> > --- a/drivers/gpu/drm/drm_panel.c
> > +++ b/drivers/gpu/drm/drm_panel.c
> > @@ -104,11 +104,23 @@ EXPORT_SYMBOL(drm_panel_remove);
> > */
> > int drm_panel_attach(struct drm_panel *panel, struct drm_connector
> > *connector)
> > {
> > + struct drm_display_info *info;
> > +
> > if (panel->connector)
> > return -EBUSY;
> >
> > panel->connector = connector;
> > panel->drm = connector->dev;
> > + info = >display_info;
> > + info->width_mm = panel->width_mm;
> > + info->height_mm = panel->height_mm;
> > + info->bpc = panel->bpc;
> > + info->panel_orientation = panel->orientation;
> > + info->bus_flags = panel->bus_flags;
> > + if (panel->bus_formats)
> > + drm_display_info_set_bus_formats(>display_info,
> > + panel->bus_formats,
> > + panel->num_bus_formats);
> >
> > return 0;
> > }
> > @@ -126,6 +138,22 @@ EXPORT_SYMBOL(drm_panel_attach);
> > */
> > void drm_panel_detach(struct drm_panel *panel)
> > {
> > + struct drm_display_info *info;
> > +
> > + if (!panel->connector)
> > + goto out;
> > +
> > + info = >connector->display_info;
> > + info->width_mm = 0;
> > + info->height_mm = 0;
> > + info->bpc = 0;
> > + info->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> > + info->bus_flags = 0;
> > + kfree(info->bus_formats);
> > + info->bus_formats = NULL;
> > + info->num_bus_formats = 0;
> > +
> > +out:
> > panel->connector = NULL;
> > panel->drm = NULL;
> > }
> > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> > index d16158deacdc..f3587a54b8ac 100644
> > --- a/include/drm/drm_panel.h
> > +++ b/include/drm/drm_panel.h
> > @@ -141,6 +141,56 @@ struct drm_panel {
> >*/
> > const struct drm_panel_funcs *funcs;
> >
>
> All these new added members seems dupliated with drm_display_info,
> So I think, can we add a new drm_plane_funcs func:
>
> int (*set_display_info)(struct drm_panel *panel,
> struct drm_display_info *info);
I don't disagree personally, since I originally wrote it this way, but
2 maintainers, Daniel Vetter and Thierry Reding, requested that it be
changed. Unless that decision is reversed, I won't be changing this.
>
> Then in drm_panel_attach(), via this interface the specific panel
> driver can directly set connector->display_info. like
>
>...
>if (panel->funcs && panel->funcs->set_display_info)
> panel->funcs->unprepare(panel, connector->display_info);
>...
>
> Thanks
> James
>
> > + /**
> > + * @width_mm:
> > + *
> > + * Physical width in mm.
> > + */
> > + unsigned int width_mm;
> > +
> > + /**
> > + * @height_mm:
> > + *
> > + * Physical height in mm.
> > + */
> > + unsigned int height_mm;
> > +
> > + /**
> > + * @bpc:
> > + *
> > + * Maximum bits per color channel. Used by HDMI and DP outputs.
> > + */
> > + unsigned int bpc;
> > +
> > + /**
> > + * @orientation
> > + *
> > + * Installation orientation of the panel with respect to the chassis.
> > + */
> > + int orientation;
> > +
> > + /**
> > + * @bus_formats
> > + *
> > + * Pixel data format on the wire.
> > + */
> > + const u32 *bus_formats;
> > +
> > + /**
> > + * @num_bus_formats:
> > + *
> > + * Number of elements pointed to by @bus_formats
> > + */
> > + unsigned int num_bus_formats;
> > +
> > + /**
> > + * @bus_flags:
> > + *
> > + * Additional information (like pixel signal polarity) for the pixel
> > + * data on the bus.
> > + */
> > + u32 bus_flags;
> > +
> > /**
> >* @list:
> >*
Thanks for the review
Re: [PATCH v7 2/4] drm/panel: set display info in panel attach
Hi Sam, thanks for pointing out the potential conflict.
On Tue, Jul 23, 2019 at 2:19 AM Sam Ravnborg wrote:
>
> Hi Derek.
>
> On Tue, Jul 09, 2019 at 07:16:57PM -0700, Derek Basehore wrote:
> > Devicetree systems can set panel orientation via a panel binding, but
> > there's no way, as is, to propagate this setting to the connector,
> > where the property need to be added.
> > To address this, this patch sets orientation, as well as other fixed
> > values for the panel, in the drm_panel_attach function. These values
> > are stored from probe in the drm_panel struct.
>
> This approch seems to conflict with work done by Laurent where the
> ownership/creation of the connector will be moved to the display controller.
>
> If I understand it correct then there should not be a 1:1 relation
> between a panel and a connector anymore.
Can you point me to this work? I still see the lone drm_display_info
struct in the drm_connector struct. This seems to indicate that the
kernel still assume one display per connector.
>
> We should not try to work in two different directions with this.
> Laurent, can you comment on this?
>
> If we move forard with this patch, then all fields in drm_panel needs
> kernel-doc - preferably inline.
>
> Sam
>
> >
> > Signed-off-by: Derek Basehore
> > ---
> > drivers/gpu/drm/drm_panel.c | 28
> > include/drm/drm_panel.h | 14 ++
> > 2 files changed, 42 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> > index 169bab54d52d..ca01095470a9 100644
> > --- a/drivers/gpu/drm/drm_panel.c
> > +++ b/drivers/gpu/drm/drm_panel.c
> > @@ -104,11 +104,23 @@ EXPORT_SYMBOL(drm_panel_remove);
> > */
> > int drm_panel_attach(struct drm_panel *panel, struct drm_connector
> > *connector)
> > {
> > + struct drm_display_info *info;
> > +
> > if (panel->connector)
> > return -EBUSY;
> >
> > panel->connector = connector;
> > panel->drm = connector->dev;
> > + info = >display_info;
> > + info->width_mm = panel->width_mm;
> > + info->height_mm = panel->height_mm;
> > + info->bpc = panel->bpc;
> > + info->panel_orientation = panel->orientation;
> > + info->bus_flags = panel->bus_flags;
> > + if (panel->bus_formats)
> > + drm_display_info_set_bus_formats(>display_info,
> > + panel->bus_formats,
> > + panel->num_bus_formats);
> >
> > return 0;
> > }
> > @@ -128,6 +140,22 @@ EXPORT_SYMBOL(drm_panel_attach);
> > */
> > int drm_panel_detach(struct drm_panel *panel)
> > {
> > + struct drm_display_info *info;
> > +
> > + if (!panel->connector)
> > + goto out;
> > +
> > + info = >connector->display_info;
> > + info->width_mm = 0;
> > + info->height_mm = 0;
> > + info->bpc = 0;
> > + info->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> > + info->bus_flags = 0;
> > + kfree(info->bus_formats);
> > + info->bus_formats = NULL;
> > + info->num_bus_formats = 0;
> > +
> > +out:
> > panel->connector = NULL;
> > panel->drm = NULL;
> >
> > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> > index fc7da55f41d9..a6a881b987dd 100644
> > --- a/include/drm/drm_panel.h
> > +++ b/include/drm/drm_panel.h
> > @@ -39,6 +39,8 @@ enum drm_panel_orientation;
> > * 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
> > @@ -95,6 +97,18 @@ struct drm_panel {
> >
> > const struct drm_panel_funcs *funcs;
> >
> > + /*
> > + * panel information to be set in the connector when the panel is
> > + * attached.
> > + */
> > + unsigned int width_mm;
> > + unsigned int height_mm;
> > + unsigned int bpc;
> > + int orientation;
> > + const u32 *bus_formats;
> > + unsigned int num_bus_formats;
> > + u32 bus_flags;
> > +
> > struct list_head list;
> > };
> >
> > --
> > 2.22.0.410.gd8fdbe21b5-goog
> >
> > ___
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v3 3/4] drm/connector: Split out orientation quirk detection
On Mon, Jun 24, 2019 at 6:24 AM Ville Syrjälä
wrote:
>
> On Fri, Jun 21, 2019 at 08:41:04PM -0700, Derek Basehore wrote:
> > Not every platform needs quirk detection for panel orientation, so
> > split the drm_connector_init_panel_orientation_property into two
> > functions. One for platforms without the need for quirks, and the
> > other for platforms that need quirks.
> >
> > Signed-off-by: Derek Basehore
> > ---
> > drivers/gpu/drm/drm_connector.c | 45 -
> > drivers/gpu/drm/i915/intel_dp.c | 4 +--
> > drivers/gpu/drm/i915/vlv_dsi.c | 5 ++--
> > include/drm/drm_connector.h | 2 ++
> > 4 files changed, 39 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_connector.c
> > b/drivers/gpu/drm/drm_connector.c
> > index e17586aaa80f..c4b01adf927a 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 = >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;
> > @@ -1941,6 +1933,35 @@ int drm_connector_init_panel_orientation_property(
> > }
> > EXPORT_SYMBOL(drm_connector_init_panel_orientation_property);
> >
> > +/**
> > + * drm_connector_init_panel_orientation_property_quirk -
> > + * initialize the connecters panel_orientation property with a quirk
> > + * override
> > + * @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 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.
> > + *
> > + * Returns:
> > + * Zero on success, negative errno on failure.
> > + */
> > +int drm_connector_init_panel_orientation_property_quirk(
> > + struct drm_connector *connector, int width, int height)
> > +{
> > + int orientation_quirk;
> > +
> > + orientation_quirk = drm_get_panel_orientation_quirk(width, height);
> > + if (orientation_quirk != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
> > + connector->display_info.panel_orientation = orientation_quirk;
> > +
> > + return drm_connector_init_panel_orientation_property(connector);
> > +}
> > +EXPORT_SYMBOL(drm_connector_init_panel_orientation_property_quirk);
> > +
> > int drm_connector_set_obj_prop(struct drm_mode_object *obj,
> > struct drm_property *property,
> > uint64_t value)
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index b099a9dc28fd..7d4e61cf5463 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -7282,8 +7282,8 @@ static bool intel_edp_init_connector(struct intel_dp
> > *intel_dp,
> > intel_panel_setup_backlight(connector, pipe);
> >
> > if (fixed_mode)
> > - drm_connector_init_panel_orientation_property(
> > - connector, fixed_mode->hdisplay,
> >
Re: [PATCH v3 1/4] drm/panel: Add helper for reading DT rotation
On Mon, Jun 24, 2019 at 1:36 PM Sam Ravnborg wrote:
>
> Hi Derek.
>
> On Fri, Jun 21, 2019 at 08:41:02PM -0700, Derek Basehore wrote:
> > This adds a helper function for reading the rotation (panel
> > orientation) from the device tree.
> >
> > Signed-off-by: Derek Basehore
> > ---
> > drivers/gpu/drm/drm_panel.c | 42 +
> > include/drm/drm_panel.h | 7 +++
> > 2 files changed, 49 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> > index dbd5b873e8f2..507099af4b57 100644
> > --- a/drivers/gpu/drm/drm_panel.c
> > +++ b/drivers/gpu/drm/drm_panel.c
> > @@ -172,6 +172,48 @@ 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.
> > + */
> This function should better spell out why it talks about rotation versus
> orientation.
>
> It happens that orientation, due to bad design choices is named rotation
> in DT.
> But then this function is all about orientation, that just happens to be
> named rotation in DT.
> And the comments associated to the function should reflect this.
>
> something like:
> /**
> * of_drm_get_panel_orientation - look up the orientation 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 property of a panel in the device tree.
> * The orientation of the panel is expressed as a property named
> * "rotation" in the device tree.
> * The rotation in the device tree is counter clockwise.
> *
> * Return: 0 when a valid orientation value (0, 90, 180, or 270) is read or
> the
> * rotation property doesn't exist. -EERROR otherwise.
> */
I'll clear this up in the next patch set.
>
> This would at least remove some of my confusiuon.
> And then maybe add a bit more explanation to the binding property
> description too.
Tried this, yet I got told off for adding kernel details to the DT
documentation (which is frowned upon).
>
> Sam
>
>
>
>
>
>
>
>
>
>
>
>
> > +int of_drm_get_panel_orientation(const struct device_node *np,
> > + enum drm_panel_orientation *orientation)
> > +{
> > + int rotation, ret;
> > +
> > + ret = of_property_read_u32(np, "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 ");
> > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> > index 8c738c0e6e9f..3564952f1a4f 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,
> > + enum drm_panel_orientation *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,
> > + enum drm_panel_orientation *orientation)
> > +{
> > + return -ENODEV;
> > +}
> > #endif
> >
> > #endif
> > --
> > 2.22.0.410.gd8fdbe21b5-goog
Thanks for the review
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 2/4] drm/panel: set display info in panel attach
On Fri, Jun 21, 2019 at 8:41 PM Derek Basehore wrote:
>
> Devicetree systems can set panel orientation via a panel binding, but
> there's no way, as is, to propagate this setting to the connector,
> where the property need to be added.
> To address this, this patch sets orientation, as well as other fixed
> values for the panel, in the drm_panel_attach function. These values
> are stored from probe in the drm_panel struct.
>
> Signed-off-by: Derek Basehore
> ---
> drivers/gpu/drm/drm_panel.c | 28
> include/drm/drm_panel.h | 14 ++
> 2 files changed, 42 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index 507099af4b57..5690fca30236 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -104,11 +104,23 @@ EXPORT_SYMBOL(drm_panel_remove);
> */
> int drm_panel_attach(struct drm_panel *panel, struct drm_connector
> *connector)
> {
> + struct drm_display_info *info;
> +
> if (panel->connector)
> return -EBUSY;
>
> panel->connector = connector;
> panel->drm = connector->dev;
> + info = >display_info;
> + info->width_mm = panel->width_mm;
> + info->height_mm = panel->height_mm;
> + info->bpc = panel->bpc;
> + info->panel_orientation = panel->orientation;
> + info->bus_flags = panel->bus_flags;
> + if (panel->bus_formats)
> + drm_display_info_set_bus_formats(>display_info,
> +panel->bus_formats,
> +panel->num_bus_formats);
>
> return 0;
> }
> @@ -128,6 +140,22 @@ EXPORT_SYMBOL(drm_panel_attach);
> */
> int drm_panel_detach(struct drm_panel *panel)
> {
> + struct drm_display_info *info;
> +
> + if (!panel->connector)
> + goto out;
> +
> + info = >connector->display_info;
> + info->width_mm = 0;
> + info->height_mm = 0;
> + info->bpc = 0;
> + info->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> + info->bus_flags = 0;
> + kfree(info->bus_formats);
> + info->bus_formats = NULL;
> + info->num_bus_formats = 0;
> +
> +out:
> panel->connector = NULL;
> panel->drm = NULL;
>
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 3564952f1a4f..760ca5865962 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
> @@ -93,6 +95,18 @@ struct drm_panel {
>
> const struct drm_panel_funcs *funcs;
>
> + /*
> +* panel information to be set in the connector when the panel is
> +* attached.
> +*/
> + unsigned int width_mm;
> + unsigned int height_mm;
> + unsigned int bpc;
> + int orientation;
> + const u32 *bus_formats;
> + unsigned int num_bus_formats;
> + u32 bus_flags;
Should probably put these in a struct to ensure the connector and
panel have the same data types. Will do in a following patch if we
stay with this.
> +
> struct list_head list;
> };
>
> --
> 2.22.0.410.gd8fdbe21b5-goog
>
Re: [PATCH 3/5] drm/panel: Add attach/detach callbacks
On Fri, Jun 21, 2019 at 2:19 AM Thierry Reding wrote:
>
> On Tue, Jun 11, 2019 at 05:25:47PM -0700, dbasehore . wrote:
> > On Tue, Jun 11, 2019 at 1:57 AM Daniel Vetter 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
> > > > ---
> > > > 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
Re: [PATCH 3/5] drm/panel: Add attach/detach callbacks
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 . wrote:
>
> On Tue, Jun 11, 2019 at 1:57 AM Daniel Vetter 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
> > > ---
> > > 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 1/5] drm/panel: Add helper for reading DT rotation
On Fri, Jun 14, 2019 at 5:43 PM dbasehore . wrote:
>
> On Wed, Jun 12, 2019 at 2:18 PM Sam Ravnborg 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
> > > ---
> > > 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", );
> > > + 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 ");
> > > 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
On Wed, Jun 12, 2019 at 2:18 PM Sam Ravnborg 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
> > ---
> > 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", );
> > + 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 ");
> > 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 4/5] drm/connector: Split out orientation quirk detection
On Wed, Jun 12, 2019 at 5:33 AM Hans de Goede wrote: > > Hi, > > On 12-06-19 02:16, dbasehore . wrote: > > On Tue, Jun 11, 2019 at 1:54 AM Hans de Goede wrote: > >> > >> Hi, > >> > >> On 11-06-19 10:08, Jani Nikula wrote: > >>> On Mon, 10 Jun 2019, Derek Basehore 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
Re: [PATCH 1/5] drm/panel: Add helper for reading DT rotation
On Wed, Jun 12, 2019 at 2:20 PM Sam Ravnborg 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
> > ---
> > 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", );
>
> 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
Re: [PATCH 2/5] dt-bindings: display/panel: Expand rotation documentation
On Thu, Jun 13, 2019 at 5:52 AM Rob Herring wrote: > > On Tue, Jun 11, 2019 at 4:02 PM dbasehore . wrote: > > > > On Tue, Jun 11, 2019 at 8:25 AM Rob Herring wrote: > > > > > > On Mon, Jun 10, 2019 at 10:03 PM Derek Basehore > > > 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 > > > > --- > > > > .../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.
Re: [PATCH 3/5] drm/panel: Add attach/detach callbacks
On Tue, Jun 11, 2019 at 1:57 AM Daniel Vetter 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
> > ---
> > 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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/5] drm/connector: Split out orientation quirk detection
On Tue, Jun 11, 2019 at 1:54 AM Hans de Goede wrote: > > Hi, > > On 11-06-19 10:08, Jani Nikula wrote: > > On Mon, 10 Jun 2019, Derek Basehore 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 > >> --- > >> 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
Re: [PATCH 2/5] dt-bindings: display/panel: Expand rotation documentation
On Tue, Jun 11, 2019 at 8:25 AM Rob Herring wrote:
>
> On Mon, Jun 10, 2019 at 10:03 PM Derek Basehore
> 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
> > ---
> > .../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 = < 45 0>;
>
> > + pp33-gpios = < 35 0>;
> > + pp18-gpios = < 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 = <_pins_default>;
> > + pinctrl-1 = <_pins_3300mv>;
> > + pinctrl-2 = <_pins_1800mv>;
> > + backlight = <_lcd0>;
> > + rotation = <180>;
> > + status = "okay";
> > +
> > + port {
> > + panel_in: endpoint {
> > + remote-endpoint = <_out>;
> > + };
> > + };
> > + };
> > --
> > 2.22.0.rc2.383.gf4fbbf30c2-goog
> >
Re: [PATCH] [v7, 2/2] drm/panel: Add Boe Himax8279d MIPI-DSI LCD panel
On Tue, Mar 26, 2019 at 12:09 AM Jerry Han wrote: > > Support Boe Himax8279d 8.0" 1200x1920 TFT LCD panel, it is a MIPI DSI > panel. > > V7: > - Add the information of the reviewer > - Remove unnecessary delays, The udelay_range code gracefully returns > without hitting the scheduler on a delay of 0. (Derek) > - Merge the same data structures, like display_mode and off_cmds (Derek) > - Optimize the processing of results returned by > devm_gpiod_get_optional (Derek) Looks good to me overall. There's still some duplication on values like number of lanes, but that's not a blocking concern for me. > > V6: > - Add the information of the reviewer (Sam) > - Delete unnecessary header files #include (Sam) > - The config DRM_PANEL_BOE_HIMAX8279D appears twice. Drop one of them (Sam) > - ADD static, set_gpios function is not used outside this module (Sam) > > V5: > - Added changelog > > V4: > - Frefix all function maes with boe_ (Sam) > - Fsed "enable_gpio" replace "reset_gpio", Make it look clearer (Sam) > - Sort include lines alphabetically (Sam) > - Fixed entries in the makefile must be sorted alphabetically (Sam) > - Add send_mipi_cmds function to avoid duplicating the code (Sam) > - Add the necessary delay(reset_delay_t5) between reset and sending > the initialization command (Rock wang) > > V3: > - Remove unnecessary delays in sending initialization commands (Jitao Shi) > > V2: > - Use SPDX identifier (Sam) > - Use necessary header files replace drmP.h (Sam) > - Delete unnecessary header files #include (Sam) > - Specifies a GPIOs array to control the reset timing, > instead of reading "dsi-reset-sequence" data from DTS (Sam) > - Delete backlight_disable() function when already disabled (Sam) > - Use devm_of_find_backlight() replace of_find_backlight_by_node() (Sam) > - Move the necessary data in the DTS to the current file, > like porch, display_mode and Init code etc. (Sam) > - Add compatible device "boe,himax8279d10p" (Sam) > > Signed-off-by: Jerry Han > Reviewed-by: Sam Ravnborg > Reviewed-by: Derek Basehore > Cc: Jitao Shi > Cc: Rock wang > --- > MAINTAINERS |6 + > drivers/gpu/drm/panel/Kconfig| 11 + > drivers/gpu/drm/panel/Makefile |1 + > drivers/gpu/drm/panel/panel-boe-himax8279d.c | 1050 ++ > 4 files changed, 1068 insertions(+) > create mode 100644 drivers/gpu/drm/panel/panel-boe-himax8279d.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index f8e63bcc4c1c..267468ca72bb 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -4816,6 +4816,12 @@ T: git git://anongit.freedesktop.org/drm/drm-misc > S: Maintained > F: drivers/gpu/drm/bochs/ > > +DRM DRIVER FOR BOE HIMAX8279D PANELS > +M: Jerry Han > +S: Maintained > +F: drivers/gpu/drm/panel/panel-boe-himax8279d.c > +F: Documentation/devicetree/bindings/display/panel/boe,himax8279d.txt > + > DRM DRIVER FOR FARADAY TVE200 TV ENCODER > M: Linus Walleij > T: git git://anongit.freedesktop.org/drm/drm-misc > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig > index f53f817356db..fe2f87cc6627 100644 > --- a/drivers/gpu/drm/panel/Kconfig > +++ b/drivers/gpu/drm/panel/Kconfig > @@ -17,6 +17,17 @@ config DRM_PANEL_ARM_VERSATILE > reference designs. The panel is detected using special registers > in the Versatile family syscon registers. > > +config DRM_PANEL_BOE_HIMAX8279D > + tristate "Boe Himax8279d panel" > + depends on OF > + depends on DRM_MIPI_DSI > + depends on BACKLIGHT_CLASS_DEVICE > + help > + Say Y here if you want to enable support for Boe Himax8279d > + TFT-LCD modules. The panel has a 1200x1920 resolution and uses > + 24 bit RGB per pixel. It provides a MIPI DSI interface to > + the host and has a built-in LED backlight. > + > config DRM_PANEL_LVDS > tristate "Generic LVDS panel driver" > depends on OF > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile > index 7834947a53b0..c4814b6f069e 100644 > --- a/drivers/gpu/drm/panel/Makefile > +++ b/drivers/gpu/drm/panel/Makefile > @@ -1,5 +1,6 @@ > # SPDX-License-Identifier: GPL-2.0 > obj-$(CONFIG_DRM_PANEL_ARM_VERSATILE) += panel-arm-versatile.o > +obj-$(CONFIG_DRM_PANEL_BOE_HIMAX8279D) += panel-boe-himax8279d.o > obj-$(CONFIG_DRM_PANEL_LVDS) += panel-lvds.o > obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o > obj-$(CONFIG_DRM_PANEL_ILITEK_IL9322) += panel-ilitek-ili9322.o > diff --git a/drivers/gpu/drm/panel/panel-boe-himax8279d.c > b/drivers/gpu/drm/panel/panel-boe-himax8279d.c > new file mode 100644 > index ..ff5a89e38fd7 > --- /dev/null > +++ b/drivers/gpu/drm/panel/panel-boe-himax8279d.c > @@ -0,0 +1,1050 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2019, Huaqin Telecom Technology Co., Ltd > + * > + * Author: Jerry Han > + * > + */ > + > +#include >
Re: [PATCH] [v6, 2/2] drm/panel: Add Boe Himax8279d MIPI-DSI LCD panel
On Mon, Mar 11, 2019 at 3:04 AM Jerry Han wrote:
>
> Support Boe Himax8279d 8.0" 1200x1920 TFT LCD panel, it is a MIPI DSI
> panel.
>
> V6:
> - Add the information of the reviewer (Sam)
> - Delete unnecessary header files #include (Sam)
> - The config DRM_PANEL_BOE_HIMAX8279D appears twice. Drop one of them (Sam)
> - ADD static, set_gpios function is not used outside this module (Sam)
>
> v5:
> - Added changelog
>
> v4:
> - Frefix all function maes with boe_ (Sam)
> - Fsed "enable_gpio" replace "reset_gpio", Make it look clearer (Sam)
> - Sort include lines alphabetically (Sam)
> - Fixed entries in the makefile must be sorted alphabetically (Sam)
> - Add send_mipi_cmds function to avoid duplicating the code (Sam)
> - Add the necessary delay(reset_delay_t5) between reset and sending
> the initialization command (Rock wang)
>
> v3:
> - Remove unnecessary delays in sending initialization commands (Jitao Shi)
>
> V2:
> - Use SPDX identifier (Sam)
> - Use necessary header files replace drmP.h (Sam)
> - Delete unnecessary header files #include (Sam)
> - Specifies a GPIOs array to control the reset timing,
> instead of reading "dsi-reset-sequence" data from DTS (Sam)
> - Delete backlight_disable() function when already disabled (Sam)
> - Use devm_of_find_backlight() replace of_find_backlight_by_node() (Sam)
> - Move the necessary data in the DTS to the current file,
> like porch, display_mode and Init code etc. (Sam)
> - Add compatible device "boe,himax8279d10p" (Sam)
>
> Signed-off-by: Jerry Han
> Reviewed-by: Sam Ravnborg
> Cc: Jitao Shi
> Cc: Derek Basehore
> Cc: Rock wang
> ---
> MAINTAINERS |6 +
> drivers/gpu/drm/panel/Kconfig| 11 +
> drivers/gpu/drm/panel/Makefile |1 +
> drivers/gpu/drm/panel/panel-boe-himax8279d.c | 1069
> ++
> 4 files changed, 1087 insertions(+)
> create mode 100644 drivers/gpu/drm/panel/panel-boe-himax8279d.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b2f710e..095fbbe 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4624,6 +4624,12 @@ T: git git://anongit.freedesktop.org/drm/drm-misc
> S: Maintained
> F: drivers/gpu/drm/bochs/
>
> +DRM DRIVER FOR BOE HIMAX8279D PANELS
> +M: Jerry Han
> +S: Maintained
> +F: drivers/gpu/drm/panel/panel-boe-himax8279d.c
> +F: Documentation/devicetree/bindings/display/panel/boe,himax8279d.txt
> +
> DRM DRIVER FOR FARADAY TVE200 TV ENCODER
> M: Linus Walleij
> T: git git://anongit.freedesktop.org/drm/drm-misc
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 6020c30..4aae4a7 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -17,6 +17,17 @@ config DRM_PANEL_ARM_VERSATILE
> reference designs. The panel is detected using special registers
> in the Versatile family syscon registers.
>
> +config DRM_PANEL_BOE_HIMAX8279D
> + tristate "Boe Himax8279d panel"
> + depends on OF
> + depends on DRM_MIPI_DSI
> + depends on BACKLIGHT_CLASS_DEVICE
> + help
> + Say Y here if you want to enable support for Boe Himax8279d
> + TFT-LCD modules. The panel has a 1200x1920 resolution and uses
> + 24 bit RGB per pixel. It provides a MIPI DSI interface to
> + the host and has a built-in LED backlight.
> +
> config DRM_PANEL_LVDS
> tristate "Generic LVDS panel driver"
> depends on OF
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 5ccaaa9..7285539 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_DRM_PANEL_ARM_VERSATILE) += panel-arm-versatile.o
> +obj-$(CONFIG_DRM_PANEL_BOE_HIMAX8279D) += panel-boe-himax8279d.o
> obj-$(CONFIG_DRM_PANEL_LVDS) += panel-lvds.o
> obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
> obj-$(CONFIG_DRM_PANEL_ILITEK_IL9322) += panel-ilitek-ili9322.o
> diff --git a/drivers/gpu/drm/panel/panel-boe-himax8279d.c
> b/drivers/gpu/drm/panel/panel-boe-himax8279d.c
> new file mode 100644
> index 000..c050a48
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-boe-himax8279d.c
> @@ -0,0 +1,1069 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019, Huaqin Telecom Technology Co., Ltd
> + *
> + * Author: Jerry Han
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +struct panel_cmd {
> + size_t len;
> + const char *data;
> +};
> +
> +#define _INIT_CMD(...) { \
> + .len = sizeof((char[]){__VA_ARGS__}), \
> + .data = (char[]){__VA_ARGS__} }
> +
> +struct panel_desc {
> + const struct drm_display_mode *display_mode;
> + unsigned int bpc;
> + unsigned int
Re: [PATCH] [v6, 2/2] drm/panel: Add Boe Himax8279d MIPI-DSI LCD panel
On Thu, Mar 21, 2019 at 11:28 PM Jerry Han
wrote:
>
> Hi Derek Basehore:
> Thank you for your valuable advice.I'll make the changes as you suggest.
>
> I have a question to ask you ,
> > +static const struct panel_desc boe_himax8279d8p_panel_desc = {
> > + .display_mode = _himax8279d8p_display_mode,
> > + .bpc = 8,
> > + .width_mm = 107,
> > + .height_mm = 172,
> > + .delay_t1 = 5000,
> > + .reset_delay_t2 = 14000,
> > + .reset_delay_t3 = 1000,
> > + .reset_delay_t4 = 1000,
> > + .reset_delay_t5 = 5000,
>
> nit: could #define the values that are shared between both panels. In
> fact, you could likely remove values like the delays, bpc, lanes,
> mode_flags, and format from the panel_desc struct.
>
> -> -> ->
> #define the values is unnecessary, These values have no special meaning and
> universal.
> This device current values are not fixed and may be adjusted later,
> Other compatible devices may have different values.
Will the 2 displays, which seem to pretty much be the same display
with different sizes, likely have different reset delays?
bpc and lanes aren't going to change. mode_flags and format are
unlikely to change independently. Also, if we need different reset
delays for the two different panels, we can add the code back in.
>
> > + .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> > + MIPI_DSI_CLOCK_NON_CONTINUOUS | MIPI_DSI_MODE_LPM,
> > + .format = MIPI_DSI_FMT_RGB888,
> > + .lanes = 4,
> > + .on_cmds = boe_himax8279d8p_on_cmds,
> > + .off_cmds = boe_himax8279d8p_off_cmds,
> > +};
>
> Thanks
>
> dbasehore . 于2019年3月22日周五 上午9:24写道：
>>
>> On Mon, Mar 11, 2019 at 3:04 AM Jerry Han wrote:
>> >
>> > Support Boe Himax8279d 8.0" 1200x1920 TFT LCD panel, it is a MIPI DSI
>> > panel.
>> >
>> > V6:
>> > - Add the information of the reviewer (Sam)
>> > - Delete unnecessary header files #include (Sam)
>> > - The config DRM_PANEL_BOE_HIMAX8279D appears twice. Drop one of them (Sam)
>> > - ADD static, set_gpios function is not used outside this module (Sam)
>> >
>> > v5:
>> > - Added changelog
>> >
>> > v4:
>> > - Frefix all function maes with boe_ (Sam)
>> > - Fsed "enable_gpio" replace "reset_gpio", Make it look clearer (Sam)
>> > - Sort include lines alphabetically (Sam)
>> > - Fixed entries in the makefile must be sorted alphabetically (Sam)
>> > - Add send_mipi_cmds function to avoid duplicating the code (Sam)
>> > - Add the necessary delay(reset_delay_t5) between reset and sending
>> > the initialization command (Rock wang)
>> >
>> > v3:
>> > - Remove unnecessary delays in sending initialization commands (Jitao Shi)
>> >
>> > V2:
>> > - Use SPDX identifier (Sam)
>> > - Use necessary header files replace drmP.h (Sam)
>> > - Delete unnecessary header files #include (Sam)
>> > - Specifies a GPIOs array to control the reset timing,
>> > instead of reading "dsi-reset-sequence" data from DTS (Sam)
>> > - Delete backlight_disable() function when already disabled (Sam)
>> > - Use devm_of_find_backlight() replace of_find_backlight_by_node() (Sam)
>> > - Move the necessary data in the DTS to the current file,
>> > like porch, display_mode and Init code etc. (Sam)
>> > - Add compatible device "boe,himax8279d10p" (Sam)
>> >
>> > Signed-off-by: Jerry Han
>> > Reviewed-by: Sam Ravnborg
>> > Cc: Jitao Shi
>> > Cc: Derek Basehore
>> > Cc: Rock wang
>> > ---
>> > MAINTAINERS |6 +
>> > drivers/gpu/drm/panel/Kconfig| 11 +
>> > drivers/gpu/drm/panel/Makefile |1 +
>> > drivers/gpu/drm/panel/panel-boe-himax8279d.c | 1069
>> > ++
>> > 4 files changed, 1087 insertions(+)
>> > create mode 100644 drivers/gpu/drm/panel/panel-boe-himax8279d.c
>> >
>> > diff --git a/MAINTAINERS b/MAINTAINERS
>> > index b2f710e..095fbbe 100644
>> > --- a/MAINTAINERS
>> > +++ b/MAINTAINERS
>> > @@ -4624,6 +4624,12 @@ T: git
>> > git://anongit.freedesktop.org/drm/drm-misc
>> > S: Maintained
>> > F: drivers/gpu/drm/bochs/
>> >
>> > +DRM DRIVER FOR BOE HIMAX8279D PANELS
>> > +M: Jerry Han
>> > +
[PATCH v10 5/5] drm/rockchip: Add dmc notifier in vop driver
On Wed, Sep 7, 2016 at 1:31 PM, Sean Paul wrote:
> On Wed, Sep 7, 2016 at 2:13 PM, dbasehore . wrote:
>> On Tue, Sep 6, 2016 at 10:18 AM, Sean Paul wrote:
>>> On Mon, Sep 5, 2016 at 1:06 AM, Lin Huang wrote:
>>>> when in ddr frequency scaling process, vop can not do enable or
>>>> disable operation, since in dcf we check vop clock to see whether
>>>> vop work. If vop work, dcf do ddr frequency scaling when vop
>>>> in vblank status, and we need to read vop register to check whether
>>>> vop go into vblank status. If vop not work, dcf can do ddr frequency
>>>> any time. So when do ddr frequency scaling, you disabled or enable
>>>> vop, there may two bad thing happen: 1, the panel flicker(when vop from
>>>> disable status change to enable). 2, kernel hang (when vop from enable
>>>> status change to disable, dcf need to read vblank status, but if you
>>>> disable
>>>> vop clock, it can not get the status, it will lead soc dead) So we need
>>>> register to devfreq notifier, and we can get the dmc status. Also, when
>>>> there have two vop enabled, we need to disable dmc, since dcf only base
>>>> on one vop vblank time, so the other panel will flicker when do ddr
>>>> frequency scaling.
>>>>
>>>> Signed-off-by: Lin Huang
>>>> Reviewed-by: Chanwoo Choi
>>>> ---
>>>> Changes in v10:
>>>> - None
>>>>
>>>> Changes in v9:
>>>> - None
>>>>
>>>> Changes in v8:
>>>> - None
>>>>
>>>> Changes in v7:
>>>> - None
>>>>
>>>> Changes in v6:
>>>> - fix a build error
>>>>
>>>> Changes in v5:
>>>> - improve some nits
>>>>
>>>> Changes in v4:
>>>> - register notifier to devfreq_register_notifier
>>>> - use DEVFREQ_PRECHANGE and DEVFREQ_POSTCHANGE to get dmc status
>>>> - when two vop enable, disable dmc
>>>> - when two vop back to one vop, enable dmc
>>>>
>>>> Changes in v3:
>>>> - when do vop eanble/disable, dmc will wait until it finish
>>>>
>>>> Changes in v2:
>>>> - None
>>>>
>>>> Changes in v1:
>>>> - use wait_event instead usleep
>>>>
>>>> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 116
>>>>
>>>> 1 file changed, 116 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>> index efbc41a..a73f3aa 100644
>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>> @@ -19,6 +19,8 @@
>>>> #include
>>>> #include
>>>>
>>>> +#include
>>>> +#include
>>>> #include
>>>> #include
>>>> #include
>>>> @@ -118,6 +120,13 @@ struct vop {
>>>>
>>>> const struct vop_data *data;
>>>>
>>>> + struct devfreq *devfreq;
>>>> + struct devfreq_event_dev *devfreq_event_dev;
>>>> + struct notifier_block dmc_nb;
>>>> + int dmc_in_process;
>>>> + int vop_switch_status;
>>>> + wait_queue_head_t wait_dmc_queue;
>>>> + wait_queue_head_t wait_vop_switch_queue;
>>>> uint32_t *regsbak;
>>>> void __iomem *regs;
>>>>
>>>> @@ -428,11 +437,47 @@ static void vop_dsp_hold_valid_irq_disable(struct
>>>> vop *vop)
>>>> spin_unlock_irqrestore(>irq_lock, flags);
>>>> }
>>>>
>>>> +static int dmc_notify(struct notifier_block *nb, unsigned long event,
>>>> + void *data)
>>>> +{
>>>> + struct vop *vop = container_of(nb, struct vop, dmc_nb);
>>>> +
>>>> + if (event == DEVFREQ_PRECHANGE) {
>>>> + /*
>>>> +* check if vop in enable or disable process,
>>>> +* if yes, wait until it finishes, use 200ms as
>>>> +* timeout.
>>>> +*/
>>>> + if (!wait_event_timeout(vop->wait_vop_switch_queue,
>>>> + !vop->vop_switch_status, HZ / 5))
>
[PATCH v10 5/5] drm/rockchip: Add dmc notifier in vop driver
On Tue, Sep 6, 2016 at 10:18 AM, Sean Paul wrote:
> On Mon, Sep 5, 2016 at 1:06 AM, Lin Huang wrote:
>> when in ddr frequency scaling process, vop can not do enable or
>> disable operation, since in dcf we check vop clock to see whether
>> vop work. If vop work, dcf do ddr frequency scaling when vop
>> in vblank status, and we need to read vop register to check whether
>> vop go into vblank status. If vop not work, dcf can do ddr frequency
>> any time. So when do ddr frequency scaling, you disabled or enable
>> vop, there may two bad thing happen: 1, the panel flicker(when vop from
>> disable status change to enable). 2, kernel hang (when vop from enable
>> status change to disable, dcf need to read vblank status, but if you disable
>> vop clock, it can not get the status, it will lead soc dead) So we need
>> register to devfreq notifier, and we can get the dmc status. Also, when
>> there have two vop enabled, we need to disable dmc, since dcf only base
>> on one vop vblank time, so the other panel will flicker when do ddr
>> frequency scaling.
>>
>> Signed-off-by: Lin Huang
>> Reviewed-by: Chanwoo Choi
>> ---
>> Changes in v10:
>> - None
>>
>> Changes in v9:
>> - None
>>
>> Changes in v8:
>> - None
>>
>> Changes in v7:
>> - None
>>
>> Changes in v6:
>> - fix a build error
>>
>> Changes in v5:
>> - improve some nits
>>
>> Changes in v4:
>> - register notifier to devfreq_register_notifier
>> - use DEVFREQ_PRECHANGE and DEVFREQ_POSTCHANGE to get dmc status
>> - when two vop enable, disable dmc
>> - when two vop back to one vop, enable dmc
>>
>> Changes in v3:
>> - when do vop eanble/disable, dmc will wait until it finish
>>
>> Changes in v2:
>> - None
>>
>> Changes in v1:
>> - use wait_event instead usleep
>>
>> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 116
>>
>> 1 file changed, 116 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index efbc41a..a73f3aa 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -19,6 +19,8 @@
>> #include
>> #include
>>
>> +#include
>> +#include
>> #include
>> #include
>> #include
>> @@ -118,6 +120,13 @@ struct vop {
>>
>> const struct vop_data *data;
>>
>> + struct devfreq *devfreq;
>> + struct devfreq_event_dev *devfreq_event_dev;
>> + struct notifier_block dmc_nb;
>> + int dmc_in_process;
>> + int vop_switch_status;
>> + wait_queue_head_t wait_dmc_queue;
>> + wait_queue_head_t wait_vop_switch_queue;
>> uint32_t *regsbak;
>> void __iomem *regs;
>>
>> @@ -428,11 +437,47 @@ static void vop_dsp_hold_valid_irq_disable(struct vop
>> *vop)
>> spin_unlock_irqrestore(>irq_lock, flags);
>> }
>>
>> +static int dmc_notify(struct notifier_block *nb, unsigned long event,
>> + void *data)
>> +{
>> + struct vop *vop = container_of(nb, struct vop, dmc_nb);
>> +
>> + if (event == DEVFREQ_PRECHANGE) {
>> + /*
>> +* check if vop in enable or disable process,
>> +* if yes, wait until it finishes, use 200ms as
>> +* timeout.
>> +*/
>> + if (!wait_event_timeout(vop->wait_vop_switch_queue,
>> + !vop->vop_switch_status, HZ / 5))
>> + dev_warn(vop->dev,
>> +"Timeout waiting for vop swtich status\n");
>> + vop->dmc_in_process = 1;
>> + } else if (event == DEVFREQ_POSTCHANGE) {
>> + vop->dmc_in_process = 0;
>> + wake_up(>wait_dmc_queue);
>> + }
>> +
>> + return NOTIFY_OK;
>> +}
>> +
>> static int vop_enable(struct drm_crtc *crtc)
>> {
>> struct vop *vop = to_vop(crtc);
>> + int num_enabled_crtc = 0;
>> int ret;
>>
>> + /*
>> +* if in dmc scaling frequency process, wait until it finishes
>> +* use 200ms as timeout time.
>> +*/
>> + if (!wait_event_timeout(vop->wait_dmc_queue,
>> + !vop->dmc_in_process, HZ / 5))
>> + dev_warn(vop->dev,
>> +"Timeout waiting for dmc when vop enable\n");
>> +
>
>
> This wait_event_timeout code is terribly racey (same goes above and below).
>
>
>> + vop->vop_switch_status = 1;
>> +
>> ret = pm_runtime_get_sync(vop->dev);
>> if (ret < 0) {
>> dev_err(vop->dev, "failed to get pm runtime: %d\n", ret);
>> @@ -479,6 +524,21 @@ static int vop_enable(struct drm_crtc *crtc)
>>
>> drm_crtc_vblank_on(crtc);
>>
>> + vop->vop_switch_status = 0;
>> + wake_up(>wait_vop_switch_queue);
>> +
>> + /* check how many VOPs in use now */
>> + drm_for_each_crtc(crtc, vop->drm_dev) {
>> + if (crtc->state->enable)
>
> I think you really want to check
[PATCH v10 5/5] drm/rockchip: Add dmc notifier in vop driver
On Tue, Sep 6, 2016 at 12:07 PM, Sean Paul wrote:
> On Tue, Sep 6, 2016 at 3:01 PM, hl wrote:
>> Hi
>>
>>
>> On 2016å¹´09æ07æ¥ 02:55, Sean Paul wrote:
>>>
>>> On Tue, Sep 6, 2016 at 2:15 PM, hl wrote:
Hi Sean,
On 2016å¹´09æ07æ¥ 01:18, Sean Paul wrote:
>
> On Mon, Sep 5, 2016 at 1:06 AM, Lin Huang wrote:
>>
>> when in ddr frequency scaling process, vop can not do enable or
>> disable operation, since in dcf we check vop clock to see whether
>> vop work. If vop work, dcf do ddr frequency scaling when vop
>> in vblank status, and we need to read vop register to check whether
>> vop go into vblank status. If vop not work, dcf can do ddr frequency
>> any time. So when do ddr frequency scaling, you disabled or enable
>> vop, there may two bad thing happen: 1, the panel flicker(when vop from
>> disable status change to enable). 2, kernel hang (when vop from enable
>> status change to disable, dcf need to read vblank status, but if you
>> disable
>> vop clock, it can not get the status, it will lead soc dead) So we need
>> register to devfreq notifier, and we can get the dmc status. Also, when
>> there have two vop enabled, we need to disable dmc, since dcf only base
>> on one vop vblank time, so the other panel will flicker when do ddr
>> frequency scaling.
>>
>> Signed-off-by: Lin Huang
>> Reviewed-by: Chanwoo Choi
>> ---
>> Changes in v10:
>> - None
>>
>> Changes in v9:
>> - None
>>
>> Changes in v8:
>> - None
>>
>> Changes in v7:
>> - None
>>
>> Changes in v6:
>> - fix a build error
>>
>> Changes in v5:
>> - improve some nits
>>
>> Changes in v4:
>> - register notifier to devfreq_register_notifier
>> - use DEVFREQ_PRECHANGE and DEVFREQ_POSTCHANGE to get dmc status
>> - when two vop enable, disable dmc
>> - when two vop back to one vop, enable dmc
>>
>> Changes in v3:
>> - when do vop eanble/disable, dmc will wait until it finish
>>
>> Changes in v2:
>> - None
>>
>> Changes in v1:
>> - use wait_event instead usleep
>>
>>drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 116
>>
>>1 file changed, 116 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index efbc41a..a73f3aa 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -19,6 +19,8 @@
>>#include
>>#include
>>
>> +#include
>> +#include
>>#include
>>#include
>>#include
>> @@ -118,6 +120,13 @@ struct vop {
>>
>> const struct vop_data *data;
>>
>> + struct devfreq *devfreq;
>> + struct devfreq_event_dev *devfreq_event_dev;
>> + struct notifier_block dmc_nb;
>> + int dmc_in_process;
>> + int vop_switch_status;
>> + wait_queue_head_t wait_dmc_queue;
>> + wait_queue_head_t wait_vop_switch_queue;
>> uint32_t *regsbak;
>> void __iomem *regs;
>>
>> @@ -428,11 +437,47 @@ static void vop_dsp_hold_valid_irq_disable(struct
>> vop *vop)
>> spin_unlock_irqrestore(>irq_lock, flags);
>>}
>>
>> +static int dmc_notify(struct notifier_block *nb, unsigned long event,
>> + void *data)
>> +{
>> + struct vop *vop = container_of(nb, struct vop, dmc_nb);
>> +
>> + if (event == DEVFREQ_PRECHANGE) {
>> + /*
>> +* check if vop in enable or disable process,
>> +* if yes, wait until it finishes, use 200ms as
>> +* timeout.
>> +*/
>> + if (!wait_event_timeout(vop->wait_vop_switch_queue,
>> + !vop->vop_switch_status, HZ /
>> 5))
>> + dev_warn(vop->dev,
>> +"Timeout waiting for vop swtich
>> status\n");
>> + vop->dmc_in_process = 1;
>> + } else if (event == DEVFREQ_POSTCHANGE) {
>> + vop->dmc_in_process = 0;
>> + wake_up(>wait_dmc_queue);
>> + }
>> +
>> + return NOTIFY_OK;
>> +}
>> +
>>static int vop_enable(struct drm_crtc *crtc)
>>{
>> struct vop *vop = to_vop(crtc);
>> + int num_enabled_crtc = 0;
>> int ret;
>>
>> + /*
>> +* if in dmc scaling frequency process, wait until it finishes
>> +* use 200ms as timeout time.
>> +*/
>> + if (!wait_event_timeout(vop->wait_dmc_queue,
>> +
[RFC PATCH 4/4] drm/rockchip: Add dmc notifier in vop driver
On Wed, Jun 1, 2016 at 2:35 AM, Lin Huang wrote:
> when in ddr frequency scaling process, vop can not do
> enable or disable operate, since dcf will base on vop vblank
> time to do frequency scaling and need to get vop irq if there
> have vop enabled. So need register to dmc notifier, and we can
> get the dmc status.
>
> Signed-off-by: Lin Huang
> ---
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 51
> +++--
> 1 file changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 1c4d5b5..7ee0cd0 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -31,6 +31,8 @@
> #include
> #include
>
> +#include
> +
> #include "rockchip_drm_drv.h"
> #include "rockchip_drm_gem.h"
> #include "rockchip_drm_fb.h"
> @@ -116,6 +118,9 @@ struct vop {
>
> const struct vop_data *data;
>
> + struct notifier_block dmc_nb;
> + int dmc_in_process;
> +
> uint32_t *regsbak;
> void __iomem *regs;
>
> @@ -426,14 +431,41 @@ static void vop_dsp_hold_valid_irq_disable(struct vop
> *vop)
> spin_unlock_irqrestore(>irq_lock, flags);
> }
>
> +static int dmc_notify(struct notifier_block *nb, unsigned long event,
> + void *data)
> +{
> + struct vop *vop = container_of(nb, struct vop, dmc_nb);
> +
> + if (event == DMCFREQ_ADJUST)
> + vop->dmc_in_process = 1;
> + else if (event == DMCFREQ_FINISH)
> + vop->dmc_in_process = 0;
> +
> + return NOTIFY_OK;
> +}
> +
> static void vop_enable(struct drm_crtc *crtc)
> {
> struct vop *vop = to_vop(crtc);
> int ret;
> + int timeout_count = 500;
> + int timeout_loop = 0;
>
> if (vop->is_enabled)
> return;
>
> + /*
> +* if in dmc scaling frequency process, wait until it finish
> +* use 100ms as timeout time.
> +*/
> + while (timeout_loop < timeout_count) {
> + if (vop->dmc_in_process == 0)
> + break;
> +
> + timeout_loop++;
> + usleep_range(150, 200);
> + }
wait_event with wake_up_all should work better here than using usleep_range.
> +
> ret = pm_runtime_get_sync(vop->dev);
> if (ret < 0) {
> dev_err(vop->dev, "failed to get pm runtime: %d\n", ret);
> @@ -485,6 +517,7 @@ static void vop_enable(struct drm_crtc *crtc)
> enable_irq(vop->irq);
>
> drm_crtc_vblank_on(crtc);
> + rockchip_dmc_get(>dmc_nb);
>
> return;
>
> @@ -500,11 +533,25 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
> {
> struct vop *vop = to_vop(crtc);
> int i;
> + int timeout_count = 500;
> + int timeout_loop = 0;
>
> if (!vop->is_enabled)
> return;
>
> /*
> +* if in dmc scaling frequency process, wait until it finish
> +* use 100ms as timeout time.
> +*/
> + while (timeout_loop < timeout_count) {
> + if (vop->dmc_in_process == 0)
> + break;
> +
> + timeout_loop++;
> + usleep_range(150, 200);
> + }
> +
> + /*
> * We need to make sure that all windows are disabled before we
> * disable that crtc. Otherwise we might try to scan from a destroyed
> * buffer later.
> @@ -517,7 +564,7 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
> VOP_WIN_SET(vop, win, enable, 0);
> spin_unlock(>reg_lock);
> }
> -
> + rockchip_dmc_put(>dmc_nb);
> drm_crtc_vblank_off(crtc);
>
> /*
> @@ -1243,7 +1290,7 @@ static int vop_create_crtc(struct vop *vop)
> ret = -ENOENT;
> goto err_cleanup_crtc;
> }
> -
> + vop->dmc_nb.notifier_call = dmc_notify;
> init_completion(>dsp_hold_completion);
> init_completion(>wait_update_complete);
> crtc->port = port;
> --
> 1.9.1
>
[PATCH v11 0/4] Allow USB devices to remain runtime-suspended when sleeping
On Tue, Jan 5, 2016 at 4:48 AM, Rafael J. Wysocki wrote: > On Monday, January 04, 2016 06:27:18 PM Derek Basehore wrote: >> On Mon, Nov 02, 2015 at 02:50:40AM +0100, Rafael J. Wysocki wrote: >> > >> > I've queued up this series for the second half of the v4.4 merge window. >> > >> > Thanks, >> > Rafael >> > >> > >> > ___ >> > linux-arm-kernel mailing list >> > linux-arm-kernel at lists.infradead.org >> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> >> What's the status of this patch series? I haven't seen the patches >> posted for v4.4, plus there's the issue that Dan found that needs to be >> addressed. >> >> Is there a new revision of the patch series being worked on? > > Tomeu is not working on one AFAICS, but I may just revive his series. > > Do you have a pointer to the Dan's report? > > Thanks, > Rafael > It was a reply to the second patch in the series. Here's a link to it https://lkml.org/lkml/2015/11/10/107
