lkml.org 
[lkml]   [2019]   [Oct]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
Date
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@arm.com>
> > ---
> > .../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



\
 
 \ /
  Last update: 2019-10-16 17:52    [W:0.127 / U:2.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site