Hi Abhinav,
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.
> On 2/6/2022 11:20 PM, Abhinav Kumar wrote:
> > Hi Laurent
> >
> > On 2/6/2022 3:32 PM, Dmitry Baryshkov wrote:
> >> On Wed, 2 Feb 2022 at 16:26, Laurent Pinchart
> >> <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> >>>
> >>> Hi Jani,
> >>>
> >>> 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.
> >>
> >
> > 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.
> >
> > Thanks
> >
> > Abhinav
> >
> >>>
> >>>> So I think drm_writeback_connector should *not* use the inheritance
> >>>> abstraction because it's a midlayer that should leave that option to
> >>>> the
> >>>> 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;
> >>>>>>
--
Regards,
Laurent Pinchart
