| Message ID | 20220606152431.1889185-2-hsinyi@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a panel API to return panel orientation | expand |
Quoting Hsin-Yi Wang (2022-06-06 08:24:24) > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > index f634371c717a..e12056cfeca8 100644 > --- a/drivers/gpu/drm/drm_panel.c > +++ b/drivers/gpu/drm/drm_panel.c > @@ -223,6 +223,15 @@ int drm_panel_get_modes(struct drm_panel *panel, > } > EXPORT_SYMBOL(drm_panel_get_modes); > > +enum drm_panel_orientation drm_panel_get_orientation(struct drm_panel *panel) Should 'panel' be marked const to indicate that it can't be modified? > +{ > + if (panel && panel->funcs && panel->funcs->get_orientation) > + return panel->funcs->get_orientation(panel);
Hi Hsin-Yi, On Mon, Jun 06, 2022 at 11:24:24PM +0800, Hsin-Yi Wang wrote: > Panels usually call drm_connector_set_panel_orientation(), which is > later than drm/kms driver calling drm_dev_register(). This leads to a > WARN(). > > The orientation property is known earlier. For example, some panels > parse the property through device tree during probe. > > Add an API to return the property from panel to drm/kms driver, so the > drivers are able to call drm_connector_set_panel_orientation() before > drm_dev_register(). > > Suggested-by: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > Reviewed-by: Douglas Anderson <dianders@chromium.org> > --- > v3->v4: Add a blank line. > --- > drivers/gpu/drm/drm_panel.c | 9 +++++++++ > include/drm/drm_panel.h | 10 ++++++++++ > 2 files changed, 19 insertions(+) > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > index f634371c717a..e12056cfeca8 100644 > --- a/drivers/gpu/drm/drm_panel.c > +++ b/drivers/gpu/drm/drm_panel.c > @@ -223,6 +223,15 @@ int drm_panel_get_modes(struct drm_panel *panel, > } > EXPORT_SYMBOL(drm_panel_get_modes); > > +enum drm_panel_orientation drm_panel_get_orientation(struct drm_panel *panel) const as mentioned by Stephen. > +{ > + if (panel && panel->funcs && panel->funcs->get_orientation) > + return panel->funcs->get_orientation(panel); > + > + return DRM_MODE_PANEL_ORIENTATION_UNKNOWN; > +} > +EXPORT_SYMBOL(drm_panel_get_orientation); > + > #ifdef CONFIG_OF > /** > * of_drm_find_panel - look up a panel using a device tree node > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > index d279ee455f01..5dadbf3b0370 100644 > --- a/include/drm/drm_panel.h > +++ b/include/drm/drm_panel.h > @@ -133,6 +133,15 @@ struct drm_panel_funcs { > * Allows panels to create panels-specific debugfs files. > */ > void (*debugfs_init)(struct drm_panel *panel, struct dentry *root); > + > + /** > + * @get_orientation: > + * > + * Return the panel orientation set by device tree or EDID. > + * > + * This function is optional. > + */ > + enum drm_panel_orientation (*get_orientation)(struct drm_panel *panel); Please move this up so it is together with the other get_* methods, in alphabetic order. That is, right after get_modes(), and then this also matches the order in the .c file with is extra bonus. With the two fixes: Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > }; > > /** > @@ -202,6 +211,7 @@ int drm_panel_enable(struct drm_panel *panel); > int drm_panel_disable(struct drm_panel *panel); > > int drm_panel_get_modes(struct drm_panel *panel, struct drm_connector *connector); > +enum drm_panel_orientation drm_panel_get_orientation(struct drm_panel *panel); > > #if defined(CONFIG_OF) && defined(CONFIG_DRM_PANEL) > struct drm_panel *of_drm_find_panel(const struct device_node *np); > -- > 2.36.1.255.ge46751e96f-goog
Hi, On 6/6/22 21:20, Sam Ravnborg wrote: > Hi Hsin-Yi, > On Mon, Jun 06, 2022 at 11:24:24PM +0800, Hsin-Yi Wang wrote: >> Panels usually call drm_connector_set_panel_orientation(), which is >> later than drm/kms driver calling drm_dev_register(). This leads to a >> WARN(). >> >> The orientation property is known earlier. For example, some panels >> parse the property through device tree during probe. >> >> Add an API to return the property from panel to drm/kms driver, so the >> drivers are able to call drm_connector_set_panel_orientation() before >> drm_dev_register(). >> >> Suggested-by: Hans de Goede <hdegoede@redhat.com> >> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> >> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >> Reviewed-by: Douglas Anderson <dianders@chromium.org> >> --- >> v3->v4: Add a blank line. >> --- >> drivers/gpu/drm/drm_panel.c | 9 +++++++++ >> include/drm/drm_panel.h | 10 ++++++++++ >> 2 files changed, 19 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c >> index f634371c717a..e12056cfeca8 100644 >> --- a/drivers/gpu/drm/drm_panel.c >> +++ b/drivers/gpu/drm/drm_panel.c >> @@ -223,6 +223,15 @@ int drm_panel_get_modes(struct drm_panel *panel, >> } >> EXPORT_SYMBOL(drm_panel_get_modes); >> >> +enum drm_panel_orientation drm_panel_get_orientation(struct drm_panel *panel) > const as mentioned by Stephen. > >> +{ >> + if (panel && panel->funcs && panel->funcs->get_orientation) >> + return panel->funcs->get_orientation(panel); >> + >> + return DRM_MODE_PANEL_ORIENTATION_UNKNOWN; >> +} >> +EXPORT_SYMBOL(drm_panel_get_orientation); >> + >> #ifdef CONFIG_OF >> /** >> * of_drm_find_panel - look up a panel using a device tree node >> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h >> index d279ee455f01..5dadbf3b0370 100644 >> --- a/include/drm/drm_panel.h >> +++ b/include/drm/drm_panel.h >> @@ -133,6 +133,15 @@ struct drm_panel_funcs { >> * Allows panels to create panels-specific debugfs files. >> */ >> void (*debugfs_init)(struct drm_panel *panel, struct dentry *root); >> + >> + /** >> + * @get_orientation: >> + * >> + * Return the panel orientation set by device tree or EDID. >> + * >> + * This function is optional. >> + */ >> + enum drm_panel_orientation (*get_orientation)(struct drm_panel *panel); > > Please move this up so it is together with the other get_* methods, in > alphabetic order. That is, right after get_modes(), and then this also > matches the order in the .c file with is extra bonus. The downside of moving this up is that it will break drivers which don't use c99 style named-struct-field initializers for there drm_panel_funcs. I admit that no drivers should be using the old style struct init, but are we sure that that is the case? Regards, Hans > > With the two fixes: > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > >> }; >> >> /** >> @@ -202,6 +211,7 @@ int drm_panel_enable(struct drm_panel *panel); >> int drm_panel_disable(struct drm_panel *panel); >> >> int drm_panel_get_modes(struct drm_panel *panel, struct drm_connector *connector); >> +enum drm_panel_orientation drm_panel_get_orientation(struct drm_panel *panel); >> >> #if defined(CONFIG_OF) && defined(CONFIG_DRM_PANEL) >> struct drm_panel *of_drm_find_panel(const struct device_node *np); >> -- >> 2.36.1.255.ge46751e96f-goog >
Hi Hans, > > Please move this up so it is together with the other get_* methods, in > > alphabetic order. That is, right after get_modes(), and then this also > > matches the order in the .c file with is extra bonus. > > The downside of moving this up is that it will break drivers which don't > use c99 style named-struct-field initializers for there drm_panel_funcs. > > I admit that no drivers should be using the old style struct init, but > are we sure that that is the case? There is no in-tree driver that uses old style struct init for drm_panel_funcs - so we are safe here. I just did a quick git grep -A 4 drm_panel_funcs to verify it, browsing through the output did not reveal any old style users. Sam
On Tue, Jun 7, 2022 at 3:06 AM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Hsin-Yi Wang (2022-06-06 08:24:24) > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > > index f634371c717a..e12056cfeca8 100644 > > --- a/drivers/gpu/drm/drm_panel.c > > +++ b/drivers/gpu/drm/drm_panel.c > > @@ -223,6 +223,15 @@ int drm_panel_get_modes(struct drm_panel *panel, > > } > > EXPORT_SYMBOL(drm_panel_get_modes); > > > > +enum drm_panel_orientation drm_panel_get_orientation(struct drm_panel *panel) > > Should 'panel' be marked const to indicate that it can't be modified? > But .get_orientation() will need to call to_XXX_panel, eg. static inline struct boe_panel *to_boe_panel(struct drm_panel *panel) ==> .get_modes(panel) <-- can't be const. ==> drm_panel_get_orientation(panel) <-- can't be const. We can definitely cast it in the functions, or make to_XXX_panel() accept const, but I think this will lose the meaning of using const anyway. > > +{ > > + if (panel && panel->funcs && panel->funcs->get_orientation) > > + return panel->funcs->get_orientation(panel);
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index f634371c717a..e12056cfeca8 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -223,6 +223,15 @@ int drm_panel_get_modes(struct drm_panel *panel, } EXPORT_SYMBOL(drm_panel_get_modes); +enum drm_panel_orientation drm_panel_get_orientation(struct drm_panel *panel) +{ + if (panel && panel->funcs && panel->funcs->get_orientation) + return panel->funcs->get_orientation(panel); + + return DRM_MODE_PANEL_ORIENTATION_UNKNOWN; +} +EXPORT_SYMBOL(drm_panel_get_orientation); + #ifdef CONFIG_OF /** * of_drm_find_panel - look up a panel using a device tree node diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index d279ee455f01..5dadbf3b0370 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -133,6 +133,15 @@ struct drm_panel_funcs { * Allows panels to create panels-specific debugfs files. */ void (*debugfs_init)(struct drm_panel *panel, struct dentry *root); + + /** + * @get_orientation: + * + * Return the panel orientation set by device tree or EDID. + * + * This function is optional. + */ + enum drm_panel_orientation (*get_orientation)(struct drm_panel *panel); }; /** @@ -202,6 +211,7 @@ int drm_panel_enable(struct drm_panel *panel); int drm_panel_disable(struct drm_panel *panel); int drm_panel_get_modes(struct drm_panel *panel, struct drm_connector *connector); +enum drm_panel_orientation drm_panel_get_orientation(struct drm_panel *panel); #if defined(CONFIG_OF) && defined(CONFIG_DRM_PANEL) struct drm_panel *of_drm_find_panel(const struct device_node *np);