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

[james qian wang (Arm Technology China)]

On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote:
> 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.
>

This is a good idea, and I vote +ULONG_MAX :)

and I also checked tda998x driver, it supports bridge. so swap the
wholesale to brige is perfect. :)

Thanks
James.

> Cheers,
> -Brian
> 
> > > 
> > > Thanks
> > > James
> > > 
> > > > [snip]
> > > 
> > 
> > -- 
> > Mihail
> > 
> > 
> >