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

[Brian Starkey]

Hi,

On Wed, Oct 16, 2019 at 03:51:39PM +0000, Mihail Atanassov wrote:
> 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.

It's not ideal to have such special cases, for sure. However, I don't
see this approach causing us any issues. tda998x really is "special",
and as far as I can see the code here scales to other devices pretty
easily.

> 
> +Cc Brian
> 
> I didn't think DT is the right place for pseudo-devices.

I agree. DT should represent the HW, not the structure of the
linux kernel subsystem.

> 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?
> 

Even if we implemented a separate component encoder, as far as I can
tell there's no way to use it without either:

a) sticking it in DT
b) invoking it from komeda based on a "of_device_is_compatible" list

IMO a) isn't acceptable, and b) doesn't have any advantages over this
approach.

> 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.
> 

Yeah, my feeling is that being able to keep tda998x as a component
(for the superior bind/unbind behaviour) is worth the slight ugliness,
at least until bridges get the same functionality.

If James is strongly against merging this, maybe we just swap
wholesale to bridge? But for me, the pragmatic approach would be this
stop-gap.

Cheers,
-Brian

> > 
> > Thanks
> > James
> > 
> > > [snip]
> > 
> 
> -- 
> Mihail
> 
> 
>