From: Mihail Atanassov <Mihail.Atanassov@arm.com> To: "james qian wang (Arm Technology China)" <james.qian.wang@arm.com> Cc: "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, David Airlie <airlied@linux.ie>, Liviu Dudau <Liviu.Dudau@arm.com>, Russell King <linux@armlinux.org.uk>, Maxime Ripard <mripard@kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, nd <nd@arm.com>, Sean Paul <sean@poorly.run>, Brian Starkey <Brian.Starkey@arm.com> Subject: Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints Date: Wed, 16 Oct 2019 15:51:39 +0000 [thread overview] Message-ID: <5390495.Gzyn2rW8Nj@e123338-lin> (raw) In-Reply-To: <20191009055407.GA3082@jamwan02-TSP300> 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
WARNING: multiple messages have this Message-ID (diff)
From: Mihail Atanassov <Mihail.Atanassov@arm.com> To: "james qian wang (Arm Technology China)" <james.qian.wang@arm.com> Cc: David Airlie <airlied@linux.ie>, Liviu Dudau <Liviu.Dudau@arm.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, Russell King <linux@armlinux.org.uk>, nd <nd@arm.com>, Sean Paul <sean@poorly.run> Subject: Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints Date: Wed, 16 Oct 2019 15:51:39 +0000 [thread overview] Message-ID: <5390495.Gzyn2rW8Nj@e123338-lin> (raw) In-Reply-To: <20191009055407.GA3082@jamwan02-TSP300> 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2019-10-16 15:52 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-10-04 14:34 [PATCH 0/3] drm/komeda: Support for drm_bridge endpoints Mihail Atanassov 2019-10-04 14:34 ` Mihail Atanassov 2019-10-04 14:34 ` [PATCH 1/3] drm/komeda: Consolidate struct komeda_drv allocations Mihail Atanassov 2019-10-04 14:34 ` Mihail Atanassov 2019-10-04 14:34 ` [PATCH 2/3] drm/komeda: Memory manage struct komeda_drv in probe/remove Mihail Atanassov 2019-10-04 14:34 ` Mihail Atanassov 2019-10-04 14:34 ` [RFC PATCH 3/3] drm/komeda: Allow non-component drm_bridge only endpoints Mihail Atanassov 2019-10-04 14:34 ` Mihail Atanassov 2019-10-09 5:54 ` [RFC,3/3] " james qian wang (Arm Technology China) 2019-10-09 5:54 ` james qian wang (Arm Technology China) 2019-10-16 15:51 ` Mihail Atanassov [this message] 2019-10-16 15:51 ` Mihail Atanassov 2019-10-16 16:22 ` Brian Starkey 2019-10-17 3:07 ` james qian wang (Arm Technology China) 2019-10-17 3:07 ` james qian wang (Arm Technology China) 2019-10-17 8:20 ` Brian Starkey 2019-10-17 8:20 ` Brian Starkey 2019-10-17 10:21 ` james qian wang (Arm Technology China) 2019-10-17 10:21 ` james qian wang (Arm Technology China) 2019-10-17 10:48 ` Brian Starkey 2019-10-17 10:48 ` Brian Starkey 2019-10-17 11:41 ` Russell King - ARM Linux admin 2019-10-17 11:41 ` Russell King - ARM Linux admin 2019-10-18 6:57 ` james qian wang (Arm Technology China) 2019-10-18 6:57 ` james qian wang (Arm Technology China) 2019-10-18 9:12 ` Brian Starkey 2019-10-18 9:12 ` Brian Starkey 2019-10-22 8:42 ` Daniel Vetter 2019-10-22 8:48 ` Russell King - ARM Linux admin 2019-10-22 8:50 ` Daniel Vetter 2019-10-22 14:42 ` Russell King - ARM Linux admin 2019-10-22 14:42 ` Russell King - ARM Linux admin 2019-10-22 14:53 ` Russell King - ARM Linux admin 2019-10-22 14:53 ` Russell King - ARM Linux admin 2019-10-24 8:03 ` Mihail Atanassov 2019-10-24 8:03 ` Mihail Atanassov 2019-10-24 8:03 ` Mihail Atanassov 2019-10-24 5:21 ` james qian wang (Arm Technology China) 2019-10-24 5:21 ` james qian wang (Arm Technology China) 2019-10-18 6:38 ` james qian wang (Arm Technology China) 2019-10-18 11:01 ` Mihail Atanassov 2019-10-18 11:01 ` Mihail Atanassov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=5390495.Gzyn2rW8Nj@e123338-lin \ --to=mihail.atanassov@arm.com \ --cc=Brian.Starkey@arm.com \ --cc=Liviu.Dudau@arm.com \ --cc=airlied@linux.ie \ --cc=dri-devel@lists.freedesktop.org \ --cc=james.qian.wang@arm.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=mripard@kernel.org \ --cc=nd@arm.com \ --cc=sean@poorly.run \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.