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
