[PATCH] drm: Add drm_bridge

[Rob Clark]

On Tue, Aug 13, 2013 at 3:55 PM, Sean Paul <seanpaul at chromium.org> wrote:
> On Tue, Aug 13, 2013 at 3:16 PM, Daniel Vetter <daniel at 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.

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