On Wed, 02 Feb 2022, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> Hi Kandpal,
>
> Thank you for the patch.
>
> 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.
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.
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.
BR,
Jani.
>
> 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;
>>
--
Jani Nikula, Intel Open Source Graphics Center
