[PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Abhinav Kumar]

Hi Laurent

Gentle reminder on this.

Thanks

Abhinav

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 at ideasonboard.com> 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 at intel.com>
>>>>>> ---
>>>>>>   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@quicinc.com/ 
> 
> 
> 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
>>
>>
>>