| Message ID | 20190808151150.16336-3-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Add support for bus-format negotiation | expand |
Hi Boris, Thank you for the patch. On Thu, Aug 08, 2019 at 05:11:33PM +0200, Boris Brezillon wrote: > The VC4 and Exynos DSI encoder drivers need a custom enable/disable > sequence and manually set encoder->bridge to NULL to prevent the core > from calling the bridge hooks. > > Let's provide a more official way to support custom bridge/encoder > enable/disable sequences. So, while I'm not opposed to this, I think it's a bit of a hack, and I'd like to share my vision of how I'd like to see this being implemented in the more distant future (and thus possibly on top of this change). It has been established for quite some time now that exposing drm_encoder to userspace was likely a mistake, and that it should have stayed a kernel-only object. With the generalised usage of drm_bridge, I would go one step further: drm_encoder shouldn't matter for DRM/KMS drivers at all. drm_bridge has been introduced to model chained encoders, where the second (and subsequent) encoders couldn't easily be supported in a standard and reusable way with just drm_encoder. A bridge (with the exception of the panel bridge) is just an encoder. It shouldn't matter whether encoders are internal to the display controller, separate IPs in the SoC or external components, they could all be modelled as bridges. We do have bridges for DSI encoder IPs, and DSI (and other) bridges potentially need similar custom enable/disable sequences. I would thus ideally like to see the VC4 and Exynos DSI encoders being implemented as bridges, with support for custom enable/disable sequences in bridges, and drop support for custom enable/disable sequences in drm_encoder. Does this make sense to you ? While I would love to see this being implemented right away, it may be too much work as a prerequisite for this bus format negotiation series, so I don't want to reject this patch. I would however like to already make sure we agree on the way forward for the next steps. > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 13 +++++++--- > drivers/gpu/drm/drm_crtc_helper.c | 38 ++++++++++++++++++----------- > include/drm/drm_encoder.h | 9 +++++++ > 3 files changed, 42 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 4706439fb490..15ea61877122 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1030,7 +1030,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) > * Each encoder has at most one connector (since we always steal > * it away), so we won't call disable hooks twice. > */ > - drm_atomic_bridge_disable(encoder->bridge, old_state); > + if (!encoder->custom_bridge_enable_disable_seq) > + drm_atomic_bridge_disable(encoder->bridge, old_state); > > /* Right function depends upon target state. */ > if (funcs) { > @@ -1044,7 +1045,9 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) > funcs->dpms(encoder, DRM_MODE_DPMS_OFF); > } > > - drm_atomic_bridge_post_disable(encoder->bridge, old_state); > + if (!encoder->custom_bridge_enable_disable_seq) > + drm_atomic_bridge_post_disable(encoder->bridge, > + old_state); > } > > for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { > @@ -1342,7 +1345,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, > * Each encoder has at most one connector (since we always steal > * it away), so we won't call enable hooks twice. > */ > - drm_atomic_bridge_pre_enable(encoder->bridge, old_state); > + if (!encoder->custom_bridge_enable_disable_seq) > + drm_atomic_bridge_pre_enable(encoder->bridge, old_state); > > if (funcs) { > if (funcs->atomic_enable) > @@ -1353,7 +1357,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, > funcs->commit(encoder); > } > > - drm_atomic_bridge_enable(encoder->bridge, old_state); > + if (!encoder->custom_bridge_enable_disable_seq) > + drm_atomic_bridge_enable(encoder->bridge, old_state); > } > > drm_atomic_helper_commit_writebacks(dev, old_state); > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c > index fa3694836c22..87dae37fec12 100644 > --- a/drivers/gpu/drm/drm_crtc_helper.c > +++ b/drivers/gpu/drm/drm_crtc_helper.c > @@ -160,14 +160,16 @@ drm_encoder_disable(struct drm_encoder *encoder) > if (!encoder_funcs) > return; > > - drm_bridge_disable(encoder->bridge); > + if (!encoder->custom_bridge_enable_disable_seq) > + drm_bridge_disable(encoder->bridge); > > if (encoder_funcs->disable) > (*encoder_funcs->disable)(encoder); > else if (encoder_funcs->dpms) > (*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF); > > - drm_bridge_post_disable(encoder->bridge); > + if (!encoder->custom_bridge_enable_disable_seq) > + drm_bridge_post_disable(encoder->bridge); > } > > static void __drm_helper_disable_unused_functions(struct drm_device *dev) > @@ -365,13 +367,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, > if (!encoder_funcs) > continue; > > - drm_bridge_disable(encoder->bridge); > + if (!encoder->custom_bridge_enable_disable_seq) > + drm_bridge_disable(encoder->bridge); > > /* Disable the encoders as the first thing we do. */ > if (encoder_funcs->prepare) > encoder_funcs->prepare(encoder); > > - drm_bridge_post_disable(encoder->bridge); > + if (!encoder->custom_bridge_enable_disable_seq) > + drm_bridge_post_disable(encoder->bridge); > } > > drm_crtc_prepare_encoders(dev); > @@ -414,12 +418,14 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, > if (!encoder_funcs) > continue; > > - drm_bridge_pre_enable(encoder->bridge); > + if (!encoder->custom_bridge_enable_disable_seq) > + drm_bridge_pre_enable(encoder->bridge); > > if (encoder_funcs->commit) > encoder_funcs->commit(encoder); > > - drm_bridge_enable(encoder->bridge); > + if (!encoder->custom_bridge_enable_disable_seq) > + drm_bridge_enable(encoder->bridge); > } > > /* Calculate and store various constants which > @@ -825,18 +831,22 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode) > if (!encoder_funcs) > return; > > - if (mode == DRM_MODE_DPMS_ON) > - drm_bridge_pre_enable(bridge); > - else > - drm_bridge_disable(bridge); > + if (!encoder->custom_bridge_enable_disable_seq) { > + if (mode == DRM_MODE_DPMS_ON) > + drm_bridge_pre_enable(bridge); > + else > + drm_bridge_disable(bridge); > + } > > if (encoder_funcs->dpms) > encoder_funcs->dpms(encoder, mode); > > - if (mode == DRM_MODE_DPMS_ON) > - drm_bridge_enable(bridge); > - else > - drm_bridge_post_disable(bridge); > + if (!encoder->custom_bridge_enable_disable_seq) { > + if (mode == DRM_MODE_DPMS_ON) > + drm_bridge_enable(bridge); > + else > + drm_bridge_post_disable(bridge); > + } > } > > static int drm_helper_choose_crtc_dpms(struct drm_crtc *crtc) > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h > index 70cfca03d812..d986ff1197ce 100644 > --- a/include/drm/drm_encoder.h > +++ b/include/drm/drm_encoder.h > @@ -175,6 +175,15 @@ struct drm_encoder { > struct drm_bridge *bridge; > const struct drm_encoder_funcs *funcs; > const struct drm_encoder_helper_funcs *helper_private; > + > + /** > + * @custom_bridge_enable_disable_seq: Set to true if the encoder needs > + * a custom bridge/encoder enable/disable sequence. In that case the > + * driver is responsible for calling the > + * drm[_atomic]_bridge_{pre_enable,enable,disable,post_disable}() > + * functions as part of its encoder enable/disable handling. > + */ > + uint32_t custom_bridge_enable_disable_seq : 1; > }; > > #define obj_to_encoder(x) container_of(x, struct drm_encoder, base)
On Tue, Aug 20, 2019 at 10:05:05PM +0300, Laurent Pinchart wrote: > Hi Boris, > > Thank you for the patch. > > On Thu, Aug 08, 2019 at 05:11:33PM +0200, Boris Brezillon wrote: > > The VC4 and Exynos DSI encoder drivers need a custom enable/disable > > sequence and manually set encoder->bridge to NULL to prevent the core > > from calling the bridge hooks. > > > > Let's provide a more official way to support custom bridge/encoder > > enable/disable sequences. > > So, while I'm not opposed to this, I think it's a bit of a hack, and I'd > like to share my vision of how I'd like to see this being implemented in > the more distant future (and thus possibly on top of this change). I think it's also counter to how the atomic helpers are meant to be used. I mean you're adding a pile new hooks here all for essentially not having to type a few lines of helper code. If you look around at big drivers (i915, amdgpu, nouveau) _none_ of them use the modesets_enables/disables helpers. Exactly for reasons like this where you need a custom sequence. So proper recommendation is to just type your own, you'll be happier. > It has been established for quite some time now that exposing > drm_encoder to userspace was likely a mistake, and that it should have > stayed a kernel-only object. With the generalised usage of drm_bridge, I > would go one step further: drm_encoder shouldn't matter for DRM/KMS > drivers at all. > > drm_bridge has been introduced to model chained encoders, where the > second (and subsequent) encoders couldn't easily be supported in a > standard and reusable way with just drm_encoder. A bridge (with the > exception of the panel bridge) is just an encoder. It shouldn't matter > whether encoders are internal to the display controller, separate IPs in > the SoC or external components, they could all be modelled as bridges. > We do have bridges for DSI encoder IPs, and DSI (and other) bridges > potentially need similar custom enable/disable sequences. I would thus > ideally like to see the VC4 and Exynos DSI encoders being implemented as > bridges, with support for custom enable/disable sequences in bridges, > and drop support for custom enable/disable sequences in drm_encoder. > > Does this make sense to you ? While I would love to see this being > implemented right away, it may be too much work as a prerequisite for > this bus format negotiation series, so I don't want to reject this > patch. I would however like to already make sure we agree on the way > forward for the next steps. Yeah the other bit is that bridges are supposed to be some kind of standard. I do wonder (I haven't looked at the series) whether the problem here is that encoders don't have their own full set of pre/post enable hooks like bridges (essentially that's what you're adding here), or whether the idea behind pre/post enable/disable hooks is not good enough. Anyway, this here is imo not the right thing to do. If setting encoder->bridge to NULL doesn't work for you then just type your own enable/disable implementations. There's explicitly no requirement to use all parts of atomic helpers, au contraire, it's explicitly discouraged. Cheers, Daniel > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 13 +++++++--- > > drivers/gpu/drm/drm_crtc_helper.c | 38 ++++++++++++++++++----------- > > include/drm/drm_encoder.h | 9 +++++++ > > 3 files changed, 42 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index 4706439fb490..15ea61877122 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -1030,7 +1030,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) > > * Each encoder has at most one connector (since we always steal > > * it away), so we won't call disable hooks twice. > > */ > > - drm_atomic_bridge_disable(encoder->bridge, old_state); > > + if (!encoder->custom_bridge_enable_disable_seq) > > + drm_atomic_bridge_disable(encoder->bridge, old_state); > > > > /* Right function depends upon target state. */ > > if (funcs) { > > @@ -1044,7 +1045,9 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) > > funcs->dpms(encoder, DRM_MODE_DPMS_OFF); > > } > > > > - drm_atomic_bridge_post_disable(encoder->bridge, old_state); > > + if (!encoder->custom_bridge_enable_disable_seq) > > + drm_atomic_bridge_post_disable(encoder->bridge, > > + old_state); > > } > > > > for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { > > @@ -1342,7 +1345,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, > > * Each encoder has at most one connector (since we always steal > > * it away), so we won't call enable hooks twice. > > */ > > - drm_atomic_bridge_pre_enable(encoder->bridge, old_state); > > + if (!encoder->custom_bridge_enable_disable_seq) > > + drm_atomic_bridge_pre_enable(encoder->bridge, old_state); > > > > if (funcs) { > > if (funcs->atomic_enable) > > @@ -1353,7 +1357,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, > > funcs->commit(encoder); > > } > > > > - drm_atomic_bridge_enable(encoder->bridge, old_state); > > + if (!encoder->custom_bridge_enable_disable_seq) > > + drm_atomic_bridge_enable(encoder->bridge, old_state); > > } > > > > drm_atomic_helper_commit_writebacks(dev, old_state); > > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c > > index fa3694836c22..87dae37fec12 100644 > > --- a/drivers/gpu/drm/drm_crtc_helper.c > > +++ b/drivers/gpu/drm/drm_crtc_helper.c > > @@ -160,14 +160,16 @@ drm_encoder_disable(struct drm_encoder *encoder) > > if (!encoder_funcs) > > return; > > > > - drm_bridge_disable(encoder->bridge); > > + if (!encoder->custom_bridge_enable_disable_seq) > > + drm_bridge_disable(encoder->bridge); > > > > if (encoder_funcs->disable) > > (*encoder_funcs->disable)(encoder); > > else if (encoder_funcs->dpms) > > (*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF); > > > > - drm_bridge_post_disable(encoder->bridge); > > + if (!encoder->custom_bridge_enable_disable_seq) > > + drm_bridge_post_disable(encoder->bridge); > > } > > > > static void __drm_helper_disable_unused_functions(struct drm_device *dev) > > @@ -365,13 +367,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, > > if (!encoder_funcs) > > continue; > > > > - drm_bridge_disable(encoder->bridge); > > + if (!encoder->custom_bridge_enable_disable_seq) > > + drm_bridge_disable(encoder->bridge); > > > > /* Disable the encoders as the first thing we do. */ > > if (encoder_funcs->prepare) > > encoder_funcs->prepare(encoder); > > > > - drm_bridge_post_disable(encoder->bridge); > > + if (!encoder->custom_bridge_enable_disable_seq) > > + drm_bridge_post_disable(encoder->bridge); > > } > > > > drm_crtc_prepare_encoders(dev); > > @@ -414,12 +418,14 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, > > if (!encoder_funcs) > > continue; > > > > - drm_bridge_pre_enable(encoder->bridge); > > + if (!encoder->custom_bridge_enable_disable_seq) > > + drm_bridge_pre_enable(encoder->bridge); > > > > if (encoder_funcs->commit) > > encoder_funcs->commit(encoder); > > > > - drm_bridge_enable(encoder->bridge); > > + if (!encoder->custom_bridge_enable_disable_seq) > > + drm_bridge_enable(encoder->bridge); > > } > > > > /* Calculate and store various constants which > > @@ -825,18 +831,22 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode) > > if (!encoder_funcs) > > return; > > > > - if (mode == DRM_MODE_DPMS_ON) > > - drm_bridge_pre_enable(bridge); > > - else > > - drm_bridge_disable(bridge); > > + if (!encoder->custom_bridge_enable_disable_seq) { > > + if (mode == DRM_MODE_DPMS_ON) > > + drm_bridge_pre_enable(bridge); > > + else > > + drm_bridge_disable(bridge); > > + } > > > > if (encoder_funcs->dpms) > > encoder_funcs->dpms(encoder, mode); > > > > - if (mode == DRM_MODE_DPMS_ON) > > - drm_bridge_enable(bridge); > > - else > > - drm_bridge_post_disable(bridge); > > + if (!encoder->custom_bridge_enable_disable_seq) { > > + if (mode == DRM_MODE_DPMS_ON) > > + drm_bridge_enable(bridge); > > + else > > + drm_bridge_post_disable(bridge); > > + } > > } > > > > static int drm_helper_choose_crtc_dpms(struct drm_crtc *crtc) > > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h > > index 70cfca03d812..d986ff1197ce 100644 > > --- a/include/drm/drm_encoder.h > > +++ b/include/drm/drm_encoder.h > > @@ -175,6 +175,15 @@ struct drm_encoder { > > struct drm_bridge *bridge; > > const struct drm_encoder_funcs *funcs; > > const struct drm_encoder_helper_funcs *helper_private; > > + > > + /** > > + * @custom_bridge_enable_disable_seq: Set to true if the encoder needs > > + * a custom bridge/encoder enable/disable sequence. In that case the > > + * driver is responsible for calling the > > + * drm[_atomic]_bridge_{pre_enable,enable,disable,post_disable}() > > + * functions as part of its encoder enable/disable handling. > > + */ > > + uint32_t custom_bridge_enable_disable_seq : 1; > > }; > > > > #define obj_to_encoder(x) container_of(x, struct drm_encoder, base) > > -- > Regards, > > Laurent Pinchart
Hi Daniel, On Wed, Aug 21, 2019 at 10:21:07AM +0200, Daniel Vetter wrote: > On Tue, Aug 20, 2019 at 10:05:05PM +0300, Laurent Pinchart wrote: > > On Thu, Aug 08, 2019 at 05:11:33PM +0200, Boris Brezillon wrote: > > > The VC4 and Exynos DSI encoder drivers need a custom enable/disable > > > sequence and manually set encoder->bridge to NULL to prevent the core > > > from calling the bridge hooks. > > > > > > Let's provide a more official way to support custom bridge/encoder > > > enable/disable sequences. > > > > So, while I'm not opposed to this, I think it's a bit of a hack, and I'd > > like to share my vision of how I'd like to see this being implemented in > > the more distant future (and thus possibly on top of this change). > > I think it's also counter to how the atomic helpers are meant to be used. > I mean you're adding a pile new hooks here all for essentially not having > to type a few lines of helper code. If you look around at big drivers > (i915, amdgpu, nouveau) _none_ of them use the modesets_enables/disables > helpers. Exactly for reasons like this where you need a custom sequence. > > So proper recommendation is to just type your own, you'll be happier. > > > It has been established for quite some time now that exposing > > drm_encoder to userspace was likely a mistake, and that it should have > > stayed a kernel-only object. With the generalised usage of drm_bridge, I > > would go one step further: drm_encoder shouldn't matter for DRM/KMS > > drivers at all. > > > > drm_bridge has been introduced to model chained encoders, where the > > second (and subsequent) encoders couldn't easily be supported in a > > standard and reusable way with just drm_encoder. A bridge (with the > > exception of the panel bridge) is just an encoder. It shouldn't matter > > whether encoders are internal to the display controller, separate IPs in > > the SoC or external components, they could all be modelled as bridges. > > We do have bridges for DSI encoder IPs, and DSI (and other) bridges > > potentially need similar custom enable/disable sequences. I would thus > > ideally like to see the VC4 and Exynos DSI encoders being implemented as > > bridges, with support for custom enable/disable sequences in bridges, > > and drop support for custom enable/disable sequences in drm_encoder. > > > > Does this make sense to you ? While I would love to see this being > > implemented right away, it may be too much work as a prerequisite for > > this bus format negotiation series, so I don't want to reject this > > patch. I would however like to already make sure we agree on the way > > forward for the next steps. > > Yeah the other bit is that bridges are supposed to be some kind of > standard. I do wonder (I haven't looked at the series) whether the problem > here is that encoders don't have their own full set of pre/post enable > hooks like bridges (essentially that's what you're adding here), or > whether the idea behind pre/post enable/disable hooks is not good enough. It's a mostly-good idea :-) DSI is a bit more troublesome as it often requires enable/disable sequences that are a bit more entangled between the source and the sink. That's why the omapdrm driver originally controlled enabling from sink to source, it would locate the bridge (or the omapdrm internal equivalent of bridge to be more accurate) at the end of the chain, and enable it. The bridge would then be responsible for forwarding the enable call to its source, broken in several fine-grained calls for DSI. That allowed a DSI sink to implement completely custom enable/disable sequences, and enable the source accordingly (usually by first enabling clocks, then enabling data transmission but without actual display data, and finally enabling transfer of display data). The cost was a more complex enable/disable implementation for each bridge. I think the pre/post enable/disable operations we have now are mostly good, but for some links they need to be manually controlled. Of course I can't rule out a new display bus in the future that will shatter this down to pieces by requiring more complex enable/disable sequences. > Anyway, this here is imo not the right thing to do. If setting > encoder->bridge to NULL doesn't work for you then just type your own > enable/disable implementations. There's explicitly no requirement to use > all parts of atomic helpers, au contraire, it's explicitly discouraged. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > > > --- > > > drivers/gpu/drm/drm_atomic_helper.c | 13 +++++++--- > > > drivers/gpu/drm/drm_crtc_helper.c | 38 ++++++++++++++++++----------- > > > include/drm/drm_encoder.h | 9 +++++++ > > > 3 files changed, 42 insertions(+), 18 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > > index 4706439fb490..15ea61877122 100644 > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > @@ -1030,7 +1030,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) > > > * Each encoder has at most one connector (since we always steal > > > * it away), so we won't call disable hooks twice. > > > */ > > > - drm_atomic_bridge_disable(encoder->bridge, old_state); > > > + if (!encoder->custom_bridge_enable_disable_seq) > > > + drm_atomic_bridge_disable(encoder->bridge, old_state); > > > > > > /* Right function depends upon target state. */ > > > if (funcs) { > > > @@ -1044,7 +1045,9 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) > > > funcs->dpms(encoder, DRM_MODE_DPMS_OFF); > > > } > > > > > > - drm_atomic_bridge_post_disable(encoder->bridge, old_state); > > > + if (!encoder->custom_bridge_enable_disable_seq) > > > + drm_atomic_bridge_post_disable(encoder->bridge, > > > + old_state); > > > } > > > > > > for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { > > > @@ -1342,7 +1345,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, > > > * Each encoder has at most one connector (since we always steal > > > * it away), so we won't call enable hooks twice. > > > */ > > > - drm_atomic_bridge_pre_enable(encoder->bridge, old_state); > > > + if (!encoder->custom_bridge_enable_disable_seq) > > > + drm_atomic_bridge_pre_enable(encoder->bridge, old_state); > > > > > > if (funcs) { > > > if (funcs->atomic_enable) > > > @@ -1353,7 +1357,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, > > > funcs->commit(encoder); > > > } > > > > > > - drm_atomic_bridge_enable(encoder->bridge, old_state); > > > + if (!encoder->custom_bridge_enable_disable_seq) > > > + drm_atomic_bridge_enable(encoder->bridge, old_state); > > > } > > > > > > drm_atomic_helper_commit_writebacks(dev, old_state); > > > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c > > > index fa3694836c22..87dae37fec12 100644 > > > --- a/drivers/gpu/drm/drm_crtc_helper.c > > > +++ b/drivers/gpu/drm/drm_crtc_helper.c > > > @@ -160,14 +160,16 @@ drm_encoder_disable(struct drm_encoder *encoder) > > > if (!encoder_funcs) > > > return; > > > > > > - drm_bridge_disable(encoder->bridge); > > > + if (!encoder->custom_bridge_enable_disable_seq) > > > + drm_bridge_disable(encoder->bridge); > > > > > > if (encoder_funcs->disable) > > > (*encoder_funcs->disable)(encoder); > > > else if (encoder_funcs->dpms) > > > (*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF); > > > > > > - drm_bridge_post_disable(encoder->bridge); > > > + if (!encoder->custom_bridge_enable_disable_seq) > > > + drm_bridge_post_disable(encoder->bridge); > > > } > > > > > > static void __drm_helper_disable_unused_functions(struct drm_device *dev) > > > @@ -365,13 +367,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, > > > if (!encoder_funcs) > > > continue; > > > > > > - drm_bridge_disable(encoder->bridge); > > > + if (!encoder->custom_bridge_enable_disable_seq) > > > + drm_bridge_disable(encoder->bridge); > > > > > > /* Disable the encoders as the first thing we do. */ > > > if (encoder_funcs->prepare) > > > encoder_funcs->prepare(encoder); > > > > > > - drm_bridge_post_disable(encoder->bridge); > > > + if (!encoder->custom_bridge_enable_disable_seq) > > > + drm_bridge_post_disable(encoder->bridge); > > > } > > > > > > drm_crtc_prepare_encoders(dev); > > > @@ -414,12 +418,14 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, > > > if (!encoder_funcs) > > > continue; > > > > > > - drm_bridge_pre_enable(encoder->bridge); > > > + if (!encoder->custom_bridge_enable_disable_seq) > > > + drm_bridge_pre_enable(encoder->bridge); > > > > > > if (encoder_funcs->commit) > > > encoder_funcs->commit(encoder); > > > > > > - drm_bridge_enable(encoder->bridge); > > > + if (!encoder->custom_bridge_enable_disable_seq) > > > + drm_bridge_enable(encoder->bridge); > > > } > > > > > > /* Calculate and store various constants which > > > @@ -825,18 +831,22 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode) > > > if (!encoder_funcs) > > > return; > > > > > > - if (mode == DRM_MODE_DPMS_ON) > > > - drm_bridge_pre_enable(bridge); > > > - else > > > - drm_bridge_disable(bridge); > > > + if (!encoder->custom_bridge_enable_disable_seq) { > > > + if (mode == DRM_MODE_DPMS_ON) > > > + drm_bridge_pre_enable(bridge); > > > + else > > > + drm_bridge_disable(bridge); > > > + } > > > > > > if (encoder_funcs->dpms) > > > encoder_funcs->dpms(encoder, mode); > > > > > > - if (mode == DRM_MODE_DPMS_ON) > > > - drm_bridge_enable(bridge); > > > - else > > > - drm_bridge_post_disable(bridge); > > > + if (!encoder->custom_bridge_enable_disable_seq) { > > > + if (mode == DRM_MODE_DPMS_ON) > > > + drm_bridge_enable(bridge); > > > + else > > > + drm_bridge_post_disable(bridge); > > > + } > > > } > > > > > > static int drm_helper_choose_crtc_dpms(struct drm_crtc *crtc) > > > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h > > > index 70cfca03d812..d986ff1197ce 100644 > > > --- a/include/drm/drm_encoder.h > > > +++ b/include/drm/drm_encoder.h > > > @@ -175,6 +175,15 @@ struct drm_encoder { > > > struct drm_bridge *bridge; > > > const struct drm_encoder_funcs *funcs; > > > const struct drm_encoder_helper_funcs *helper_private; > > > + > > > + /** > > > + * @custom_bridge_enable_disable_seq: Set to true if the encoder needs > > > + * a custom bridge/encoder enable/disable sequence. In that case the > > > + * driver is responsible for calling the > > > + * drm[_atomic]_bridge_{pre_enable,enable,disable,post_disable}() > > > + * functions as part of its encoder enable/disable handling. > > > + */ > > > + uint32_t custom_bridge_enable_disable_seq : 1; > > > }; > > > > > > #define obj_to_encoder(x) container_of(x, struct drm_encoder, base)
On Tue, 20 Aug 2019 22:05:05 +0300 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Boris, > > Thank you for the patch. > > On Thu, Aug 08, 2019 at 05:11:33PM +0200, Boris Brezillon wrote: > > The VC4 and Exynos DSI encoder drivers need a custom enable/disable > > sequence and manually set encoder->bridge to NULL to prevent the core > > from calling the bridge hooks. > > > > Let's provide a more official way to support custom bridge/encoder > > enable/disable sequences. > > So, while I'm not opposed to this, I think it's a bit of a hack, and I'd > like to share my vision of how I'd like to see this being implemented in > the more distant future (and thus possibly on top of this change). I agree, it definitely looks like a hack, but I find it less hack-ish than letting drivers re-initialize the encoder->bridge field to NULL just after calling bridge_attach() :P. > > It has been established for quite some time now that exposing > drm_encoder to userspace was likely a mistake, and that it should have > stayed a kernel-only object. With the generalised usage of drm_bridge, I > would go one step further: drm_encoder shouldn't matter for DRM/KMS > drivers at all. I couldn't agree more. > > drm_bridge has been introduced to model chained encoders, where the > second (and subsequent) encoders couldn't easily be supported in a > standard and reusable way with just drm_encoder. A bridge (with the > exception of the panel bridge) is just an encoder. It shouldn't matter > whether encoders are internal to the display controller, separate IPs in > the SoC or external components, they could all be modelled as bridges. > We do have bridges for DSI encoder IPs, and DSI (and other) bridges > potentially need similar custom enable/disable sequences. I would thus > ideally like to see the VC4 and Exynos DSI encoders being implemented as > bridges, with support for custom enable/disable sequences in bridges, > and drop support for custom enable/disable sequences in drm_encoder. Looks like we're on the same page. > > Does this make sense to you ? Definitely. > While I would love to see this being > implemented right away, it may be too much work as a prerequisite for > this bus format negotiation series, so I don't want to reject this > patch. I would however like to already make sure we agree on the way > forward for the next steps. Actually, I think I have something [1] that implements what you're proposing here (which is not surprising since we discussed it on IRC ;)). I'll send a v2 so you can comment on the new approach. [1]https://github.com/bbrezillon/linux-0day/commits/drm-bridge-busfmt-2
On Wed, 21 Aug 2019 10:21:07 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Aug 20, 2019 at 10:05:05PM +0300, Laurent Pinchart wrote: > > Hi Boris, > > > > Thank you for the patch. > > > > On Thu, Aug 08, 2019 at 05:11:33PM +0200, Boris Brezillon wrote: > > > The VC4 and Exynos DSI encoder drivers need a custom enable/disable > > > sequence and manually set encoder->bridge to NULL to prevent the core > > > from calling the bridge hooks. > > > > > > Let's provide a more official way to support custom bridge/encoder > > > enable/disable sequences. > > > > So, while I'm not opposed to this, I think it's a bit of a hack, and I'd > > like to share my vision of how I'd like to see this being implemented in > > the more distant future (and thus possibly on top of this change). > > I think it's also counter to how the atomic helpers are meant to be used. > I mean you're adding a pile new hooks here all for essentially not having > to type a few lines of helper code. If you look around at big drivers > (i915, amdgpu, nouveau) _none_ of them use the modesets_enables/disables > helpers. Exactly for reasons like this where you need a custom sequence. > > So proper recommendation is to just type your own, you'll be happier. > > > It has been established for quite some time now that exposing > > drm_encoder to userspace was likely a mistake, and that it should have > > stayed a kernel-only object. With the generalised usage of drm_bridge, I > > would go one step further: drm_encoder shouldn't matter for DRM/KMS > > drivers at all. > > > > drm_bridge has been introduced to model chained encoders, where the > > second (and subsequent) encoders couldn't easily be supported in a > > standard and reusable way with just drm_encoder. A bridge (with the > > exception of the panel bridge) is just an encoder. It shouldn't matter > > whether encoders are internal to the display controller, separate IPs in > > the SoC or external components, they could all be modelled as bridges. > > We do have bridges for DSI encoder IPs, and DSI (and other) bridges > > potentially need similar custom enable/disable sequences. I would thus > > ideally like to see the VC4 and Exynos DSI encoders being implemented as > > bridges, with support for custom enable/disable sequences in bridges, > > and drop support for custom enable/disable sequences in drm_encoder. > > > > Does this make sense to you ? While I would love to see this being > > implemented right away, it may be too much work as a prerequisite for > > this bus format negotiation series, so I don't want to reject this > > patch. I would however like to already make sure we agree on the way > > forward for the next steps. > > Yeah the other bit is that bridges are supposed to be some kind of > standard. I do wonder (I haven't looked at the series) whether the problem > here is that encoders don't have their own full set of pre/post enable > hooks like bridges (essentially that's what you're adding here), or > whether the idea behind pre/post enable/disable hooks is not good enough. It's just that we lack the pre/post enable hooks at the encoder level. But I'm addressing this limitation slightly differently in the v2 I'm working on.
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 4706439fb490..15ea61877122 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1030,7 +1030,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) * Each encoder has at most one connector (since we always steal * it away), so we won't call disable hooks twice. */ - drm_atomic_bridge_disable(encoder->bridge, old_state); + if (!encoder->custom_bridge_enable_disable_seq) + drm_atomic_bridge_disable(encoder->bridge, old_state); /* Right function depends upon target state. */ if (funcs) { @@ -1044,7 +1045,9 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) funcs->dpms(encoder, DRM_MODE_DPMS_OFF); } - drm_atomic_bridge_post_disable(encoder->bridge, old_state); + if (!encoder->custom_bridge_enable_disable_seq) + drm_atomic_bridge_post_disable(encoder->bridge, + old_state); } for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { @@ -1342,7 +1345,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, * Each encoder has at most one connector (since we always steal * it away), so we won't call enable hooks twice. */ - drm_atomic_bridge_pre_enable(encoder->bridge, old_state); + if (!encoder->custom_bridge_enable_disable_seq) + drm_atomic_bridge_pre_enable(encoder->bridge, old_state); if (funcs) { if (funcs->atomic_enable) @@ -1353,7 +1357,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, funcs->commit(encoder); } - drm_atomic_bridge_enable(encoder->bridge, old_state); + if (!encoder->custom_bridge_enable_disable_seq) + drm_atomic_bridge_enable(encoder->bridge, old_state); } drm_atomic_helper_commit_writebacks(dev, old_state); diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index fa3694836c22..87dae37fec12 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -160,14 +160,16 @@ drm_encoder_disable(struct drm_encoder *encoder) if (!encoder_funcs) return; - drm_bridge_disable(encoder->bridge); + if (!encoder->custom_bridge_enable_disable_seq) + drm_bridge_disable(encoder->bridge); if (encoder_funcs->disable) (*encoder_funcs->disable)(encoder); else if (encoder_funcs->dpms) (*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF); - drm_bridge_post_disable(encoder->bridge); + if (!encoder->custom_bridge_enable_disable_seq) + drm_bridge_post_disable(encoder->bridge); } static void __drm_helper_disable_unused_functions(struct drm_device *dev) @@ -365,13 +367,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, if (!encoder_funcs) continue; - drm_bridge_disable(encoder->bridge); + if (!encoder->custom_bridge_enable_disable_seq) + drm_bridge_disable(encoder->bridge); /* Disable the encoders as the first thing we do. */ if (encoder_funcs->prepare) encoder_funcs->prepare(encoder); - drm_bridge_post_disable(encoder->bridge); + if (!encoder->custom_bridge_enable_disable_seq) + drm_bridge_post_disable(encoder->bridge); } drm_crtc_prepare_encoders(dev); @@ -414,12 +418,14 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, if (!encoder_funcs) continue; - drm_bridge_pre_enable(encoder->bridge); + if (!encoder->custom_bridge_enable_disable_seq) + drm_bridge_pre_enable(encoder->bridge); if (encoder_funcs->commit) encoder_funcs->commit(encoder); - drm_bridge_enable(encoder->bridge); + if (!encoder->custom_bridge_enable_disable_seq) + drm_bridge_enable(encoder->bridge); } /* Calculate and store various constants which @@ -825,18 +831,22 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode) if (!encoder_funcs) return; - if (mode == DRM_MODE_DPMS_ON) - drm_bridge_pre_enable(bridge); - else - drm_bridge_disable(bridge); + if (!encoder->custom_bridge_enable_disable_seq) { + if (mode == DRM_MODE_DPMS_ON) + drm_bridge_pre_enable(bridge); + else + drm_bridge_disable(bridge); + } if (encoder_funcs->dpms) encoder_funcs->dpms(encoder, mode); - if (mode == DRM_MODE_DPMS_ON) - drm_bridge_enable(bridge); - else - drm_bridge_post_disable(bridge); + if (!encoder->custom_bridge_enable_disable_seq) { + if (mode == DRM_MODE_DPMS_ON) + drm_bridge_enable(bridge); + else + drm_bridge_post_disable(bridge); + } } static int drm_helper_choose_crtc_dpms(struct drm_crtc *crtc) diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h index 70cfca03d812..d986ff1197ce 100644 --- a/include/drm/drm_encoder.h +++ b/include/drm/drm_encoder.h @@ -175,6 +175,15 @@ struct drm_encoder { struct drm_bridge *bridge; const struct drm_encoder_funcs *funcs; const struct drm_encoder_helper_funcs *helper_private; + + /** + * @custom_bridge_enable_disable_seq: Set to true if the encoder needs + * a custom bridge/encoder enable/disable sequence. In that case the + * driver is responsible for calling the + * drm[_atomic]_bridge_{pre_enable,enable,disable,post_disable}() + * functions as part of its encoder enable/disable handling. + */ + uint32_t custom_bridge_enable_disable_seq : 1; }; #define obj_to_encoder(x) container_of(x, struct drm_encoder, base)
The VC4 and Exynos DSI encoder drivers need a custom enable/disable sequence and manually set encoder->bridge to NULL to prevent the core from calling the bridge hooks. Let's provide a more official way to support custom bridge/encoder enable/disable sequences. Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> --- drivers/gpu/drm/drm_atomic_helper.c | 13 +++++++--- drivers/gpu/drm/drm_crtc_helper.c | 38 ++++++++++++++++++----------- include/drm/drm_encoder.h | 9 +++++++ 3 files changed, 42 insertions(+), 18 deletions(-)