| Message ID | 20190611040350.90064-4-dbasehore@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Panel rotation patches | expand |
On Mon, Jun 10, 2019 at 09:03:48PM -0700, Derek Basehore wrote: > This adds the attach/detach callbacks. These are for setting up > internal state for the connector/panel pair that can't be done at > probe (since the connector doesn't exist) and which don't need to be > repeatedly done for every get/modes, prepare, or enable callback. > Values such as the panel orientation, and display size can be filled > in for the connector. > > Signed-off-by: Derek Basehore <dbasehore@chromium.org> > --- > drivers/gpu/drm/drm_panel.c | 14 ++++++++++++++ > include/drm/drm_panel.h | 4 ++++ > 2 files changed, 18 insertions(+) > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > index 3b689ce4a51a..72f67678d9d5 100644 > --- a/drivers/gpu/drm/drm_panel.c > +++ b/drivers/gpu/drm/drm_panel.c > @@ -104,12 +104,23 @@ EXPORT_SYMBOL(drm_panel_remove); > */ > int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector) > { > + int ret; > + > if (panel->connector) > return -EBUSY; > > panel->connector = connector; > panel->drm = connector->dev; > > + if (panel->funcs->attach) { > + ret = panel->funcs->attach(panel); > + if (ret < 0) { > + panel->connector = NULL; > + panel->drm = NULL; > + return ret; > + } > + } Why can't we just implement this in the drm helpers for everyone, by e.g. storing a dt node in drm_panel? Feels a bit overkill to have these new hooks here. Also, my understanding is that this dt stuff is supposed to be standardized, so this should work. -Daniel > + > return 0; > } > EXPORT_SYMBOL(drm_panel_attach); > @@ -128,6 +139,9 @@ EXPORT_SYMBOL(drm_panel_attach); > */ > int drm_panel_detach(struct drm_panel *panel) > { > + if (panel->funcs->detach) > + panel->funcs->detach(panel); > + > panel->connector = NULL; > panel->drm = NULL; > > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > index 13631b2efbaa..e136e3a3c996 100644 > --- a/include/drm/drm_panel.h > +++ b/include/drm/drm_panel.h > @@ -37,6 +37,8 @@ struct display_timing; > * struct drm_panel_funcs - perform operations on a given panel > * @disable: disable panel (turn off back light, etc.) > * @unprepare: turn off panel > + * @detach: detach panel->connector (clear internal state, etc.) > + * @attach: attach panel->connector (update internal state, etc.) > * @prepare: turn on panel and perform set up > * @enable: enable panel (turn on back light, etc.) > * @get_modes: add modes to the connector that the panel is attached to and > @@ -70,6 +72,8 @@ struct display_timing; > struct drm_panel_funcs { > int (*disable)(struct drm_panel *panel); > int (*unprepare)(struct drm_panel *panel); > + void (*detach)(struct drm_panel *panel); > + int (*attach)(struct drm_panel *panel); > int (*prepare)(struct drm_panel *panel); > int (*enable)(struct drm_panel *panel); > int (*get_modes)(struct drm_panel *panel); > -- > 2.22.0.rc2.383.gf4fbbf30c2-goog >
On Tue, Jun 11, 2019 at 1:57 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Mon, Jun 10, 2019 at 09:03:48PM -0700, Derek Basehore wrote: > > This adds the attach/detach callbacks. These are for setting up > > internal state for the connector/panel pair that can't be done at > > probe (since the connector doesn't exist) and which don't need to be > > repeatedly done for every get/modes, prepare, or enable callback. > > Values such as the panel orientation, and display size can be filled > > in for the connector. > > > > Signed-off-by: Derek Basehore <dbasehore@chromium.org> > > --- > > drivers/gpu/drm/drm_panel.c | 14 ++++++++++++++ > > include/drm/drm_panel.h | 4 ++++ > > 2 files changed, 18 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > > index 3b689ce4a51a..72f67678d9d5 100644 > > --- a/drivers/gpu/drm/drm_panel.c > > +++ b/drivers/gpu/drm/drm_panel.c > > @@ -104,12 +104,23 @@ EXPORT_SYMBOL(drm_panel_remove); > > */ > > int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector) > > { > > + int ret; > > + > > if (panel->connector) > > return -EBUSY; > > > > panel->connector = connector; > > panel->drm = connector->dev; > > > > + if (panel->funcs->attach) { > > + ret = panel->funcs->attach(panel); > > + if (ret < 0) { > > + panel->connector = NULL; > > + panel->drm = NULL; > > + return ret; > > + } > > + } > > Why can't we just implement this in the drm helpers for everyone, by e.g. > storing a dt node in drm_panel? Feels a bit overkill to have these new > hooks here. > > Also, my understanding is that this dt stuff is supposed to be > standardized, so this should work. So do you want all of this information added to the drm_panel struct? If we do that, we don't necessarily even need the drm helper function. We could just copy the values over here in the drm_panel_attach function (and clear them in drm_panel_detach). > -Daniel > > > + > > return 0; > > } > > EXPORT_SYMBOL(drm_panel_attach); > > @@ -128,6 +139,9 @@ EXPORT_SYMBOL(drm_panel_attach); > > */ > > int drm_panel_detach(struct drm_panel *panel) > > { > > + if (panel->funcs->detach) > > + panel->funcs->detach(panel); > > + > > panel->connector = NULL; > > panel->drm = NULL; > > > > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > > index 13631b2efbaa..e136e3a3c996 100644 > > --- a/include/drm/drm_panel.h > > +++ b/include/drm/drm_panel.h > > @@ -37,6 +37,8 @@ struct display_timing; > > * struct drm_panel_funcs - perform operations on a given panel > > * @disable: disable panel (turn off back light, etc.) > > * @unprepare: turn off panel > > + * @detach: detach panel->connector (clear internal state, etc.) > > + * @attach: attach panel->connector (update internal state, etc.) > > * @prepare: turn on panel and perform set up > > * @enable: enable panel (turn on back light, etc.) > > * @get_modes: add modes to the connector that the panel is attached to and > > @@ -70,6 +72,8 @@ struct display_timing; > > struct drm_panel_funcs { > > int (*disable)(struct drm_panel *panel); > > int (*unprepare)(struct drm_panel *panel); > > + void (*detach)(struct drm_panel *panel); > > + int (*attach)(struct drm_panel *panel); > > int (*prepare)(struct drm_panel *panel); > > int (*enable)(struct drm_panel *panel); > > int (*get_modes)(struct drm_panel *panel); > > -- > > 2.22.0.rc2.383.gf4fbbf30c2-goog > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
If we want to query the device tree outside of the panel code in helper functions, we can do this with the struct as is. There's already a device struct pointer in drm_panel, so I think we can pull from that. On Tue, Jun 11, 2019 at 5:25 PM dbasehore . <dbasehore@chromium.org> wrote: > > On Tue, Jun 11, 2019 at 1:57 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > On Mon, Jun 10, 2019 at 09:03:48PM -0700, Derek Basehore wrote: > > > This adds the attach/detach callbacks. These are for setting up > > > internal state for the connector/panel pair that can't be done at > > > probe (since the connector doesn't exist) and which don't need to be > > > repeatedly done for every get/modes, prepare, or enable callback. > > > Values such as the panel orientation, and display size can be filled > > > in for the connector. > > > > > > Signed-off-by: Derek Basehore <dbasehore@chromium.org> > > > --- > > > drivers/gpu/drm/drm_panel.c | 14 ++++++++++++++ > > > include/drm/drm_panel.h | 4 ++++ > > > 2 files changed, 18 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > > > index 3b689ce4a51a..72f67678d9d5 100644 > > > --- a/drivers/gpu/drm/drm_panel.c > > > +++ b/drivers/gpu/drm/drm_panel.c > > > @@ -104,12 +104,23 @@ EXPORT_SYMBOL(drm_panel_remove); > > > */ > > > int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector) > > > { > > > + int ret; > > > + > > > if (panel->connector) > > > return -EBUSY; > > > > > > panel->connector = connector; > > > panel->drm = connector->dev; > > > > > > + if (panel->funcs->attach) { > > > + ret = panel->funcs->attach(panel); > > > + if (ret < 0) { > > > + panel->connector = NULL; > > > + panel->drm = NULL; > > > + return ret; > > > + } > > > + } > > > > Why can't we just implement this in the drm helpers for everyone, by e.g. > > storing a dt node in drm_panel? Feels a bit overkill to have these new > > hooks here. > > > > Also, my understanding is that this dt stuff is supposed to be > > standardized, so this should work. > > So do you want all of this information added to the drm_panel struct? > If we do that, we don't necessarily even need the drm helper function. > We could just copy the values over here in the drm_panel_attach > function (and clear them in drm_panel_detach). > > > -Daniel > > > > > + > > > return 0; > > > } > > > EXPORT_SYMBOL(drm_panel_attach); > > > @@ -128,6 +139,9 @@ EXPORT_SYMBOL(drm_panel_attach); > > > */ > > > int drm_panel_detach(struct drm_panel *panel) > > > { > > > + if (panel->funcs->detach) > > > + panel->funcs->detach(panel); > > > + > > > panel->connector = NULL; > > > panel->drm = NULL; > > > > > > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > > > index 13631b2efbaa..e136e3a3c996 100644 > > > --- a/include/drm/drm_panel.h > > > +++ b/include/drm/drm_panel.h > > > @@ -37,6 +37,8 @@ struct display_timing; > > > * struct drm_panel_funcs - perform operations on a given panel > > > * @disable: disable panel (turn off back light, etc.) > > > * @unprepare: turn off panel > > > + * @detach: detach panel->connector (clear internal state, etc.) > > > + * @attach: attach panel->connector (update internal state, etc.) > > > * @prepare: turn on panel and perform set up > > > * @enable: enable panel (turn on back light, etc.) > > > * @get_modes: add modes to the connector that the panel is attached to and > > > @@ -70,6 +72,8 @@ struct display_timing; > > > struct drm_panel_funcs { > > > int (*disable)(struct drm_panel *panel); > > > int (*unprepare)(struct drm_panel *panel); > > > + void (*detach)(struct drm_panel *panel); > > > + int (*attach)(struct drm_panel *panel); > > > int (*prepare)(struct drm_panel *panel); > > > int (*enable)(struct drm_panel *panel); > > > int (*get_modes)(struct drm_panel *panel); > > > -- > > > 2.22.0.rc2.383.gf4fbbf30c2-goog > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch
On Tue, Jun 11, 2019 at 05:25:47PM -0700, dbasehore . wrote: > On Tue, Jun 11, 2019 at 1:57 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > On Mon, Jun 10, 2019 at 09:03:48PM -0700, Derek Basehore wrote: > > > This adds the attach/detach callbacks. These are for setting up > > > internal state for the connector/panel pair that can't be done at > > > probe (since the connector doesn't exist) and which don't need to be > > > repeatedly done for every get/modes, prepare, or enable callback. > > > Values such as the panel orientation, and display size can be filled > > > in for the connector. > > > > > > Signed-off-by: Derek Basehore <dbasehore@chromium.org> > > > --- > > > drivers/gpu/drm/drm_panel.c | 14 ++++++++++++++ > > > include/drm/drm_panel.h | 4 ++++ > > > 2 files changed, 18 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > > > index 3b689ce4a51a..72f67678d9d5 100644 > > > --- a/drivers/gpu/drm/drm_panel.c > > > +++ b/drivers/gpu/drm/drm_panel.c > > > @@ -104,12 +104,23 @@ EXPORT_SYMBOL(drm_panel_remove); > > > */ > > > int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector) > > > { > > > + int ret; > > > + > > > if (panel->connector) > > > return -EBUSY; > > > > > > panel->connector = connector; > > > panel->drm = connector->dev; > > > > > > + if (panel->funcs->attach) { > > > + ret = panel->funcs->attach(panel); > > > + if (ret < 0) { > > > + panel->connector = NULL; > > > + panel->drm = NULL; > > > + return ret; > > > + } > > > + } > > > > Why can't we just implement this in the drm helpers for everyone, by e.g. > > storing a dt node in drm_panel? Feels a bit overkill to have these new > > hooks here. > > > > Also, my understanding is that this dt stuff is supposed to be > > standardized, so this should work. > > So do you want all of this information added to the drm_panel struct? > If we do that, we don't necessarily even need the drm helper function. > We could just copy the values over here in the drm_panel_attach > function (and clear them in drm_panel_detach). Yeah, I think we should have all this extra information in the struct drm_panel. However, I think we need to more carefully split things such that the DT parsing happens at panel probe time. That way we can catch errors in DT, or missing entries/resources when we can still do something about it. If we start parsing DT and encounter failures, it's going to be very confusing if that's at panel attach time where code will usually just assume that everything is already validated and can't fail anymore. Thierry
On Fri, Jun 21, 2019 at 2:19 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Tue, Jun 11, 2019 at 05:25:47PM -0700, dbasehore . wrote: > > On Tue, Jun 11, 2019 at 1:57 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > On Mon, Jun 10, 2019 at 09:03:48PM -0700, Derek Basehore wrote: > > > > This adds the attach/detach callbacks. These are for setting up > > > > internal state for the connector/panel pair that can't be done at > > > > probe (since the connector doesn't exist) and which don't need to be > > > > repeatedly done for every get/modes, prepare, or enable callback. > > > > Values such as the panel orientation, and display size can be filled > > > > in for the connector. > > > > > > > > Signed-off-by: Derek Basehore <dbasehore@chromium.org> > > > > --- > > > > drivers/gpu/drm/drm_panel.c | 14 ++++++++++++++ > > > > include/drm/drm_panel.h | 4 ++++ > > > > 2 files changed, 18 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > > > > index 3b689ce4a51a..72f67678d9d5 100644 > > > > --- a/drivers/gpu/drm/drm_panel.c > > > > +++ b/drivers/gpu/drm/drm_panel.c > > > > @@ -104,12 +104,23 @@ EXPORT_SYMBOL(drm_panel_remove); > > > > */ > > > > int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector) > > > > { > > > > + int ret; > > > > + > > > > if (panel->connector) > > > > return -EBUSY; > > > > > > > > panel->connector = connector; > > > > panel->drm = connector->dev; > > > > > > > > + if (panel->funcs->attach) { > > > > + ret = panel->funcs->attach(panel); > > > > + if (ret < 0) { > > > > + panel->connector = NULL; > > > > + panel->drm = NULL; > > > > + return ret; > > > > + } > > > > + } > > > > > > Why can't we just implement this in the drm helpers for everyone, by e.g. > > > storing a dt node in drm_panel? Feels a bit overkill to have these new > > > hooks here. > > > > > > Also, my understanding is that this dt stuff is supposed to be > > > standardized, so this should work. > > > > So do you want all of this information added to the drm_panel struct? > > If we do that, we don't necessarily even need the drm helper function. > > We could just copy the values over here in the drm_panel_attach > > function (and clear them in drm_panel_detach). > > Yeah, I think we should have all this extra information in the struct > drm_panel. However, I think we need to more carefully split things such > that the DT parsing happens at panel probe time. That way we can catch > errors in DT, or missing entries/resources when we can still do > something about it. For now, I'll just put width, height, bpc, orientation, bus_flags, and bus_formats in the drm_panel struct. Those are pretty consistently set from either get_modes or prepare. The other thing those all have in common is that the values don't change. We could just add an entire drm_display_info struct inside drm_panel, but I don't know if we can just copy that over or set a pointer without breaking something else, since some of the values in the drm_display_info struct are not set by the panel (but maybe set by something else). > > If we start parsing DT and encounter failures, it's going to be very > confusing if that's at panel attach time where code will usually just > assume that everything is already validated and can't fail anymore. > > Thierry Thanks for the review
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index 3b689ce4a51a..72f67678d9d5 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -104,12 +104,23 @@ EXPORT_SYMBOL(drm_panel_remove); */ int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector) { + int ret; + if (panel->connector) return -EBUSY; panel->connector = connector; panel->drm = connector->dev; + if (panel->funcs->attach) { + ret = panel->funcs->attach(panel); + if (ret < 0) { + panel->connector = NULL; + panel->drm = NULL; + return ret; + } + } + return 0; } EXPORT_SYMBOL(drm_panel_attach); @@ -128,6 +139,9 @@ EXPORT_SYMBOL(drm_panel_attach); */ int drm_panel_detach(struct drm_panel *panel) { + if (panel->funcs->detach) + panel->funcs->detach(panel); + panel->connector = NULL; panel->drm = NULL; diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 13631b2efbaa..e136e3a3c996 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -37,6 +37,8 @@ struct display_timing; * struct drm_panel_funcs - perform operations on a given panel * @disable: disable panel (turn off back light, etc.) * @unprepare: turn off panel + * @detach: detach panel->connector (clear internal state, etc.) + * @attach: attach panel->connector (update internal state, etc.) * @prepare: turn on panel and perform set up * @enable: enable panel (turn on back light, etc.) * @get_modes: add modes to the connector that the panel is attached to and @@ -70,6 +72,8 @@ struct display_timing; struct drm_panel_funcs { int (*disable)(struct drm_panel *panel); int (*unprepare)(struct drm_panel *panel); + void (*detach)(struct drm_panel *panel); + int (*attach)(struct drm_panel *panel); int (*prepare)(struct drm_panel *panel); int (*enable)(struct drm_panel *panel); int (*get_modes)(struct drm_panel *panel);
This adds the attach/detach callbacks. These are for setting up internal state for the connector/panel pair that can't be done at probe (since the connector doesn't exist) and which don't need to be repeatedly done for every get/modes, prepare, or enable callback. Values such as the panel orientation, and display size can be filled in for the connector. Signed-off-by: Derek Basehore <dbasehore@chromium.org> --- drivers/gpu/drm/drm_panel.c | 14 ++++++++++++++ include/drm/drm_panel.h | 4 ++++ 2 files changed, 18 insertions(+)