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
