Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[Mihail Atanassov]

Hi James,

On Wednesday, 9 October 2019 06:54:15 BST james qian wang (Arm Technology China) wrote:
> On Fri, Oct 04, 2019 at 02:34:42PM +0000, Mihail Atanassov wrote:
> > To support transmitters other than the tda998x, we need to allow
> > non-component framework bridges to be attached to a dummy drm_encoder in
> > our driver.
> > 
> > For the existing supported encoder (tda998x), keep the behaviour as-is,
> > since there's no way to unbind if a drm_bridge module goes away under
> > our feet.
> > 
> > Signed-off-by: Mihail Atanassov <mihail.atanassov@xxxxxxx>
> > ---
> >  .../gpu/drm/arm/display/komeda/komeda_dev.h   |   5 +
> >  .../gpu/drm/arm/display/komeda/komeda_drv.c   |  58 ++++++--
> >  .../gpu/drm/arm/display/komeda/komeda_kms.c   | 133 +++++++++++++++++-
> >  .../gpu/drm/arm/display/komeda/komeda_kms.h   |   5 +
> >  4 files changed, 187 insertions(+), 14 deletions(-)
> > 
> > [snip]
> >  
> > +static void komeda_encoder_destroy(struct drm_encoder *encoder)
> > +{
> > +	drm_encoder_cleanup(encoder);
> > +}
> > +
> > +static const struct drm_encoder_funcs komeda_dummy_enc_funcs = {
> > +	.destroy = komeda_encoder_destroy,
> > +};
> > +
> > +bool komeda_remote_device_is_component(struct device_node *local,
> > +				       u32 port, u32 endpoint)
> > +{
> > +	struct device_node *remote;
> > +	char const * const component_devices[] = {
> > +		"nxp,tda998x",
> 
> I really don't think put this dummy_encoder into komeda is a good
> idea.
> 
> And I suggest to seperate this dummy_encoder to a individual module
> which will build the drm_ridge to a standard drm encoder and component
> based module, which will be enable by DT, totally transparent for komeda.
> 
> BTW:
> I really don't like such logic: distingush the SYSTEM configuration
> by check the connected device name, it's hard to maintain and code
> sharing, and that's why NOW we have the device-tree.

+Cc Brian

I didn't think DT is the right place for pseudo-devices. The tda998x
looks to be in a small group of drivers that contain encoder +
bridge + connector; my impression of the current state of affairs is
that the drm_encoder tends to live where the CRTC provider is rather
than representing a HW entity (hence why drm_bridge based drivers
exist in drivers/gpu/drm/bridge). See the overview DOC comment in
drm_encoder.c ("drivers are free to use [drm_encoder] however they
wish"). I may be completely wrong, so would appreciate some
context and comment from others on the Cc list.

In any case, converting a do-nothing dummy encoder into its own
component-module will add a lot more bloat compared to the current
~10 SLoC implementation of the drm_encoder. probe/remove/bind/unbind,
a few extra structs here and there, yet another object file, DT
bindings, docs for the same, and maintaining all of that? It's a hard
sell for me. I'd prefer if we went ahead with the code as-is and fix it
up later if it really proves unwieldy and too hard to maintain. Could
this patch be improved? Sure! Can we improve it in follow-up work
though, as I think this is valuable enough on its own without any major
drawbacks?

As per my cover letter, in an ideal world we'd have the encoder locally
and do drm_bridge_attach() even for tda998x, but the lifetime issues
around bridges inside modules mean that doing that now is a bit of a
step back for this specific case.

> 
> Thanks
> James
> 
> > [snip]
> 

-- 
Mihail