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

[Abhinav Kumar]

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 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.
> 
> 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@quicinc.com/
>>>>>
>>>>> 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;
>>>>>>>>>>
>