Hi Laurent
Thanks for responding.
On 2/21/2022 11:34 PM, Laurent Pinchart wrote:
Hi Dmitry,
On Tue, Feb 22, 2022 at 06:32:50AM +0300, Dmitry Baryshkov wrote:
On Thu, 10 Feb 2022 at 07:59, Laurent Pinchart wrote:
On Wed, Feb 09, 2022 at 05:40:29PM -0800, Abhinav Kumar wrote:
Hi Laurent
Gentle reminder on this.
I won't have time before next week I'm afraid.
Laurent, another gentle ping.
I'm really late on this so I probably deserve a bit of a rougher ping,
but thanks for being gentle :-)
On 2/6/2022 11:20 PM, Abhinav Kumar wrote:
On 2/6/2022 3:32 PM, Dmitry Baryshkov wrote:
On Wed, 2 Feb 2022 at 16:26, Laurent Pinchart wrote:
On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
On Wed, 02 Feb 2022, Laurent Pinchart wrote:
On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
Changing rcar_du driver to accomadate the change of
drm_writeback_connector.base and drm_writeback_connector.encoder
to a pointer the reason for which is explained in the
Patch(drm: add writeback pointers to drm_connector).
Signed-off-by: Kandpal Suraj <suraj.kandpal@xxxxxxxxx>
---
drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++
drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
index 66e8839db708..68f387a04502 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -72,6 +72,8 @@ struct rcar_du_crtc {
const char *const *sources;
unsigned int sources_count;
+ struct drm_connector connector;
+ struct drm_encoder encoder;
Those fields are, at best, poorly named. Furthermore, there's no need in
this driver or in other drivers using drm_writeback_connector to create
an encoder or connector manually. Let's not polute all drivers because
i915 doesn't have its abstractions right.
i915 uses the quite common model for struct inheritance:
struct intel_connector {
struct drm_connector base;
/* ... */
}
Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
radeon, tilcdc, and vboxvideo.
We could argue about the relative merits of that abstraction, but I
think the bottom line is that it's popular and the drivers using it are
not going to be persuaded to move away from it.
Nobody said inheritance is bad.
It's no coincidence that the drivers who've implemented writeback so far
(komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
because the drm_writeback_connector midlayer does, forcing the issue.
Are you sure it's not a coincidence ? :-)
The encoder and especially connector created by drm_writeback_connector
are there only because KMS requires a drm_encoder and a drm_connector to
be exposed to userspace (and I could argue that using a connector for
writeback is a hack, but that won't change). The connector is "virtual",
I still fail to see why i915 or any other driver would need to wrap it
into something else. The whole point of the drm_writeback_connector
abstraction is that drivers do not have to manage the writeback
drm_connector manually, they shouldn't touch it at all.
Laurent, I wanted to shift a bit from the question of drm_connector to
the question of drm_encoder being embedded in the drm_writeback_connector.
In case of the msm driver the drm_encoder is not a lightweight entity,
but a full-featured driver part. Significant part of it can be shared
with the writeback implementation, if we allow using a pointer to the
external drm_encoder with the drm_writeback_connector.
Does the following patch set stand a chance to receive your ack?
- Switch drm_writeback_connector to point to drm_encoder rather than
embedding it?
- Create drm_encoder for the drm_writeback_connector when one is not
specified, so the current drivers can be left unchanged.
The situation is a bit different for the encoder indeed.
The encoder concept is loosely defined nowadays, with more and more of
the "real" encoders being implemented as a drm_bridge. That's what I
usually recommend when reviewing new drivers. drm_encoder is slowly
becoming an empty shell (see for instance [1]), although that transition
is not enforced globally and will thus take a long time to complete (if
ever).
This being said, lots of drivers have "featureful" encoder
implementations, and that won't go away any time soon. In those cases, I
could be OK with drivers optionally passing an encoder fo the writeback
helper if the hardware really shares resources between writeback and a
real encoder. I would however be careful there, as in many cases I would
expect the need to pass a custom encoder to originate from an old
software design decision rather than from the hardware architecture. In
those cases it would be best, I think, to move towards cleaning up the
software architecture, but that can be done step by step and I won't
consider that a requirement to implement writeback support.
In the MSM case in particular, can you explain what resources are shared
between writeback and hardware encoder(s) ?
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
Yes, we indeed have a lot of functionality in our encoder. It shares
both interrupt and clock control for all interfaces including writeback.
Moreover, like I was mentioning earlier, on some of the chipsets where
display hardware is limited, the hardware components mapped to a drm
encoder can be shared between the panel and writeback paths.
For your reference, please check [1]
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#n174
Hence we are requesting that drm_writeback not embed an encoder but
acommodate a pointer to drm_encoder instead.
I second Dmitry's request here. For the reasons he has mentioned along
with the possibility of the writeback encoder being shared across
display pipelines, strengthens our request of the drm encoder being a
pointer inside the drm_writeback_connector instead of embedding it.
Like I had shown in my RFC, in case the other drivers dont specify one,
we can allocate one:
https://patchwork.kernel.org/project/dri-devel/patch/1642732195-25349-1-git-send-email-quic_abhinavk@xxxxxxxxxxx/
We think this should be a reasonable accomodation to the existing
drm_writeback driver.
So I think drm_writeback_connector should *not* use the inheritance
abstraction because it's a midlayer that should leave that option tothe
drivers. I think drm_writeback_connector needs to be changed to
accommodate that, and, unfortunately, it means current writeback users
need to be changed as well.
I am not sure, however, if the series at hand is the right
approach. Perhaps writeback can be modified to allocate the stuff for
you if you prefer it that way, as long as the drm_connector is not
embedded in struct drm_writeback_connector.
Nack.
struct drm_writeback_connector writeback;
};
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
index c79d1259e49b..5b1e83380c47 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
@@ -200,8 +200,10 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
{
struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
- wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
- drm_connector_helper_add(&wb_conn->base,
+ wb_conn->base = &rcrtc->connector;
+ wb_conn->encoder = &rcrtc->encoder;
+ wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
+ drm_connector_helper_add(wb_conn->base,
&rcar_du_wb_conn_helper_funcs);
return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
@@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
struct drm_framebuffer *fb;
unsigned int i;
- state = rcrtc->writeback.base.state;
+ state = rcrtc->writeback.base->state;
if (!state || !state->writeback_job)
return;
