| Message ID | 20211020181901.2114645-2-sam@ravnborg.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/7] drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs | expand |
Hi Sam, Thank you for the patch. On Wed, Oct 20, 2021 at 08:18:55PM +0200, Sam Ravnborg wrote: > The atomic variants of enable/disable in drm_bridge_funcs are the > preferred operations - introduce these. > > The ps8640 driver used the non-atomic variants of the drm_bridge_chain_pre_enable/ > drm_bridge_chain_post_disable - convert these to the atomic variants. > > v2: > - Added a few more people to cc: (Jitao, Enric, Philip) to increase > possibility to get test feedback > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > Reviewed-by: Maxime Ripard <maxime@cerno.tech> > Cc: Jitao Shi <jitao.shi@mediatek.com> > Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com> > Cc: Philip Chen <philipchen@chromium.org> > Cc: Andrzej Hajda <a.hajda@samsung.com> > Cc: Neil Armstrong <narmstrong@baylibre.com> > Cc: Robert Foss <robert.foss@linaro.org> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > Cc: Jonas Karlman <jonas@kwiboo.se> > Cc: Jernej Skrabec <jernej.skrabec@gmail.com> > --- > drivers/gpu/drm/bridge/parade-ps8640.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > index 3aaa90913bf8..0b620afe99c0 100644 > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > @@ -376,7 +376,8 @@ static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge) > ps_bridge->powered = false; > } > > -static void ps8640_pre_enable(struct drm_bridge *bridge) > +static void ps8640_atomic_pre_enable(struct drm_bridge *bridge, > + struct drm_bridge_state *old_bridge_state) > { > struct ps8640 *ps_bridge = bridge_to_ps8640(bridge); > int ret; > @@ -388,7 +389,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge) > ps8640_bridge_poweroff(ps_bridge); > } > > -static void ps8640_post_disable(struct drm_bridge *bridge) > +static void ps8640_atomic_post_disable(struct drm_bridge *bridge, > + struct drm_bridge_state *old_bridge_state) > { > struct ps8640 *ps_bridge = bridge_to_ps8640(bridge); > > @@ -489,7 +491,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, > * EDID, for this chip, we need to do a full poweron, otherwise it will > * fail. > */ > - drm_bridge_chain_pre_enable(bridge); > + drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); > > edid = drm_get_edid(connector, > ps_bridge->page[PAGE0_DP_CNTL]->adapter); > @@ -499,7 +501,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, > * before, return the chip to its original power state. > */ > if (poweroff) > - drm_bridge_chain_post_disable(bridge); > + drm_atomic_bridge_chain_post_disable(bridge, connector->state->state); > > return edid; > } > @@ -508,8 +510,8 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = { > .attach = ps8640_bridge_attach, > .detach = ps8640_bridge_detach, > .get_edid = ps8640_bridge_get_edid, > - .post_disable = ps8640_post_disable, > - .pre_enable = ps8640_pre_enable, > + .atomic_post_disable = ps8640_atomic_post_disable, > + .atomic_pre_enable = ps8640_atomic_pre_enable, Don't you also need to implement .atomic_duplicate_state(), .atomic_destroy_state() and .atomic_reset() to use the atomic API ? > }; > > static int ps8640_probe(struct i2c_client *client)
Hi Laurent, > > @@ -508,8 +510,8 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = { > > .attach = ps8640_bridge_attach, > > .detach = ps8640_bridge_detach, > > .get_edid = ps8640_bridge_get_edid, > > - .post_disable = ps8640_post_disable, > > - .pre_enable = ps8640_pre_enable, > > + .atomic_post_disable = ps8640_atomic_post_disable, > > + .atomic_pre_enable = ps8640_atomic_pre_enable, > > Don't you also need to implement .atomic_duplicate_state(), > .atomic_destroy_state() and .atomic_reset() to use the atomic API ? What I think happens is that the atomic drivers uses the bridges and the implementation in drm_bridge has fall-backs to the non-atomic variants. So for example drm_atomic_bridge_chain_pre_enable() will call atomic_pre_enable() if it exists, and if not it will call pre_enable() if it exists. With this patch-set I show that the non-atomic versions of several of the drm_bridge helper functions are almost not used and easy to drop. So based on this I concluded that the bridge drivers were used in a way so we only would need to implement the atomic variants of the functions. That said I did not consider .atomic_duplicate_state(), .atomic_destroy_state(), or .atomic_reset(). From a quick look only cadence/cdns-mhdp8546 subclass drm_bridge_state and I wonder if the right thing to do would be to implement fallback to the helpers if the bridge driver do not set any of the .atomic_duplicate_state(), .atomic_destroy_state(), or .atomic_reset(). That would drop the following from a few bridges: .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, .atomic_reset = drm_atomic_helper_bridge_reset, And new bridges (or bridges we convert to use the atomic operations) would not have to specify them. I would prefer implementing the fallback - rather than adding the above to this driver. Sam
Hi Laurent, > > >From a quick look only cadence/cdns-mhdp8546 subclass > drm_bridge_state and I wonder if the right thing to do would be to > implement fallback to the helpers if the bridge driver do not set > any of the .atomic_duplicate_state(), .atomic_destroy_state(), or .atomic_reset(). > > That would drop the following from a few bridges: > .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > .atomic_reset = drm_atomic_helper_bridge_reset, To answer myself here. This would create a dependency from the core to the helpers which is not OK so idea dropped again. Sam
Hi Sam, On Fri, Oct 22, 2021 at 07:13:58PM +0200, Sam Ravnborg wrote: > Hi Laurent, > > > From a quick look only cadence/cdns-mhdp8546 subclass > > drm_bridge_state and I wonder if the right thing to do would be to > > implement fallback to the helpers if the bridge driver do not set > > any of the .atomic_duplicate_state(), .atomic_destroy_state(), or .atomic_reset(). > > > > That would drop the following from a few bridges: > > .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > > .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > > .atomic_reset = drm_atomic_helper_bridge_reset, > > To answer myself here. This would create a dependency from the core to > the helpers which is not OK so idea dropped again. I agree it would be nicer, but the dependency is likely a problem. That being said, we have multiple types of helpers. The first set is the modeset helpers, which were designed as one implementation of KMS operations, with an opt-in API for drivers. The core should not depend on those. There are however other helpers that are only default implementations of some operations, without any dependency on other components. The atomic state helpers fall in this category, they implement .atomic_* operations of the drm_*_funcs structures, not drm_*_helper_funcs. It could make sense to move them to the DRM core.
Hi Laurent, On Fri, Oct 22, 2021 at 09:42:37PM +0300, Laurent Pinchart wrote: > Hi Sam, > > On Fri, Oct 22, 2021 at 07:13:58PM +0200, Sam Ravnborg wrote: > > Hi Laurent, > > > > > From a quick look only cadence/cdns-mhdp8546 subclass > > > drm_bridge_state and I wonder if the right thing to do would be to > > > implement fallback to the helpers if the bridge driver do not set > > > any of the .atomic_duplicate_state(), .atomic_destroy_state(), or .atomic_reset(). > > > > > > That would drop the following from a few bridges: > > > .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > > > .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > > > .atomic_reset = drm_atomic_helper_bridge_reset, > > > > To answer myself here. This would create a dependency from the core to > > the helpers which is not OK so idea dropped again. > > I agree it would be nicer, but the dependency is likely a problem. That > being said, we have multiple types of helpers. The first set is the > modeset helpers, which were designed as one implementation of KMS > operations, with an opt-in API for drivers. The core should not depend > on those. There are however other helpers that are only default > implementations of some operations, without any dependency on other > components. The atomic state helpers fall in this category, they > implement .atomic_* operations of the drm_*_funcs structures, not > drm_*_helper_funcs. It could make sense to move them to the DRM core. For now I went with a simple macro: +/** + * DRM_BRIDGE_STATE_OPS - Default drm_bridge state funcs + * + * Bridge driver that do not subclass &drm_bridge_state can use the helpers + * for reset, duplicate, and destroy. This macro provides a shortcut for + * setting the helpers in the &drm_bridge_funcs structure. + */ +#define DRM_BRIDGE_STATE_OPS \ + .atomic_reset = drm_atomic_helper_bridge_reset, \ + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, \ + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state + Thomas Z. is trying to make the core smaller so pulling in these helpers would be counterproductive to that. So I took the simpler approach here which we have already done in several places. It will be part of v3 when I post it. Drop a note if you (or any other reader) have better ideas. Sam
Hi Sam, On Fri, Oct 22, 2021 at 09:32:08PM +0200, Sam Ravnborg wrote: > On Fri, Oct 22, 2021 at 09:42:37PM +0300, Laurent Pinchart wrote: > > On Fri, Oct 22, 2021 at 07:13:58PM +0200, Sam Ravnborg wrote: > > > Hi Laurent, > > > > > > > From a quick look only cadence/cdns-mhdp8546 subclass > > > > drm_bridge_state and I wonder if the right thing to do would be to > > > > implement fallback to the helpers if the bridge driver do not set > > > > any of the .atomic_duplicate_state(), .atomic_destroy_state(), or .atomic_reset(). > > > > > > > > That would drop the following from a few bridges: > > > > .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > > > > .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > > > > .atomic_reset = drm_atomic_helper_bridge_reset, > > > > > > To answer myself here. This would create a dependency from the core to > > > the helpers which is not OK so idea dropped again. > > > > I agree it would be nicer, but the dependency is likely a problem. That > > being said, we have multiple types of helpers. The first set is the > > modeset helpers, which were designed as one implementation of KMS > > operations, with an opt-in API for drivers. The core should not depend > > on those. There are however other helpers that are only default > > implementations of some operations, without any dependency on other > > components. The atomic state helpers fall in this category, they > > implement .atomic_* operations of the drm_*_funcs structures, not > > drm_*_helper_funcs. It could make sense to move them to the DRM core. > > For now I went with a simple macro: > > +/** > + * DRM_BRIDGE_STATE_OPS - Default drm_bridge state funcs > + * > + * Bridge driver that do not subclass &drm_bridge_state can use the helpers > + * for reset, duplicate, and destroy. This macro provides a shortcut for > + * setting the helpers in the &drm_bridge_funcs structure. > + */ > +#define DRM_BRIDGE_STATE_OPS \ > + .atomic_reset = drm_atomic_helper_bridge_reset, \ > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, \ > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state > + > > Thomas Z. is trying to make the core smaller so pulling in these helpers > would be counterproductive to that. So I took the simpler approach here > which we have already done in several places. Those helpers are in the same file as the other state helpers, which are used by all atomic drivers as far as I can tell, so I'm not sure we can really make anything smaller (except if we moved the bridge helpers to a separate file, but I don't think it would be worth it). > It will be part of v3 when I post it. > > Drop a note if you (or any other reader) have better ideas.
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 3aaa90913bf8..0b620afe99c0 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -376,7 +376,8 @@ static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge) ps_bridge->powered = false; } -static void ps8640_pre_enable(struct drm_bridge *bridge) +static void ps8640_atomic_pre_enable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) { struct ps8640 *ps_bridge = bridge_to_ps8640(bridge); int ret; @@ -388,7 +389,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge) ps8640_bridge_poweroff(ps_bridge); } -static void ps8640_post_disable(struct drm_bridge *bridge) +static void ps8640_atomic_post_disable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) { struct ps8640 *ps_bridge = bridge_to_ps8640(bridge); @@ -489,7 +491,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, * EDID, for this chip, we need to do a full poweron, otherwise it will * fail. */ - drm_bridge_chain_pre_enable(bridge); + drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); edid = drm_get_edid(connector, ps_bridge->page[PAGE0_DP_CNTL]->adapter); @@ -499,7 +501,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, * before, return the chip to its original power state. */ if (poweroff) - drm_bridge_chain_post_disable(bridge); + drm_atomic_bridge_chain_post_disable(bridge, connector->state->state); return edid; } @@ -508,8 +510,8 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = { .attach = ps8640_bridge_attach, .detach = ps8640_bridge_detach, .get_edid = ps8640_bridge_get_edid, - .post_disable = ps8640_post_disable, - .pre_enable = ps8640_pre_enable, + .atomic_post_disable = ps8640_atomic_post_disable, + .atomic_pre_enable = ps8640_atomic_pre_enable, }; static int ps8640_probe(struct i2c_client *client)
The atomic variants of enable/disable in drm_bridge_funcs are the preferred operations - introduce these. The ps8640 driver used the non-atomic variants of the drm_bridge_chain_pre_enable/ drm_bridge_chain_post_disable - convert these to the atomic variants. v2: - Added a few more people to cc: (Jitao, Enric, Philip) to increase possibility to get test feedback Signed-off-by: Sam Ravnborg <sam@ravnborg.org> Reviewed-by: Maxime Ripard <maxime@cerno.tech> Cc: Jitao Shi <jitao.shi@mediatek.com> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com> Cc: Philip Chen <philipchen@chromium.org> Cc: Andrzej Hajda <a.hajda@samsung.com> Cc: Neil Armstrong <narmstrong@baylibre.com> Cc: Robert Foss <robert.foss@linaro.org> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> Cc: Jonas Karlman <jonas@kwiboo.se> Cc: Jernej Skrabec <jernej.skrabec@gmail.com> --- drivers/gpu/drm/bridge/parade-ps8640.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)