[PATCH] drm: Add drm_bridge

Daniel Vetter daniel at ffwll.ch
Tue Aug 13 12:16:12 PDT 2013


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


More information about the dri-devel mailing list