[PATCH] drm: Add drm_bridge
Hi all, Ok I've tossed around another idea with Sean on irc, but it turned out to be too complicated for irc. So here it's in more detail. So the goal is to share bridge drivers/transcoders/whatever between different drivers without going all-in with something like cdf but allowing easy transitions. Which means all the different drm drivers should be allowed to have their own special-sauce interfaces on top of whatever drm_bridge currently provides, and encoder/bdrige drivers should still be able to share as much as possible. So for an example let's look at the sil164 chip for which we have one driver in drm/i2c/sil164_drv.c which implements the drm i2c slave encoder interface and 2 second driver in drm/i915/dvo_sil164.c which implements the i915 dvo interface. So we have two interface structures: - struct drm_encoder_slave which subclasses currently a drm_encoder. - struct intel_dvo_device which doesn't subclass anything The first mismatch is that the encoder slaves are real drm encoders, so we'd need to add a drm_bridge first somewhere. But I guess that can be hacked up (and it's kinda irrelevant for new drivers like msm). struct intel_dvo_device is conceptually like a drm_bridge, the only change would be to move most of the interface vfuncs from intel_dvo_dev_ops to the drm_bdrige. Now furthermore we have a bunch of bridge driver specific stuff like the i2c adapter, saved register state and other similar stuff. For the i2c slave encoder we keep that in struct sil164_priv, for the i915 dvo case it's splattered over intel_dvo_device and a (very tiny) struct sil164_priv. But we could unify that easily by moving e.g. the i2c_adapter from the dvo interface to the driver private. So the first important thing to note is that we won't be able to unify these different things completely because it'd be a big pile of refactoring work (so we need to be able to live with in-between states anyway). And there's always special fun like the ->get_hw_state callback i915 drivers have. So I think we need to allow such flexibility, even when ignoring how to get there. My idea to be able to pull shared drivers of in this scenario is to add a drm_bridge->driver_private void* pointer. Then we could embedde a drm_bridge into both both drm_encoder_slave and into intel_dvo_device. The unified sil164 driver would then have two init functions: One sets up a drm encoder slave, the other a dvo i915 device. Both get required setup data in their own special means (and we could easily add a new dt based setup functon, too). But thanks to the ->driver_private pointer shared code between these two interfaces doesn't need to care one bit whether it implements the dvo or the encoder slave interface. It just follows the ->driver_private pointer to get at all the relevant data it needs. Otoh this still allows special sauce functions like get_hw_state to accept the more specific interface struct and do fancy things on top. Of course that specialized stuff can't be shared, but all the common parts can. Not having a ->driver_private pointer would require that drm_bridge drivers would need to embed the drm_bridge into their own data structures. Which means that they can't support two different drm_bridge interface extensions and so would (at least partially) defeat the code sharing goals. Note that if we use drm_bridge as the baseline for implementing dsi slaves (I think this would make sense) such issues with drm_bridge interface extensions will be even more fun. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[PATCH] drm: Add drm_bridge
On Tue, Aug 13, 2013 at 3:55 PM, Sean Paul wrote: > On Tue, Aug 13, 2013 at 3:16 PM, Daniel Vetter wrote: >> Hi all, >> >> Ok I've tossed around another idea with Sean on irc, but it turned out >> to be too complicated for irc. So here it's in more detail. So the >> goal is to share bridge drivers/transcoders/whatever between different >> drivers without going all-in with something like cdf but allowing easy >> transitions. Which means all the different drm drivers should be >> allowed to have their own special-sauce interfaces on top of whatever >> drm_bridge currently provides, and encoder/bdrige drivers should still >> be able to share as much as possible. >> > > [For the benefit of those not on irc atm] This sounds good to me. I > also agree with everything in your previous email, with the exception > of those pesky helper functions and my stubborn insistence to keep > them in as long as possible. btw, I think I still like the idea of not calling enable/disable from crtc helpers (ie. let the encoder call 'em), since this way we have more flexibility with the sequencing without an explosion of pre/post variants.. mode_fixup/mode_set, etc should stay called from the crtc helpers, since the sequencing here is not really so critical. For i915 or anyone not using the helpers, they just add calls to these from wherever is appropriate in their own internal helpers. BR, -R > I'm working on a v2 patch set that will include one of our DP->LVDS > bridge drivers as a proof-of-concept. > > Sean > > > >> So for an example let's look at the sil164 chip for which we have one >> driver in drm/i2c/sil164_drv.c which implements the drm i2c slave >> encoder interface and 2 second driver in drm/i915/dvo_sil164.c which >> implements the i915 dvo interface. So we have two interface >> structures: >> - struct drm_encoder_slave which subclasses currently a drm_encoder. >> - struct intel_dvo_device which doesn't subclass anything >> >> The first mismatch is that the encoder slaves are real drm encoders, >> so we'd need to add a drm_bridge first somewhere. But I guess that can >> be hacked up (and it's kinda irrelevant for new drivers like msm). >> struct intel_dvo_device is conceptually like a drm_bridge, the only >> change would be to move most of the interface vfuncs from >> intel_dvo_dev_ops to the drm_bdrige. >> >> Now furthermore we have a bunch of bridge driver specific stuff like >> the i2c adapter, saved register state and other similar stuff. For the >> i2c slave encoder we keep that in struct sil164_priv, for the i915 dvo >> case it's splattered over intel_dvo_device and a (very tiny) struct >> sil164_priv. But we could unify that easily by moving e.g. the >> i2c_adapter from the dvo interface to the driver private. >> >> So the first important thing to note is that we won't be able to unify >> these different things completely because it'd be a big pile of >> refactoring work (so we need to be able to live with in-between states >> anyway). And there's always special fun like the ->get_hw_state >> callback i915 drivers have. So I think we need to allow such >> flexibility, even when ignoring how to get there. >> >> My idea to be able to pull shared drivers of in this scenario is to >> add a drm_bridge->driver_private void* pointer. Then we could embedde >> a drm_bridge into both both drm_encoder_slave and into >> intel_dvo_device. >> >> The unified sil164 driver would then have two init functions: One sets >> up a drm encoder slave, the other a dvo i915 device. Both get required >> setup data in their own special means (and we could easily add a new >> dt based setup functon, too). But thanks to the ->driver_private >> pointer shared code between these two interfaces doesn't need to care >> one bit whether it implements the dvo or the encoder slave interface. >> It just follows the ->driver_private pointer to get at all the >> relevant data it needs. >> >> Otoh this still allows special sauce functions like get_hw_state to >> accept the more specific interface struct and do fancy things on top. >> Of course that specialized stuff can't be shared, but all the common >> parts can. >> >> Not having a ->driver_private pointer would require that drm_bridge >> drivers would need to embed the drm_bridge into their own data >> structures. Which means that they can't support two different >> drm_bridge interface extensions and so would (at least partially) >> defeat the code sharing goals. >> >> Note that if we use drm_bridge as the baseline for implementing dsi >> slaves (I think this would make sense) such issues with drm_bridge >> interface extensions will be even more fun. >> >> Cheers, Daniel >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> +41 (0) 79 365 57 48 - http://blog.ffwll.ch > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: Add drm_bridge
On Tue, Aug 13, 2013 at 3:16 PM, Daniel Vetter wrote: > Hi all, > > Ok I've tossed around another idea with Sean on irc, but it turned out > to be too complicated for irc. So here it's in more detail. So the > goal is to share bridge drivers/transcoders/whatever between different > drivers without going all-in with something like cdf but allowing easy > transitions. Which means all the different drm drivers should be > allowed to have their own special-sauce interfaces on top of whatever > drm_bridge currently provides, and encoder/bdrige drivers should still > be able to share as much as possible. > [For the benefit of those not on irc atm] This sounds good to me. I also agree with everything in your previous email, with the exception of those pesky helper functions and my stubborn insistence to keep them in as long as possible. I'm working on a v2 patch set that will include one of our DP->LVDS bridge drivers as a proof-of-concept. Sean > So for an example let's look at the sil164 chip for which we have one > driver in drm/i2c/sil164_drv.c which implements the drm i2c slave > encoder interface and 2 second driver in drm/i915/dvo_sil164.c which > implements the i915 dvo interface. So we have two interface > structures: > - struct drm_encoder_slave which subclasses currently a drm_encoder. > - struct intel_dvo_device which doesn't subclass anything > > The first mismatch is that the encoder slaves are real drm encoders, > so we'd need to add a drm_bridge first somewhere. But I guess that can > be hacked up (and it's kinda irrelevant for new drivers like msm). > struct intel_dvo_device is conceptually like a drm_bridge, the only > change would be to move most of the interface vfuncs from > intel_dvo_dev_ops to the drm_bdrige. > > Now furthermore we have a bunch of bridge driver specific stuff like > the i2c adapter, saved register state and other similar stuff. For the > i2c slave encoder we keep that in struct sil164_priv, for the i915 dvo > case it's splattered over intel_dvo_device and a (very tiny) struct > sil164_priv. But we could unify that easily by moving e.g. the > i2c_adapter from the dvo interface to the driver private. > > So the first important thing to note is that we won't be able to unify > these different things completely because it'd be a big pile of > refactoring work (so we need to be able to live with in-between states > anyway). And there's always special fun like the ->get_hw_state > callback i915 drivers have. So I think we need to allow such > flexibility, even when ignoring how to get there. > > My idea to be able to pull shared drivers of in this scenario is to > add a drm_bridge->driver_private void* pointer. Then we could embedde > a drm_bridge into both both drm_encoder_slave and into > intel_dvo_device. > > The unified sil164 driver would then have two init functions: One sets > up a drm encoder slave, the other a dvo i915 device. Both get required > setup data in their own special means (and we could easily add a new > dt based setup functon, too). But thanks to the ->driver_private > pointer shared code between these two interfaces doesn't need to care > one bit whether it implements the dvo or the encoder slave interface. > It just follows the ->driver_private pointer to get at all the > relevant data it needs. > > Otoh this still allows special sauce functions like get_hw_state to > accept the more specific interface struct and do fancy things on top. > Of course that specialized stuff can't be shared, but all the common > parts can. > > Not having a ->driver_private pointer would require that drm_bridge > drivers would need to embed the drm_bridge into their own data > structures. Which means that they can't support two different > drm_bridge interface extensions and so would (at least partially) > defeat the code sharing goals. > > Note that if we use drm_bridge as the baseline for implementing dsi > slaves (I think this would make sense) such issues with drm_bridge > interface extensions will be even more fun. > > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Re: [PATCH] drm: Add drm_bridge
Hi all, Ok I've tossed around another idea with Sean on irc, but it turned out to be too complicated for irc. So here it's in more detail. So the goal is to share bridge drivers/transcoders/whatever between different drivers without going all-in with something like cdf but allowing easy transitions. Which means all the different drm drivers should be allowed to have their own special-sauce interfaces on top of whatever drm_bridge currently provides, and encoder/bdrige drivers should still be able to share as much as possible. So for an example let's look at the sil164 chip for which we have one driver in drm/i2c/sil164_drv.c which implements the drm i2c slave encoder interface and 2 second driver in drm/i915/dvo_sil164.c which implements the i915 dvo interface. So we have two interface structures: - struct drm_encoder_slave which subclasses currently a drm_encoder. - struct intel_dvo_device which doesn't subclass anything The first mismatch is that the encoder slaves are real drm encoders, so we'd need to add a drm_bridge first somewhere. But I guess that can be hacked up (and it's kinda irrelevant for new drivers like msm). struct intel_dvo_device is conceptually like a drm_bridge, the only change would be to move most of the interface vfuncs from intel_dvo_dev_ops to the drm_bdrige. Now furthermore we have a bunch of bridge driver specific stuff like the i2c adapter, saved register state and other similar stuff. For the i2c slave encoder we keep that in struct sil164_priv, for the i915 dvo case it's splattered over intel_dvo_device and a (very tiny) struct sil164_priv. But we could unify that easily by moving e.g. the i2c_adapter from the dvo interface to the driver private. So the first important thing to note is that we won't be able to unify these different things completely because it'd be a big pile of refactoring work (so we need to be able to live with in-between states anyway). And there's always special fun like the -get_hw_state callback i915 drivers have. So I think we need to allow such flexibility, even when ignoring how to get there. My idea to be able to pull shared drivers of in this scenario is to add a drm_bridge-driver_private void* pointer. Then we could embedde a drm_bridge into both both drm_encoder_slave and into intel_dvo_device. The unified sil164 driver would then have two init functions: One sets up a drm encoder slave, the other a dvo i915 device. Both get required setup data in their own special means (and we could easily add a new dt based setup functon, too). But thanks to the -driver_private pointer shared code between these two interfaces doesn't need to care one bit whether it implements the dvo or the encoder slave interface. It just follows the -driver_private pointer to get at all the relevant data it needs. Otoh this still allows special sauce functions like get_hw_state to accept the more specific interface struct and do fancy things on top. Of course that specialized stuff can't be shared, but all the common parts can. Not having a -driver_private pointer would require that drm_bridge drivers would need to embed the drm_bridge into their own data structures. Which means that they can't support two different drm_bridge interface extensions and so would (at least partially) defeat the code sharing goals. Note that if we use drm_bridge as the baseline for implementing dsi slaves (I think this would make sense) such issues with drm_bridge interface extensions will be even more fun. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: Add drm_bridge
On Tue, Aug 13, 2013 at 3:16 PM, Daniel Vetter dan...@ffwll.ch wrote: Hi all, Ok I've tossed around another idea with Sean on irc, but it turned out to be too complicated for irc. So here it's in more detail. So the goal is to share bridge drivers/transcoders/whatever between different drivers without going all-in with something like cdf but allowing easy transitions. Which means all the different drm drivers should be allowed to have their own special-sauce interfaces on top of whatever drm_bridge currently provides, and encoder/bdrige drivers should still be able to share as much as possible. [For the benefit of those not on irc atm] This sounds good to me. I also agree with everything in your previous email, with the exception of those pesky helper functions and my stubborn insistence to keep them in as long as possible. I'm working on a v2 patch set that will include one of our DP-LVDS bridge drivers as a proof-of-concept. Sean So for an example let's look at the sil164 chip for which we have one driver in drm/i2c/sil164_drv.c which implements the drm i2c slave encoder interface and 2 second driver in drm/i915/dvo_sil164.c which implements the i915 dvo interface. So we have two interface structures: - struct drm_encoder_slave which subclasses currently a drm_encoder. - struct intel_dvo_device which doesn't subclass anything The first mismatch is that the encoder slaves are real drm encoders, so we'd need to add a drm_bridge first somewhere. But I guess that can be hacked up (and it's kinda irrelevant for new drivers like msm). struct intel_dvo_device is conceptually like a drm_bridge, the only change would be to move most of the interface vfuncs from intel_dvo_dev_ops to the drm_bdrige. Now furthermore we have a bunch of bridge driver specific stuff like the i2c adapter, saved register state and other similar stuff. For the i2c slave encoder we keep that in struct sil164_priv, for the i915 dvo case it's splattered over intel_dvo_device and a (very tiny) struct sil164_priv. But we could unify that easily by moving e.g. the i2c_adapter from the dvo interface to the driver private. So the first important thing to note is that we won't be able to unify these different things completely because it'd be a big pile of refactoring work (so we need to be able to live with in-between states anyway). And there's always special fun like the -get_hw_state callback i915 drivers have. So I think we need to allow such flexibility, even when ignoring how to get there. My idea to be able to pull shared drivers of in this scenario is to add a drm_bridge-driver_private void* pointer. Then we could embedde a drm_bridge into both both drm_encoder_slave and into intel_dvo_device. The unified sil164 driver would then have two init functions: One sets up a drm encoder slave, the other a dvo i915 device. Both get required setup data in their own special means (and we could easily add a new dt based setup functon, too). But thanks to the -driver_private pointer shared code between these two interfaces doesn't need to care one bit whether it implements the dvo or the encoder slave interface. It just follows the -driver_private pointer to get at all the relevant data it needs. Otoh this still allows special sauce functions like get_hw_state to accept the more specific interface struct and do fancy things on top. Of course that specialized stuff can't be shared, but all the common parts can. Not having a -driver_private pointer would require that drm_bridge drivers would need to embed the drm_bridge into their own data structures. Which means that they can't support two different drm_bridge interface extensions and so would (at least partially) defeat the code sharing goals. Note that if we use drm_bridge as the baseline for implementing dsi slaves (I think this would make sense) such issues with drm_bridge interface extensions will be even more fun. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: Add drm_bridge
On Fri, Aug 09, 2013 at 09:19:22AM -0400, Sean Paul wrote: > On Thu, Aug 8, 2013 at 8:36 PM, Daniel Vetter wrote: > > On Thu, Aug 8, 2013 at 11:03 PM, Sean Paul wrote: > >> This patch adds the notion of a drm_bridge. A bridge is a chained > >> device which hangs off an encoder. The drm driver using the bridge > >> should provide the association between encoder and bridge. Once a > >> bridge is associated with an encoder, it will participate in mode > >> set, dpms, and optionally connection detection. > >> > >> Since a connector may not be able to determine the connection state > >> downstream of the bridge, bridges may implement detect() which will take > >> precedence over connector detect() if the bridge is already attached > >> to an encoder and that encoder is already attached to the connector. > >> In practical terms, this requires the drm driver to make these > >> associations at init time, which is fine for SoC applications where > >> this link is static. > >> > >> Signed-off-by: Sean Paul > > > > A few comments below. I think overall any such infrastructure simply > > needs to demonstrate viability on a few drivers, that makes it much > > simpler to check whether the interfaces make sense. > > Thanks for your feedback, Daniel. > > As mentioned by Stephane and Rob, there are a few drivers already > using this. In the ChromeOS tree, we have 2 simple DP->LVDS bridges > which quite frankly aren't terribly interesting atm, and we'll be > converting an HDMI->myDP bridge in a couple of weeks (time permitting, > of course). > > > > Generally I'd make > > more sense for me if the bridge is just an implementation detail of a > > given encoder and if the bridge would not be exposed to the crtc > > helpers. > > After we discussed this on IRC, I converted our exynos driver to do > this. You can check out the patch here > (https://gerrit.chromium.org/gerrit/#/c/64680). Hm, looking at this (I didn't read the full thing) it smells a bit like exynos registers the connector first eg as hdmi. And then it sets up the hdmi->whatever bridge later on. Imo (and presuming my 5 minutes strolling around guess is right) that's the wrong approach since the connector is the userspace interface object and should represent the actual output port. So if you have a pile of conversion bridges that's irrelevant and the user doesn't really care that the first step is a hmdi connetion if it eventually ends up on a lvds panel (or something like that). Instead the connector should be registered by the last bridge, with the correct type. > It felt like a lot of boilerplate code that would be duplicated in > every drm driver. Given the number of hooks we have, it's a pretty > fine granularity such that there aren't *that* many creative ways to > order things (famous last words). If a driver finds the need to differ > from the helper order, they probably aren't going to be using the > helper anyways. I think you haven't seen some of the fun stuff we've done in i915 before we've given up and scrapped it all ;-) But the current interface look sensible enough that we can fix things by moving it back into drivers again (and just leaving the the generic encoder->bridge pointer NULL). > > With that abstraction chaining would be more natural imo and > > we'd also have a much higher chance that bridge drivers work accross > > different drm drivers: Atm you can run some encoder stuff in the crtc > > callbacks and the other way round (and pretty much every driver for a > > bit more complicated hw does that), and every driver can differ in > > those tricks a bit. If we bake the bridge callbacks into the crtc > > helpers I expect that for bridges with tricky ordering constraints > > this will become a giant mess. So I'd prefer much if this would work > > like drm i2c slave encoders. > > > > In the three bridge chips I've dealt with, they all try their best to > be transparent to the encoder. They all fail at this. However their > quirks are usually not dependent on the encoder implementation, but > rather the general order of operations (ie: when to assert hotplug, > video signal on, etc.). > > Furthermore, drm_bridge will allow us to use bridges across drm > drivers. For example, the HDMI->myDP driver I mentioned earlier is > used on a number of platforms, not just exynos. It would be a waste to > bind it to exynos when a more general implementation can be achieved. I guess it's best to just look a bit at actual implementations, without them my comments are likely rather useless. > >> diff --git a/drivers/gpu/drm/drm_crtc_helper.c > >> b/drivers/gpu/drm/drm_crtc_helper.c > >> index 6a64749..30139fe 100644 > >> --- a/drivers/gpu/drm/drm_crtc_helper.c > >> +++ b/drivers/gpu/drm/drm_crtc_helper.c > >> @@ -71,6 +71,23 @@ EXPORT_SYMBOL(drm_helper_move_panel_connectors_to_head); > >> static bool drm_kms_helper_poll = true; > >> module_param_named(poll, drm_kms_helper_poll, bool, 0600); > >> > >> +static enum drm_connector_status
[PATCH] drm: Add drm_bridge
On Fri, Aug 9, 2013 at 9:19 AM, Sean Paul wrote: > On Thu, Aug 8, 2013 at 8:36 PM, Daniel Vetter wrote: >> On Thu, Aug 8, 2013 at 11:03 PM, Sean Paul wrote: >>> This patch adds the notion of a drm_bridge. A bridge is a chained >>> device which hangs off an encoder. The drm driver using the bridge >>> should provide the association between encoder and bridge. Once a >>> bridge is associated with an encoder, it will participate in mode >>> set, dpms, and optionally connection detection. >>> >>> Since a connector may not be able to determine the connection state >>> downstream of the bridge, bridges may implement detect() which will take >>> precedence over connector detect() if the bridge is already attached >>> to an encoder and that encoder is already attached to the connector. >>> In practical terms, this requires the drm driver to make these >>> associations at init time, which is fine for SoC applications where >>> this link is static. >>> >>> Signed-off-by: Sean Paul >> >> A few comments below. I think overall any such infrastructure simply >> needs to demonstrate viability on a few drivers, that makes it much >> simpler to check whether the interfaces make sense. > > Thanks for your feedback, Daniel. > > As mentioned by Stephane and Rob, there are a few drivers already > using this. In the ChromeOS tree, we have 2 simple DP->LVDS bridges > which quite frankly aren't terribly interesting atm, and we'll be > converting an HDMI->myDP bridge in a couple of weeks (time permitting, > of course). radeon also uses DP->LVDS and DP->VGA bridges on a number of systems. It currently uses a driver specific hack, but it could probably be converted to use the bridge infrastructure. Alex
[PATCH] drm: Add drm_bridge
On Thu, Aug 8, 2013 at 8:36 PM, Daniel Vetter wrote:
> On Thu, Aug 8, 2013 at 11:03 PM, Sean Paul wrote:
>> This patch adds the notion of a drm_bridge. A bridge is a chained
>> device which hangs off an encoder. The drm driver using the bridge
>> should provide the association between encoder and bridge. Once a
>> bridge is associated with an encoder, it will participate in mode
>> set, dpms, and optionally connection detection.
>>
>> Since a connector may not be able to determine the connection state
>> downstream of the bridge, bridges may implement detect() which will take
>> precedence over connector detect() if the bridge is already attached
>> to an encoder and that encoder is already attached to the connector.
>> In practical terms, this requires the drm driver to make these
>> associations at init time, which is fine for SoC applications where
>> this link is static.
>>
>> Signed-off-by: Sean Paul
>
> A few comments below. I think overall any such infrastructure simply
> needs to demonstrate viability on a few drivers, that makes it much
> simpler to check whether the interfaces make sense.
Thanks for your feedback, Daniel.
As mentioned by Stephane and Rob, there are a few drivers already
using this. In the ChromeOS tree, we have 2 simple DP->LVDS bridges
which quite frankly aren't terribly interesting atm, and we'll be
converting an HDMI->myDP bridge in a couple of weeks (time permitting,
of course).
> Generally I'd make
> more sense for me if the bridge is just an implementation detail of a
> given encoder and if the bridge would not be exposed to the crtc
> helpers.
After we discussed this on IRC, I converted our exynos driver to do
this. You can check out the patch here
(https://gerrit.chromium.org/gerrit/#/c/64680).
It felt like a lot of boilerplate code that would be duplicated in
every drm driver. Given the number of hooks we have, it's a pretty
fine granularity such that there aren't *that* many creative ways to
order things (famous last words). If a driver finds the need to differ
from the helper order, they probably aren't going to be using the
helper anyways.
> With that abstraction chaining would be more natural imo and
> we'd also have a much higher chance that bridge drivers work accross
> different drm drivers: Atm you can run some encoder stuff in the crtc
> callbacks and the other way round (and pretty much every driver for a
> bit more complicated hw does that), and every driver can differ in
> those tricks a bit. If we bake the bridge callbacks into the crtc
> helpers I expect that for bridges with tricky ordering constraints
> this will become a giant mess. So I'd prefer much if this would work
> like drm i2c slave encoders.
>
In the three bridge chips I've dealt with, they all try their best to
be transparent to the encoder. They all fail at this. However their
quirks are usually not dependent on the encoder implementation, but
rather the general order of operations (ie: when to assert hotplug,
video signal on, etc.).
Furthermore, drm_bridge will allow us to use bridges across drm
drivers. For example, the HDMI->myDP driver I mentioned earlier is
used on a number of platforms, not just exynos. It would be a waste to
bind it to exynos when a more general implementation can be achieved.
> Cheers, Daniel
>
>> ---
>> drivers/gpu/drm/drm_crtc.c| 50 +
>> drivers/gpu/drm/drm_crtc_helper.c | 112
>> ++
>> drivers/gpu/drm/drm_sysfs.c | 8 ++-
>> include/drm/drm_crtc.h| 48
>> include/drm/drm_crtc_helper.h | 34
>> 5 files changed, 241 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index fc83bb9..0311e2b 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -781,6 +781,41 @@ void drm_connector_unplug_all(struct drm_device *dev)
>> }
>> EXPORT_SYMBOL(drm_connector_unplug_all);
>>
>> +int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
>> + const struct drm_bridge_funcs *funcs)
>> +{
>> + int ret;
>> +
>> + drm_modeset_lock_all(dev);
>> +
>> + ret = drm_mode_object_get(dev, >base,
>> DRM_MODE_OBJECT_BRIDGE);
>> + if (ret)
>> + goto out;
>> +
>> + bridge->dev = dev;
>> + bridge->funcs = funcs;
>> +
>> + list_add_tail(>head, >mode_config.bridge_list);
>> + dev->mode_config.num_bridge++;
>> +
>> + out:
>> + drm_modeset_unlock_all(dev);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(drm_bridge_init);
>> +
>> +void drm_bridge_cleanup(struct drm_bridge *bridge)
>> +{
>> + struct drm_device *dev = bridge->dev;
>> +
>> + drm_modeset_lock_all(dev);
>> + drm_mode_object_put(dev, >base);
>> + list_del(>head);
>> + dev->mode_config.num_bridge--;
>> + drm_modeset_unlock_all(dev);
>> +}
>> +EXPORT_SYMBOL(drm_bridge_cleanup);
>> +
>> int
[PATCH] drm: Add drm_bridge
On Thu, Aug 8, 2013 at 11:03 PM, Sean Paul wrote:
> This patch adds the notion of a drm_bridge. A bridge is a chained
> device which hangs off an encoder. The drm driver using the bridge
> should provide the association between encoder and bridge. Once a
> bridge is associated with an encoder, it will participate in mode
> set, dpms, and optionally connection detection.
>
> Since a connector may not be able to determine the connection state
> downstream of the bridge, bridges may implement detect() which will take
> precedence over connector detect() if the bridge is already attached
> to an encoder and that encoder is already attached to the connector.
> In practical terms, this requires the drm driver to make these
> associations at init time, which is fine for SoC applications where
> this link is static.
>
> Signed-off-by: Sean Paul
A few comments below. I think overall any such infrastructure simply
needs to demonstrate viability on a few drivers, that makes it much
simpler to check whether the interfaces make sense. Generally I'd make
more sense for me if the bridge is just an implementation detail of a
given encoder and if the bridge would not be exposed to the crtc
helpers. With that abstraction chaining would be more natural imo and
we'd also have a much higher chance that bridge drivers work accross
different drm drivers: Atm you can run some encoder stuff in the crtc
callbacks and the other way round (and pretty much every driver for a
bit more complicated hw does that), and every driver can differ in
those tricks a bit. If we bake the bridge callbacks into the crtc
helpers I expect that for bridges with tricky ordering constraints
this will become a giant mess. So I'd prefer much if this would work
like drm i2c slave encoders.
Cheers, Daniel
> ---
> drivers/gpu/drm/drm_crtc.c| 50 +
> drivers/gpu/drm/drm_crtc_helper.c | 112
> ++
> drivers/gpu/drm/drm_sysfs.c | 8 ++-
> include/drm/drm_crtc.h| 48
> include/drm/drm_crtc_helper.h | 34
> 5 files changed, 241 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index fc83bb9..0311e2b 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -781,6 +781,41 @@ void drm_connector_unplug_all(struct drm_device *dev)
> }
> EXPORT_SYMBOL(drm_connector_unplug_all);
>
> +int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
> + const struct drm_bridge_funcs *funcs)
> +{
> + int ret;
> +
> + drm_modeset_lock_all(dev);
> +
> + ret = drm_mode_object_get(dev, >base, DRM_MODE_OBJECT_BRIDGE);
> + if (ret)
> + goto out;
> +
> + bridge->dev = dev;
> + bridge->funcs = funcs;
> +
> + list_add_tail(>head, >mode_config.bridge_list);
> + dev->mode_config.num_bridge++;
> +
> + out:
> + drm_modeset_unlock_all(dev);
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_bridge_init);
> +
> +void drm_bridge_cleanup(struct drm_bridge *bridge)
> +{
> + struct drm_device *dev = bridge->dev;
> +
> + drm_modeset_lock_all(dev);
> + drm_mode_object_put(dev, >base);
> + list_del(>head);
> + dev->mode_config.num_bridge--;
> + drm_modeset_unlock_all(dev);
> +}
> +EXPORT_SYMBOL(drm_bridge_cleanup);
> +
> int drm_encoder_init(struct drm_device *dev,
> struct drm_encoder *encoder,
> const struct drm_encoder_funcs *funcs,
> @@ -1190,6 +1225,7 @@ static int drm_mode_group_init(struct drm_device *dev,
> struct drm_mode_group *gr
> total_objects += dev->mode_config.num_crtc;
> total_objects += dev->mode_config.num_connector;
> total_objects += dev->mode_config.num_encoder;
> + total_objects += dev->mode_config.num_bridge;
>
> group->id_list = kzalloc(total_objects * sizeof(uint32_t),
> GFP_KERNEL);
> if (!group->id_list)
> @@ -1198,6 +1234,7 @@ static int drm_mode_group_init(struct drm_device *dev,
> struct drm_mode_group *gr
> group->num_crtcs = 0;
> group->num_connectors = 0;
> group->num_encoders = 0;
> + group->num_bridges = 0;
> return 0;
> }
>
> @@ -1207,6 +1244,7 @@ int drm_mode_group_init_legacy_group(struct drm_device
> *dev,
> struct drm_crtc *crtc;
> struct drm_encoder *encoder;
> struct drm_connector *connector;
> + struct drm_bridge *bridge;
> int ret;
>
> if ((ret = drm_mode_group_init(dev, group)))
> @@ -1223,6 +1261,11 @@ int drm_mode_group_init_legacy_group(struct drm_device
> *dev,
> group->id_list[group->num_crtcs + group->num_encoders +
>group->num_connectors++] = connector->base.id;
>
> + list_for_each_entry(bridge, >mode_config.bridge_list, head)
> + group->id_list[group->num_crtcs +
Re: [PATCH] drm: Add drm_bridge
On Thu, Aug 8, 2013 at 8:36 PM, Daniel Vetter dan...@ffwll.ch wrote:
On Thu, Aug 8, 2013 at 11:03 PM, Sean Paul seanp...@chromium.org wrote:
This patch adds the notion of a drm_bridge. A bridge is a chained
device which hangs off an encoder. The drm driver using the bridge
should provide the association between encoder and bridge. Once a
bridge is associated with an encoder, it will participate in mode
set, dpms, and optionally connection detection.
Since a connector may not be able to determine the connection state
downstream of the bridge, bridges may implement detect() which will take
precedence over connector detect() if the bridge is already attached
to an encoder and that encoder is already attached to the connector.
In practical terms, this requires the drm driver to make these
associations at init time, which is fine for SoC applications where
this link is static.
Signed-off-by: Sean Paul seanp...@chromium.org
A few comments below. I think overall any such infrastructure simply
needs to demonstrate viability on a few drivers, that makes it much
simpler to check whether the interfaces make sense.
Thanks for your feedback, Daniel.
As mentioned by Stephane and Rob, there are a few drivers already
using this. In the ChromeOS tree, we have 2 simple DP-LVDS bridges
which quite frankly aren't terribly interesting atm, and we'll be
converting an HDMI-myDP bridge in a couple of weeks (time permitting,
of course).
Generally I'd make
more sense for me if the bridge is just an implementation detail of a
given encoder and if the bridge would not be exposed to the crtc
helpers.
After we discussed this on IRC, I converted our exynos driver to do
this. You can check out the patch here
(https://gerrit.chromium.org/gerrit/#/c/64680).
It felt like a lot of boilerplate code that would be duplicated in
every drm driver. Given the number of hooks we have, it's a pretty
fine granularity such that there aren't *that* many creative ways to
order things (famous last words). If a driver finds the need to differ
from the helper order, they probably aren't going to be using the
helper anyways.
With that abstraction chaining would be more natural imo and
we'd also have a much higher chance that bridge drivers work accross
different drm drivers: Atm you can run some encoder stuff in the crtc
callbacks and the other way round (and pretty much every driver for a
bit more complicated hw does that), and every driver can differ in
those tricks a bit. If we bake the bridge callbacks into the crtc
helpers I expect that for bridges with tricky ordering constraints
this will become a giant mess. So I'd prefer much if this would work
like drm i2c slave encoders.
In the three bridge chips I've dealt with, they all try their best to
be transparent to the encoder. They all fail at this. However their
quirks are usually not dependent on the encoder implementation, but
rather the general order of operations (ie: when to assert hotplug,
video signal on, etc.).
Furthermore, drm_bridge will allow us to use bridges across drm
drivers. For example, the HDMI-myDP driver I mentioned earlier is
used on a number of platforms, not just exynos. It would be a waste to
bind it to exynos when a more general implementation can be achieved.
Cheers, Daniel
---
drivers/gpu/drm/drm_crtc.c| 50 +
drivers/gpu/drm/drm_crtc_helper.c | 112
++
drivers/gpu/drm/drm_sysfs.c | 8 ++-
include/drm/drm_crtc.h| 48
include/drm/drm_crtc_helper.h | 34
5 files changed, 241 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index fc83bb9..0311e2b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -781,6 +781,41 @@ void drm_connector_unplug_all(struct drm_device *dev)
}
EXPORT_SYMBOL(drm_connector_unplug_all);
+int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
+ const struct drm_bridge_funcs *funcs)
+{
+ int ret;
+
+ drm_modeset_lock_all(dev);
+
+ ret = drm_mode_object_get(dev, bridge-base,
DRM_MODE_OBJECT_BRIDGE);
+ if (ret)
+ goto out;
+
+ bridge-dev = dev;
+ bridge-funcs = funcs;
+
+ list_add_tail(bridge-head, dev-mode_config.bridge_list);
+ dev-mode_config.num_bridge++;
+
+ out:
+ drm_modeset_unlock_all(dev);
+ return ret;
+}
+EXPORT_SYMBOL(drm_bridge_init);
+
+void drm_bridge_cleanup(struct drm_bridge *bridge)
+{
+ struct drm_device *dev = bridge-dev;
+
+ drm_modeset_lock_all(dev);
+ drm_mode_object_put(dev, bridge-base);
+ list_del(bridge-head);
+ dev-mode_config.num_bridge--;
+ drm_modeset_unlock_all(dev);
+}
+EXPORT_SYMBOL(drm_bridge_cleanup);
+
int drm_encoder_init(struct drm_device *dev,
struct drm_encoder
Re: [PATCH] drm: Add drm_bridge
On Fri, Aug 9, 2013 at 9:19 AM, Sean Paul seanp...@chromium.org wrote: On Thu, Aug 8, 2013 at 8:36 PM, Daniel Vetter dan...@ffwll.ch wrote: On Thu, Aug 8, 2013 at 11:03 PM, Sean Paul seanp...@chromium.org wrote: This patch adds the notion of a drm_bridge. A bridge is a chained device which hangs off an encoder. The drm driver using the bridge should provide the association between encoder and bridge. Once a bridge is associated with an encoder, it will participate in mode set, dpms, and optionally connection detection. Since a connector may not be able to determine the connection state downstream of the bridge, bridges may implement detect() which will take precedence over connector detect() if the bridge is already attached to an encoder and that encoder is already attached to the connector. In practical terms, this requires the drm driver to make these associations at init time, which is fine for SoC applications where this link is static. Signed-off-by: Sean Paul seanp...@chromium.org A few comments below. I think overall any such infrastructure simply needs to demonstrate viability on a few drivers, that makes it much simpler to check whether the interfaces make sense. Thanks for your feedback, Daniel. As mentioned by Stephane and Rob, there are a few drivers already using this. In the ChromeOS tree, we have 2 simple DP-LVDS bridges which quite frankly aren't terribly interesting atm, and we'll be converting an HDMI-myDP bridge in a couple of weeks (time permitting, of course). radeon also uses DP-LVDS and DP-VGA bridges on a number of systems. It currently uses a driver specific hack, but it could probably be converted to use the bridge infrastructure. Alex ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: Add drm_bridge
On Fri, Aug 09, 2013 at 09:19:22AM -0400, Sean Paul wrote:
On Thu, Aug 8, 2013 at 8:36 PM, Daniel Vetter dan...@ffwll.ch wrote:
On Thu, Aug 8, 2013 at 11:03 PM, Sean Paul seanp...@chromium.org wrote:
This patch adds the notion of a drm_bridge. A bridge is a chained
device which hangs off an encoder. The drm driver using the bridge
should provide the association between encoder and bridge. Once a
bridge is associated with an encoder, it will participate in mode
set, dpms, and optionally connection detection.
Since a connector may not be able to determine the connection state
downstream of the bridge, bridges may implement detect() which will take
precedence over connector detect() if the bridge is already attached
to an encoder and that encoder is already attached to the connector.
In practical terms, this requires the drm driver to make these
associations at init time, which is fine for SoC applications where
this link is static.
Signed-off-by: Sean Paul seanp...@chromium.org
A few comments below. I think overall any such infrastructure simply
needs to demonstrate viability on a few drivers, that makes it much
simpler to check whether the interfaces make sense.
Thanks for your feedback, Daniel.
As mentioned by Stephane and Rob, there are a few drivers already
using this. In the ChromeOS tree, we have 2 simple DP-LVDS bridges
which quite frankly aren't terribly interesting atm, and we'll be
converting an HDMI-myDP bridge in a couple of weeks (time permitting,
of course).
Generally I'd make
more sense for me if the bridge is just an implementation detail of a
given encoder and if the bridge would not be exposed to the crtc
helpers.
After we discussed this on IRC, I converted our exynos driver to do
this. You can check out the patch here
(https://gerrit.chromium.org/gerrit/#/c/64680).
Hm, looking at this (I didn't read the full thing) it smells a bit like
exynos registers the connector first eg as hdmi. And then it sets up the
hdmi-whatever bridge later on. Imo (and presuming my 5 minutes strolling
around guess is right) that's the wrong approach since the connector is
the userspace interface object and should represent the actual output
port. So if you have a pile of conversion bridges that's irrelevant and
the user doesn't really care that the first step is a hmdi connetion if it
eventually ends up on a lvds panel (or something like that). Instead the
connector should be registered by the last bridge, with the correct type.
It felt like a lot of boilerplate code that would be duplicated in
every drm driver. Given the number of hooks we have, it's a pretty
fine granularity such that there aren't *that* many creative ways to
order things (famous last words). If a driver finds the need to differ
from the helper order, they probably aren't going to be using the
helper anyways.
I think you haven't seen some of the fun stuff we've done in i915 before
we've given up and scrapped it all ;-) But the current interface look
sensible enough that we can fix things by moving it back into drivers
again (and just leaving the the generic encoder-bridge pointer NULL).
With that abstraction chaining would be more natural imo and
we'd also have a much higher chance that bridge drivers work accross
different drm drivers: Atm you can run some encoder stuff in the crtc
callbacks and the other way round (and pretty much every driver for a
bit more complicated hw does that), and every driver can differ in
those tricks a bit. If we bake the bridge callbacks into the crtc
helpers I expect that for bridges with tricky ordering constraints
this will become a giant mess. So I'd prefer much if this would work
like drm i2c slave encoders.
In the three bridge chips I've dealt with, they all try their best to
be transparent to the encoder. They all fail at this. However their
quirks are usually not dependent on the encoder implementation, but
rather the general order of operations (ie: when to assert hotplug,
video signal on, etc.).
Furthermore, drm_bridge will allow us to use bridges across drm
drivers. For example, the HDMI-myDP driver I mentioned earlier is
used on a number of platforms, not just exynos. It would be a waste to
bind it to exynos when a more general implementation can be achieved.
I guess it's best to just look a bit at actual implementations, without
them my comments are likely rather useless.
diff --git a/drivers/gpu/drm/drm_crtc_helper.c
b/drivers/gpu/drm/drm_crtc_helper.c
index 6a64749..30139fe 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -71,6 +71,23 @@ EXPORT_SYMBOL(drm_helper_move_panel_connectors_to_head);
static bool drm_kms_helper_poll = true;
module_param_named(poll, drm_kms_helper_poll, bool, 0600);
+static enum drm_connector_status detect_connection(
+ struct drm_connector *connector, bool force)
+{
+
[PATCH] drm: Add drm_bridge
On Thu, Aug 8, 2013 at 8:36 PM, Daniel Vetter wrote:
> On Thu, Aug 8, 2013 at 11:03 PM, Sean Paul wrote:
>> This patch adds the notion of a drm_bridge. A bridge is a chained
>> device which hangs off an encoder. The drm driver using the bridge
>> should provide the association between encoder and bridge. Once a
>> bridge is associated with an encoder, it will participate in mode
>> set, dpms, and optionally connection detection.
>>
>> Since a connector may not be able to determine the connection state
>> downstream of the bridge, bridges may implement detect() which will take
>> precedence over connector detect() if the bridge is already attached
>> to an encoder and that encoder is already attached to the connector.
>> In practical terms, this requires the drm driver to make these
>> associations at init time, which is fine for SoC applications where
>> this link is static.
>>
>> Signed-off-by: Sean Paul
>
> A few comments below. I think overall any such infrastructure simply
> needs to demonstrate viability on a few drivers, that makes it much
> simpler to check whether the interfaces make sense.
Just a quick note.. I have a branch of msm where I've been using a
slightly earlier version of this (and in next day or so will be
rebased on this version).
and I guess Sean is using this on a driver or two. So that's at least
two drivers ;-)
BR,
-R
> Generally I'd make
> more sense for me if the bridge is just an implementation detail of a
> given encoder and if the bridge would not be exposed to the crtc
> helpers. With that abstraction chaining would be more natural imo and
> we'd also have a much higher chance that bridge drivers work accross
> different drm drivers: Atm you can run some encoder stuff in the crtc
> callbacks and the other way round (and pretty much every driver for a
> bit more complicated hw does that), and every driver can differ in
> those tricks a bit. If we bake the bridge callbacks into the crtc
> helpers I expect that for bridges with tricky ordering constraints
> this will become a giant mess. So I'd prefer much if this would work
> like drm i2c slave encoders.
>
> Cheers, Daniel
>
>> ---
>> drivers/gpu/drm/drm_crtc.c| 50 +
>> drivers/gpu/drm/drm_crtc_helper.c | 112
>> ++
>> drivers/gpu/drm/drm_sysfs.c | 8 ++-
>> include/drm/drm_crtc.h| 48
>> include/drm/drm_crtc_helper.h | 34
>> 5 files changed, 241 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index fc83bb9..0311e2b 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -781,6 +781,41 @@ void drm_connector_unplug_all(struct drm_device *dev)
>> }
>> EXPORT_SYMBOL(drm_connector_unplug_all);
>>
>> +int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
>> + const struct drm_bridge_funcs *funcs)
>> +{
>> + int ret;
>> +
>> + drm_modeset_lock_all(dev);
>> +
>> + ret = drm_mode_object_get(dev, >base,
>> DRM_MODE_OBJECT_BRIDGE);
>> + if (ret)
>> + goto out;
>> +
>> + bridge->dev = dev;
>> + bridge->funcs = funcs;
>> +
>> + list_add_tail(>head, >mode_config.bridge_list);
>> + dev->mode_config.num_bridge++;
>> +
>> + out:
>> + drm_modeset_unlock_all(dev);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(drm_bridge_init);
>> +
>> +void drm_bridge_cleanup(struct drm_bridge *bridge)
>> +{
>> + struct drm_device *dev = bridge->dev;
>> +
>> + drm_modeset_lock_all(dev);
>> + drm_mode_object_put(dev, >base);
>> + list_del(>head);
>> + dev->mode_config.num_bridge--;
>> + drm_modeset_unlock_all(dev);
>> +}
>> +EXPORT_SYMBOL(drm_bridge_cleanup);
>> +
>> int drm_encoder_init(struct drm_device *dev,
>> struct drm_encoder *encoder,
>> const struct drm_encoder_funcs *funcs,
>> @@ -1190,6 +1225,7 @@ static int drm_mode_group_init(struct drm_device *dev,
>> struct drm_mode_group *gr
>> total_objects += dev->mode_config.num_crtc;
>> total_objects += dev->mode_config.num_connector;
>> total_objects += dev->mode_config.num_encoder;
>> + total_objects += dev->mode_config.num_bridge;
>>
>> group->id_list = kzalloc(total_objects * sizeof(uint32_t),
>> GFP_KERNEL);
>> if (!group->id_list)
>> @@ -1198,6 +1234,7 @@ static int drm_mode_group_init(struct drm_device *dev,
>> struct drm_mode_group *gr
>> group->num_crtcs = 0;
>> group->num_connectors = 0;
>> group->num_encoders = 0;
>> + group->num_bridges = 0;
>> return 0;
>> }
>>
>> @@ -1207,6 +1244,7 @@ int drm_mode_group_init_legacy_group(struct drm_device
>> *dev,
>> struct drm_crtc *crtc;
>> struct drm_encoder *encoder;
>> struct drm_connector *connector;
>> + struct drm_bridge *bridge;
>>
[PATCH] drm: Add drm_bridge
On Thu, Aug 8, 2013 at 5:36 PM, Daniel Vetter wrote:
> On Thu, Aug 8, 2013 at 11:03 PM, Sean Paul wrote:
>> This patch adds the notion of a drm_bridge. A bridge is a chained
>> device which hangs off an encoder. The drm driver using the bridge
>> should provide the association between encoder and bridge. Once a
>> bridge is associated with an encoder, it will participate in mode
>> set, dpms, and optionally connection detection.
>>
>> Since a connector may not be able to determine the connection state
>> downstream of the bridge, bridges may implement detect() which will take
>> precedence over connector detect() if the bridge is already attached
>> to an encoder and that encoder is already attached to the connector.
>> In practical terms, this requires the drm driver to make these
>> associations at init time, which is fine for SoC applications where
>> this link is static.
>>
>> Signed-off-by: Sean Paul
>
> A few comments below. I think overall any such infrastructure simply
> needs to demonstrate viability on a few drivers, that makes it much
> simpler to check whether the interfaces make sense.
We have two bridges using it here, and we're working on adding a
third. Rob want to add one too.
> Generally I'd make
> more sense for me if the bridge is just an implementation detail of a
> given encoder and if the bridge would not be exposed to the crtc
> helpers. With that abstraction chaining would be more natural imo and
> we'd also have a much higher chance that bridge drivers work accross
> different drm drivers: Atm you can run some encoder stuff in the crtc
> callbacks and the other way round (and pretty much every driver for a
> bit more complicated hw does that), and every driver can differ in
> those tricks a bit. If we bake the bridge callbacks into the crtc
> helpers I expect that for bridges with tricky ordering constraints
> this will become a giant mess. So I'd prefer much if this would work
> like drm i2c slave encoders.
Interestingly, this is how we started. We first put these bridges
drivers in the i2c/ dir and called them directly from exynos. Then
things grew, and became very invasive in the host driver. So we added
a new interface which allowed us to implement this properly in all
drivers in a way which also extends to more drivers.
St?phane
>
> Cheers, Daniel
>
>> ---
>> drivers/gpu/drm/drm_crtc.c| 50 +
>> drivers/gpu/drm/drm_crtc_helper.c | 112
>> ++
>> drivers/gpu/drm/drm_sysfs.c | 8 ++-
>> include/drm/drm_crtc.h| 48
>> include/drm/drm_crtc_helper.h | 34
>> 5 files changed, 241 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index fc83bb9..0311e2b 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -781,6 +781,41 @@ void drm_connector_unplug_all(struct drm_device *dev)
>> }
>> EXPORT_SYMBOL(drm_connector_unplug_all);
>>
>> +int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
>> + const struct drm_bridge_funcs *funcs)
>> +{
>> + int ret;
>> +
>> + drm_modeset_lock_all(dev);
>> +
>> + ret = drm_mode_object_get(dev, >base,
>> DRM_MODE_OBJECT_BRIDGE);
>> + if (ret)
>> + goto out;
>> +
>> + bridge->dev = dev;
>> + bridge->funcs = funcs;
>> +
>> + list_add_tail(>head, >mode_config.bridge_list);
>> + dev->mode_config.num_bridge++;
>> +
>> + out:
>> + drm_modeset_unlock_all(dev);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(drm_bridge_init);
>> +
>> +void drm_bridge_cleanup(struct drm_bridge *bridge)
>> +{
>> + struct drm_device *dev = bridge->dev;
>> +
>> + drm_modeset_lock_all(dev);
>> + drm_mode_object_put(dev, >base);
>> + list_del(>head);
>> + dev->mode_config.num_bridge--;
>> + drm_modeset_unlock_all(dev);
>> +}
>> +EXPORT_SYMBOL(drm_bridge_cleanup);
>> +
>> int drm_encoder_init(struct drm_device *dev,
>> struct drm_encoder *encoder,
>> const struct drm_encoder_funcs *funcs,
>> @@ -1190,6 +1225,7 @@ static int drm_mode_group_init(struct drm_device *dev,
>> struct drm_mode_group *gr
>> total_objects += dev->mode_config.num_crtc;
>> total_objects += dev->mode_config.num_connector;
>> total_objects += dev->mode_config.num_encoder;
>> + total_objects += dev->mode_config.num_bridge;
>>
>> group->id_list = kzalloc(total_objects * sizeof(uint32_t),
>> GFP_KERNEL);
>> if (!group->id_list)
>> @@ -1198,6 +1234,7 @@ static int drm_mode_group_init(struct drm_device *dev,
>> struct drm_mode_group *gr
>> group->num_crtcs = 0;
>> group->num_connectors = 0;
>> group->num_encoders = 0;
>> + group->num_bridges = 0;
>> return 0;
>> }
>>
>> @@ -1207,6 +1244,7 @@ int drm_mode_group_init_legacy_group(struct
[PATCH] drm: Add drm_bridge
This patch adds the notion of a drm_bridge. A bridge is a chained
device which hangs off an encoder. The drm driver using the bridge
should provide the association between encoder and bridge. Once a
bridge is associated with an encoder, it will participate in mode
set, dpms, and optionally connection detection.
Since a connector may not be able to determine the connection state
downstream of the bridge, bridges may implement detect() which will take
precedence over connector detect() if the bridge is already attached
to an encoder and that encoder is already attached to the connector.
In practical terms, this requires the drm driver to make these
associations at init time, which is fine for SoC applications where
this link is static.
Signed-off-by: Sean Paul
---
drivers/gpu/drm/drm_crtc.c| 50 +
drivers/gpu/drm/drm_crtc_helper.c | 112 ++
drivers/gpu/drm/drm_sysfs.c | 8 ++-
include/drm/drm_crtc.h| 48
include/drm/drm_crtc_helper.h | 34
5 files changed, 241 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index fc83bb9..0311e2b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -781,6 +781,41 @@ void drm_connector_unplug_all(struct drm_device *dev)
}
EXPORT_SYMBOL(drm_connector_unplug_all);
+int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
+ const struct drm_bridge_funcs *funcs)
+{
+ int ret;
+
+ drm_modeset_lock_all(dev);
+
+ ret = drm_mode_object_get(dev, >base, DRM_MODE_OBJECT_BRIDGE);
+ if (ret)
+ goto out;
+
+ bridge->dev = dev;
+ bridge->funcs = funcs;
+
+ list_add_tail(>head, >mode_config.bridge_list);
+ dev->mode_config.num_bridge++;
+
+ out:
+ drm_modeset_unlock_all(dev);
+ return ret;
+}
+EXPORT_SYMBOL(drm_bridge_init);
+
+void drm_bridge_cleanup(struct drm_bridge *bridge)
+{
+ struct drm_device *dev = bridge->dev;
+
+ drm_modeset_lock_all(dev);
+ drm_mode_object_put(dev, >base);
+ list_del(>head);
+ dev->mode_config.num_bridge--;
+ drm_modeset_unlock_all(dev);
+}
+EXPORT_SYMBOL(drm_bridge_cleanup);
+
int drm_encoder_init(struct drm_device *dev,
struct drm_encoder *encoder,
const struct drm_encoder_funcs *funcs,
@@ -1190,6 +1225,7 @@ static int drm_mode_group_init(struct drm_device *dev,
struct drm_mode_group *gr
total_objects += dev->mode_config.num_crtc;
total_objects += dev->mode_config.num_connector;
total_objects += dev->mode_config.num_encoder;
+ total_objects += dev->mode_config.num_bridge;
group->id_list = kzalloc(total_objects * sizeof(uint32_t), GFP_KERNEL);
if (!group->id_list)
@@ -1198,6 +1234,7 @@ static int drm_mode_group_init(struct drm_device *dev,
struct drm_mode_group *gr
group->num_crtcs = 0;
group->num_connectors = 0;
group->num_encoders = 0;
+ group->num_bridges = 0;
return 0;
}
@@ -1207,6 +1244,7 @@ int drm_mode_group_init_legacy_group(struct drm_device
*dev,
struct drm_crtc *crtc;
struct drm_encoder *encoder;
struct drm_connector *connector;
+ struct drm_bridge *bridge;
int ret;
if ((ret = drm_mode_group_init(dev, group)))
@@ -1223,6 +1261,11 @@ int drm_mode_group_init_legacy_group(struct drm_device
*dev,
group->id_list[group->num_crtcs + group->num_encoders +
group->num_connectors++] = connector->base.id;
+ list_for_each_entry(bridge, >mode_config.bridge_list, head)
+ group->id_list[group->num_crtcs + group->num_encoders +
+ group->num_connectors + group->num_bridges++] =
+ bridge->base.id;
+
return 0;
}
EXPORT_SYMBOL(drm_mode_group_init_legacy_group);
@@ -3905,6 +3948,7 @@ void drm_mode_config_init(struct drm_device *dev)
INIT_LIST_HEAD(>mode_config.fb_list);
INIT_LIST_HEAD(>mode_config.crtc_list);
INIT_LIST_HEAD(>mode_config.connector_list);
+ INIT_LIST_HEAD(>mode_config.bridge_list);
INIT_LIST_HEAD(>mode_config.encoder_list);
INIT_LIST_HEAD(>mode_config.property_list);
INIT_LIST_HEAD(>mode_config.property_blob_list);
@@ -3941,6 +3985,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
struct drm_connector *connector, *ot;
struct drm_crtc *crtc, *ct;
struct drm_encoder *encoder, *enct;
+ struct drm_bridge *bridge, *brt;
struct drm_framebuffer *fb, *fbt;
struct drm_property *property, *pt;
struct drm_property_blob *blob, *bt;
@@ -3951,6 +3996,11 @@ void drm_mode_config_cleanup(struct drm_device *dev)
encoder->funcs->destroy(encoder);
}
+
[PATCH] drm: Add drm_bridge
This patch adds the notion of a drm_bridge. A bridge is a chained
device which hangs off an encoder. The drm driver using the bridge
should provide the association between encoder and bridge. Once a
bridge is associated with an encoder, it will participate in mode
set, dpms, and optionally connection detection.
Since a connector may not be able to determine the connection state
downstream of the bridge, bridges may implement detect() which will take
precedence over connector detect() if the bridge is already attached
to an encoder and that encoder is already attached to the connector.
In practical terms, this requires the drm driver to make these
associations at init time, which is fine for SoC applications where
this link is static.
Signed-off-by: Sean Paul seanp...@chromium.org
---
drivers/gpu/drm/drm_crtc.c| 50 +
drivers/gpu/drm/drm_crtc_helper.c | 112 ++
drivers/gpu/drm/drm_sysfs.c | 8 ++-
include/drm/drm_crtc.h| 48
include/drm/drm_crtc_helper.h | 34
5 files changed, 241 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index fc83bb9..0311e2b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -781,6 +781,41 @@ void drm_connector_unplug_all(struct drm_device *dev)
}
EXPORT_SYMBOL(drm_connector_unplug_all);
+int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
+ const struct drm_bridge_funcs *funcs)
+{
+ int ret;
+
+ drm_modeset_lock_all(dev);
+
+ ret = drm_mode_object_get(dev, bridge-base, DRM_MODE_OBJECT_BRIDGE);
+ if (ret)
+ goto out;
+
+ bridge-dev = dev;
+ bridge-funcs = funcs;
+
+ list_add_tail(bridge-head, dev-mode_config.bridge_list);
+ dev-mode_config.num_bridge++;
+
+ out:
+ drm_modeset_unlock_all(dev);
+ return ret;
+}
+EXPORT_SYMBOL(drm_bridge_init);
+
+void drm_bridge_cleanup(struct drm_bridge *bridge)
+{
+ struct drm_device *dev = bridge-dev;
+
+ drm_modeset_lock_all(dev);
+ drm_mode_object_put(dev, bridge-base);
+ list_del(bridge-head);
+ dev-mode_config.num_bridge--;
+ drm_modeset_unlock_all(dev);
+}
+EXPORT_SYMBOL(drm_bridge_cleanup);
+
int drm_encoder_init(struct drm_device *dev,
struct drm_encoder *encoder,
const struct drm_encoder_funcs *funcs,
@@ -1190,6 +1225,7 @@ static int drm_mode_group_init(struct drm_device *dev,
struct drm_mode_group *gr
total_objects += dev-mode_config.num_crtc;
total_objects += dev-mode_config.num_connector;
total_objects += dev-mode_config.num_encoder;
+ total_objects += dev-mode_config.num_bridge;
group-id_list = kzalloc(total_objects * sizeof(uint32_t), GFP_KERNEL);
if (!group-id_list)
@@ -1198,6 +1234,7 @@ static int drm_mode_group_init(struct drm_device *dev,
struct drm_mode_group *gr
group-num_crtcs = 0;
group-num_connectors = 0;
group-num_encoders = 0;
+ group-num_bridges = 0;
return 0;
}
@@ -1207,6 +1244,7 @@ int drm_mode_group_init_legacy_group(struct drm_device
*dev,
struct drm_crtc *crtc;
struct drm_encoder *encoder;
struct drm_connector *connector;
+ struct drm_bridge *bridge;
int ret;
if ((ret = drm_mode_group_init(dev, group)))
@@ -1223,6 +1261,11 @@ int drm_mode_group_init_legacy_group(struct drm_device
*dev,
group-id_list[group-num_crtcs + group-num_encoders +
group-num_connectors++] = connector-base.id;
+ list_for_each_entry(bridge, dev-mode_config.bridge_list, head)
+ group-id_list[group-num_crtcs + group-num_encoders +
+ group-num_connectors + group-num_bridges++] =
+ bridge-base.id;
+
return 0;
}
EXPORT_SYMBOL(drm_mode_group_init_legacy_group);
@@ -3905,6 +3948,7 @@ void drm_mode_config_init(struct drm_device *dev)
INIT_LIST_HEAD(dev-mode_config.fb_list);
INIT_LIST_HEAD(dev-mode_config.crtc_list);
INIT_LIST_HEAD(dev-mode_config.connector_list);
+ INIT_LIST_HEAD(dev-mode_config.bridge_list);
INIT_LIST_HEAD(dev-mode_config.encoder_list);
INIT_LIST_HEAD(dev-mode_config.property_list);
INIT_LIST_HEAD(dev-mode_config.property_blob_list);
@@ -3941,6 +3985,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
struct drm_connector *connector, *ot;
struct drm_crtc *crtc, *ct;
struct drm_encoder *encoder, *enct;
+ struct drm_bridge *bridge, *brt;
struct drm_framebuffer *fb, *fbt;
struct drm_property *property, *pt;
struct drm_property_blob *blob, *bt;
@@ -3951,6 +3996,11 @@ void drm_mode_config_cleanup(struct drm_device *dev)
Re: [PATCH] drm: Add drm_bridge
On Thu, Aug 8, 2013 at 11:03 PM, Sean Paul seanp...@chromium.org wrote:
This patch adds the notion of a drm_bridge. A bridge is a chained
device which hangs off an encoder. The drm driver using the bridge
should provide the association between encoder and bridge. Once a
bridge is associated with an encoder, it will participate in mode
set, dpms, and optionally connection detection.
Since a connector may not be able to determine the connection state
downstream of the bridge, bridges may implement detect() which will take
precedence over connector detect() if the bridge is already attached
to an encoder and that encoder is already attached to the connector.
In practical terms, this requires the drm driver to make these
associations at init time, which is fine for SoC applications where
this link is static.
Signed-off-by: Sean Paul seanp...@chromium.org
A few comments below. I think overall any such infrastructure simply
needs to demonstrate viability on a few drivers, that makes it much
simpler to check whether the interfaces make sense. Generally I'd make
more sense for me if the bridge is just an implementation detail of a
given encoder and if the bridge would not be exposed to the crtc
helpers. With that abstraction chaining would be more natural imo and
we'd also have a much higher chance that bridge drivers work accross
different drm drivers: Atm you can run some encoder stuff in the crtc
callbacks and the other way round (and pretty much every driver for a
bit more complicated hw does that), and every driver can differ in
those tricks a bit. If we bake the bridge callbacks into the crtc
helpers I expect that for bridges with tricky ordering constraints
this will become a giant mess. So I'd prefer much if this would work
like drm i2c slave encoders.
Cheers, Daniel
---
drivers/gpu/drm/drm_crtc.c| 50 +
drivers/gpu/drm/drm_crtc_helper.c | 112
++
drivers/gpu/drm/drm_sysfs.c | 8 ++-
include/drm/drm_crtc.h| 48
include/drm/drm_crtc_helper.h | 34
5 files changed, 241 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index fc83bb9..0311e2b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -781,6 +781,41 @@ void drm_connector_unplug_all(struct drm_device *dev)
}
EXPORT_SYMBOL(drm_connector_unplug_all);
+int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
+ const struct drm_bridge_funcs *funcs)
+{
+ int ret;
+
+ drm_modeset_lock_all(dev);
+
+ ret = drm_mode_object_get(dev, bridge-base, DRM_MODE_OBJECT_BRIDGE);
+ if (ret)
+ goto out;
+
+ bridge-dev = dev;
+ bridge-funcs = funcs;
+
+ list_add_tail(bridge-head, dev-mode_config.bridge_list);
+ dev-mode_config.num_bridge++;
+
+ out:
+ drm_modeset_unlock_all(dev);
+ return ret;
+}
+EXPORT_SYMBOL(drm_bridge_init);
+
+void drm_bridge_cleanup(struct drm_bridge *bridge)
+{
+ struct drm_device *dev = bridge-dev;
+
+ drm_modeset_lock_all(dev);
+ drm_mode_object_put(dev, bridge-base);
+ list_del(bridge-head);
+ dev-mode_config.num_bridge--;
+ drm_modeset_unlock_all(dev);
+}
+EXPORT_SYMBOL(drm_bridge_cleanup);
+
int drm_encoder_init(struct drm_device *dev,
struct drm_encoder *encoder,
const struct drm_encoder_funcs *funcs,
@@ -1190,6 +1225,7 @@ static int drm_mode_group_init(struct drm_device *dev,
struct drm_mode_group *gr
total_objects += dev-mode_config.num_crtc;
total_objects += dev-mode_config.num_connector;
total_objects += dev-mode_config.num_encoder;
+ total_objects += dev-mode_config.num_bridge;
group-id_list = kzalloc(total_objects * sizeof(uint32_t),
GFP_KERNEL);
if (!group-id_list)
@@ -1198,6 +1234,7 @@ static int drm_mode_group_init(struct drm_device *dev,
struct drm_mode_group *gr
group-num_crtcs = 0;
group-num_connectors = 0;
group-num_encoders = 0;
+ group-num_bridges = 0;
return 0;
}
@@ -1207,6 +1244,7 @@ int drm_mode_group_init_legacy_group(struct drm_device
*dev,
struct drm_crtc *crtc;
struct drm_encoder *encoder;
struct drm_connector *connector;
+ struct drm_bridge *bridge;
int ret;
if ((ret = drm_mode_group_init(dev, group)))
@@ -1223,6 +1261,11 @@ int drm_mode_group_init_legacy_group(struct drm_device
*dev,
group-id_list[group-num_crtcs + group-num_encoders +
group-num_connectors++] = connector-base.id;
+ list_for_each_entry(bridge, dev-mode_config.bridge_list, head)
+ group-id_list[group-num_crtcs + group-num_encoders +
+
Re: [PATCH] drm: Add drm_bridge
On Thu, Aug 8, 2013 at 8:36 PM, Daniel Vetter dan...@ffwll.ch wrote:
On Thu, Aug 8, 2013 at 11:03 PM, Sean Paul seanp...@chromium.org wrote:
This patch adds the notion of a drm_bridge. A bridge is a chained
device which hangs off an encoder. The drm driver using the bridge
should provide the association between encoder and bridge. Once a
bridge is associated with an encoder, it will participate in mode
set, dpms, and optionally connection detection.
Since a connector may not be able to determine the connection state
downstream of the bridge, bridges may implement detect() which will take
precedence over connector detect() if the bridge is already attached
to an encoder and that encoder is already attached to the connector.
In practical terms, this requires the drm driver to make these
associations at init time, which is fine for SoC applications where
this link is static.
Signed-off-by: Sean Paul seanp...@chromium.org
A few comments below. I think overall any such infrastructure simply
needs to demonstrate viability on a few drivers, that makes it much
simpler to check whether the interfaces make sense.
Just a quick note.. I have a branch of msm where I've been using a
slightly earlier version of this (and in next day or so will be
rebased on this version).
and I guess Sean is using this on a driver or two. So that's at least
two drivers ;-)
BR,
-R
Generally I'd make
more sense for me if the bridge is just an implementation detail of a
given encoder and if the bridge would not be exposed to the crtc
helpers. With that abstraction chaining would be more natural imo and
we'd also have a much higher chance that bridge drivers work accross
different drm drivers: Atm you can run some encoder stuff in the crtc
callbacks and the other way round (and pretty much every driver for a
bit more complicated hw does that), and every driver can differ in
those tricks a bit. If we bake the bridge callbacks into the crtc
helpers I expect that for bridges with tricky ordering constraints
this will become a giant mess. So I'd prefer much if this would work
like drm i2c slave encoders.
Cheers, Daniel
---
drivers/gpu/drm/drm_crtc.c| 50 +
drivers/gpu/drm/drm_crtc_helper.c | 112
++
drivers/gpu/drm/drm_sysfs.c | 8 ++-
include/drm/drm_crtc.h| 48
include/drm/drm_crtc_helper.h | 34
5 files changed, 241 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index fc83bb9..0311e2b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -781,6 +781,41 @@ void drm_connector_unplug_all(struct drm_device *dev)
}
EXPORT_SYMBOL(drm_connector_unplug_all);
+int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
+ const struct drm_bridge_funcs *funcs)
+{
+ int ret;
+
+ drm_modeset_lock_all(dev);
+
+ ret = drm_mode_object_get(dev, bridge-base,
DRM_MODE_OBJECT_BRIDGE);
+ if (ret)
+ goto out;
+
+ bridge-dev = dev;
+ bridge-funcs = funcs;
+
+ list_add_tail(bridge-head, dev-mode_config.bridge_list);
+ dev-mode_config.num_bridge++;
+
+ out:
+ drm_modeset_unlock_all(dev);
+ return ret;
+}
+EXPORT_SYMBOL(drm_bridge_init);
+
+void drm_bridge_cleanup(struct drm_bridge *bridge)
+{
+ struct drm_device *dev = bridge-dev;
+
+ drm_modeset_lock_all(dev);
+ drm_mode_object_put(dev, bridge-base);
+ list_del(bridge-head);
+ dev-mode_config.num_bridge--;
+ drm_modeset_unlock_all(dev);
+}
+EXPORT_SYMBOL(drm_bridge_cleanup);
+
int drm_encoder_init(struct drm_device *dev,
struct drm_encoder *encoder,
const struct drm_encoder_funcs *funcs,
@@ -1190,6 +1225,7 @@ static int drm_mode_group_init(struct drm_device *dev,
struct drm_mode_group *gr
total_objects += dev-mode_config.num_crtc;
total_objects += dev-mode_config.num_connector;
total_objects += dev-mode_config.num_encoder;
+ total_objects += dev-mode_config.num_bridge;
group-id_list = kzalloc(total_objects * sizeof(uint32_t),
GFP_KERNEL);
if (!group-id_list)
@@ -1198,6 +1234,7 @@ static int drm_mode_group_init(struct drm_device *dev,
struct drm_mode_group *gr
group-num_crtcs = 0;
group-num_connectors = 0;
group-num_encoders = 0;
+ group-num_bridges = 0;
return 0;
}
@@ -1207,6 +1244,7 @@ int drm_mode_group_init_legacy_group(struct drm_device
*dev,
struct drm_crtc *crtc;
struct drm_encoder *encoder;
struct drm_connector *connector;
+ struct drm_bridge *bridge;
int ret;
if ((ret = drm_mode_group_init(dev, group)))
@@ -1223,6 +1261,11 @@ int drm_mode_group_init_legacy_group(struct
drm_device
Re: [PATCH] drm: Add drm_bridge
On Thu, Aug 8, 2013 at 5:36 PM, Daniel Vetter dan...@ffwll.ch wrote:
On Thu, Aug 8, 2013 at 11:03 PM, Sean Paul seanp...@chromium.org wrote:
This patch adds the notion of a drm_bridge. A bridge is a chained
device which hangs off an encoder. The drm driver using the bridge
should provide the association between encoder and bridge. Once a
bridge is associated with an encoder, it will participate in mode
set, dpms, and optionally connection detection.
Since a connector may not be able to determine the connection state
downstream of the bridge, bridges may implement detect() which will take
precedence over connector detect() if the bridge is already attached
to an encoder and that encoder is already attached to the connector.
In practical terms, this requires the drm driver to make these
associations at init time, which is fine for SoC applications where
this link is static.
Signed-off-by: Sean Paul seanp...@chromium.org
A few comments below. I think overall any such infrastructure simply
needs to demonstrate viability on a few drivers, that makes it much
simpler to check whether the interfaces make sense.
We have two bridges using it here, and we're working on adding a
third. Rob want to add one too.
Generally I'd make
more sense for me if the bridge is just an implementation detail of a
given encoder and if the bridge would not be exposed to the crtc
helpers. With that abstraction chaining would be more natural imo and
we'd also have a much higher chance that bridge drivers work accross
different drm drivers: Atm you can run some encoder stuff in the crtc
callbacks and the other way round (and pretty much every driver for a
bit more complicated hw does that), and every driver can differ in
those tricks a bit. If we bake the bridge callbacks into the crtc
helpers I expect that for bridges with tricky ordering constraints
this will become a giant mess. So I'd prefer much if this would work
like drm i2c slave encoders.
Interestingly, this is how we started. We first put these bridges
drivers in the i2c/ dir and called them directly from exynos. Then
things grew, and became very invasive in the host driver. So we added
a new interface which allowed us to implement this properly in all
drivers in a way which also extends to more drivers.
Stéphane
Cheers, Daniel
---
drivers/gpu/drm/drm_crtc.c| 50 +
drivers/gpu/drm/drm_crtc_helper.c | 112
++
drivers/gpu/drm/drm_sysfs.c | 8 ++-
include/drm/drm_crtc.h| 48
include/drm/drm_crtc_helper.h | 34
5 files changed, 241 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index fc83bb9..0311e2b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -781,6 +781,41 @@ void drm_connector_unplug_all(struct drm_device *dev)
}
EXPORT_SYMBOL(drm_connector_unplug_all);
+int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
+ const struct drm_bridge_funcs *funcs)
+{
+ int ret;
+
+ drm_modeset_lock_all(dev);
+
+ ret = drm_mode_object_get(dev, bridge-base,
DRM_MODE_OBJECT_BRIDGE);
+ if (ret)
+ goto out;
+
+ bridge-dev = dev;
+ bridge-funcs = funcs;
+
+ list_add_tail(bridge-head, dev-mode_config.bridge_list);
+ dev-mode_config.num_bridge++;
+
+ out:
+ drm_modeset_unlock_all(dev);
+ return ret;
+}
+EXPORT_SYMBOL(drm_bridge_init);
+
+void drm_bridge_cleanup(struct drm_bridge *bridge)
+{
+ struct drm_device *dev = bridge-dev;
+
+ drm_modeset_lock_all(dev);
+ drm_mode_object_put(dev, bridge-base);
+ list_del(bridge-head);
+ dev-mode_config.num_bridge--;
+ drm_modeset_unlock_all(dev);
+}
+EXPORT_SYMBOL(drm_bridge_cleanup);
+
int drm_encoder_init(struct drm_device *dev,
struct drm_encoder *encoder,
const struct drm_encoder_funcs *funcs,
@@ -1190,6 +1225,7 @@ static int drm_mode_group_init(struct drm_device *dev,
struct drm_mode_group *gr
total_objects += dev-mode_config.num_crtc;
total_objects += dev-mode_config.num_connector;
total_objects += dev-mode_config.num_encoder;
+ total_objects += dev-mode_config.num_bridge;
group-id_list = kzalloc(total_objects * sizeof(uint32_t),
GFP_KERNEL);
if (!group-id_list)
@@ -1198,6 +1234,7 @@ static int drm_mode_group_init(struct drm_device *dev,
struct drm_mode_group *gr
group-num_crtcs = 0;
group-num_connectors = 0;
group-num_encoders = 0;
+ group-num_bridges = 0;
return 0;
}
@@ -1207,6 +1244,7 @@ int drm_mode_group_init_legacy_group(struct drm_device
*dev,
struct drm_crtc *crtc;
struct drm_encoder *encoder;
struct drm_connector *connector;
+
