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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
