| Message ID | 20191227144124.210294-1-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/bridge: Fix drm_bridge_chain_pre_enable() | expand |
Hi Boris, Thank you for the patch. On Fri, Dec 27, 2019 at 03:41:22PM +0100, Boris Brezillon wrote: > Stop iterating on the bridge chain when we reach the bridge element. > That's what other helpers do and should allow bridge implementations > to execute a pre_enable operation on a sub-chain. The code looks fine to me, but I think you should update the documentation to explain this. It currently states: * Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder * chain, starting from the last bridge to the first. These are called * before calling the encoder's commit op. * * Note: the bridge passed should be the one closest to the encoder I suggest stating instead that the operation is called from the last bridge to the bridge passed as the argument. The note should then either be removed, or updated to state that bridge is usually the bridge closest to the encoder, but can be any other bridge if the caller only wants to execute the operation on a subset of the chain. It's also probably worth it updating the other functions accordingly. > Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list") > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > --- > drivers/gpu/drm/drm_bridge.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index c2cf0c90fa26..b3b269ec6a39 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -357,6 +357,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge *bridge) > list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) { > if (iter->funcs->pre_enable) > iter->funcs->pre_enable(iter); > + > + if (iter == bridge) > + break; > } > } > EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
On 06.01.2020 11:29, Laurent Pinchart wrote: > Hi Boris, > > Thank you for the patch. > > On Fri, Dec 27, 2019 at 03:41:22PM +0100, Boris Brezillon wrote: >> Stop iterating on the bridge chain when we reach the bridge element. >> That's what other helpers do and should allow bridge implementations >> to execute a pre_enable operation on a sub-chain. > The code looks fine to me, but I think you should update the > documentation to explain this. It currently states: > > * Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder > * chain, starting from the last bridge to the first. These are called > * before calling the encoder's commit op. > * > * Note: the bridge passed should be the one closest to the encoder > > I suggest stating instead that the operation is called from the last > bridge to the bridge passed as the argument. The note should then either > be removed, or updated to state that bridge is usually the bridge > closest to the encoder, but can be any other bridge if the caller only > wants to execute the operation on a subset of the chain. It's also > probably worth it updating the other functions accordingly. Apparently drm_(atomic_)bridge_chain_* helpers are always called on the 1st bridge so you can try to remove bridge argument, if it is true. Moreover after patches 2 and 3 drm_bridge_chain_* helpers have no users. Regards Andrzej > >> Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list") >> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> >> --- >> drivers/gpu/drm/drm_bridge.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c >> index c2cf0c90fa26..b3b269ec6a39 100644 >> --- a/drivers/gpu/drm/drm_bridge.c >> +++ b/drivers/gpu/drm/drm_bridge.c >> @@ -357,6 +357,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge *bridge) >> list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) { >> if (iter->funcs->pre_enable) >> iter->funcs->pre_enable(iter); >> + >> + if (iter == bridge) >> + break; >> } >> } >> EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
On Tue, 7 Jan 2020 16:27:10 +0100 Andrzej Hajda <a.hajda@samsung.com> wrote: > On 06.01.2020 11:29, Laurent Pinchart wrote: > > Hi Boris, > > > > Thank you for the patch. > > > > On Fri, Dec 27, 2019 at 03:41:22PM +0100, Boris Brezillon wrote: > >> Stop iterating on the bridge chain when we reach the bridge element. > >> That's what other helpers do and should allow bridge implementations > >> to execute a pre_enable operation on a sub-chain. > > The code looks fine to me, but I think you should update the > > documentation to explain this. It currently states: > > > > * Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder > > * chain, starting from the last bridge to the first. These are called > > * before calling the encoder's commit op. > > * > > * Note: the bridge passed should be the one closest to the encoder > > > > I suggest stating instead that the operation is called from the last > > bridge to the bridge passed as the argument. The note should then either > > be removed, or updated to state that bridge is usually the bridge > > closest to the encoder, but can be any other bridge if the caller only > > wants to execute the operation on a subset of the chain. It's also > > probably worth it updating the other functions accordingly. > > > Apparently drm_(atomic_)bridge_chain_* helpers are always called on the > 1st bridge so you can try to remove bridge argument, if it is true. You mean passing an encoder instead of a bridge? I think that's what I initially did and was told we might want to execute operations on a sub-chain at some point. > > Moreover after patches 2 and 3 drm_bridge_chain_* helpers have no users. Well, the core is still using it, but there's no external users, you're right. Do you want me to stop exporting those helpers?
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index c2cf0c90fa26..b3b269ec6a39 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -357,6 +357,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge *bridge) list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) { if (iter->funcs->pre_enable) iter->funcs->pre_enable(iter); + + if (iter == bridge) + break; } } EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
Stop iterating on the bridge chain when we reach the bridge element. That's what other helpers do and should allow bridge implementations to execute a pre_enable operation on a sub-chain. Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list") Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> --- drivers/gpu/drm/drm_bridge.c | 3 +++ 1 file changed, 3 insertions(+)