Re: [Intel-gfx] [v8, 2/4] drm/panel: set display info in panel attach
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 = &connector->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(&connector->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 = &panel->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); 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: >* ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [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 = &connector->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(&connector->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 = &panel->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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-g
Re: [Intel-gfx] [v8, 2/4] drm/panel: set display info in panel attach
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 = &connector->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(&connector->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 = &panel->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 drm_display_info struct in drm_panel (no one really weighed in on it one way or another). IMO duplicating a bunch of fields feels pretty icky. I think most fields in drm_display_info have pretty reasonable defaults, so I'd recommend just storing a copy in drm_panel. As Thierry suggests, we can populate the dt parts in probe (orientation) and then copy over all or a subset of the struct to connector on attach. Sean > > > > 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
Re: [Intel-gfx] [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 = &connector->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(&connector->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 = &panel->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 drm_display_info struct in drm_panel (no one really weighed in on > it one way or another). IMO duplicating a bunch of fields feels pretty icky. Thanks for the review. Should we duplicate the entire struct, or just create a sub-struct, say, physical_properties that will be part of drm_display_info and drm_panel? > > I think most fields in drm_display_info have pretty reasonable defaults, so > I'd > recommend just storing a copy in drm_panel. As Thierry suggests, we can > populate the dt parts in probe (orientation) and then copy over all or a > subset > of the struct to connector on attach. > > Sean > > > > > > > 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; > > > > + >
Re: [Intel-gfx] [v8, 2/4] drm/panel: set display info in panel attach
/snip > > > > > 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 drm_display_info struct in drm_panel (no one really weighed in on > > it one way or another). IMO duplicating a bunch of fields feels pretty icky. > > Thanks for the review. Should we duplicate the entire struct, or just > create a sub-struct, say, physical_properties that will be part of > drm_display_info and drm_panel? That's a good idea, but I think you can use the entire struct. Everything has reasonable default values and I think it might be difficult to draw a line in the sand as to which fields will or won't be useful for panels going forward. Best for drivers to just treat the info in drm_display_info as best effort and have good defaults IMO. Sean > > > > > I think most fields in drm_display_info have pretty reasonable defaults, so > > I'd > > recommend just storing a copy in drm_panel. As Thierry suggests, we can > > populate the dt parts in probe (orientation) and then copy over all or a > > subset > > of the struct to connector on attach. > > > > Sean > > > > > > > > > > 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 > > > > -- > > Sean Paul, Software Engineer, Google / Chromium OS -- Sean Paul, Software Engineer, Google / Chromium OS ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx