On Wed, 02 Feb 2022, Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> wrote:
> On Wed, 2 Feb 2022 at 16:15, Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
>>
>> 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.
>
> For the reference. msm does not wrap drm_connector into any _common_
> structure, which is used internally.
>
>> 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.
>
> As I wrote earlier, I am not sure if these drivers would try using
> their drm_connector subclass for writeback.
> ast: ast_connector = drm_connector + respective i2c adapter for EDID,
> not needed for WB
> fsl-dcu: fsl_dcu_drm_connector = drm_connector + drm_encoder pointer +
> drm_panel. Not needed for WB
> hisilicon, mgag200: same as ast
> tilcdc: same as fsl-dcu
> vboxdrv: the only driver that may possibly benefit from using
> vbox_connector in the writeback support, as the connector is bare
> drm_connector + crtc pointer + hints (width, height, disconnected).
>
> I have left amd, nouveau and radeon out of this list, too complex to
> analyze in several minutes.
>
> I'd second the proposal of supporting optional drm_encoder for
> drm_writeback_connector (as the crtc/encoder split can be vague), but
> I do not see the benefit for the drivers to use their own
> drm_connector subclass for drm_writeback.
If a driver uses inheritance throughout the driver, and a *different*
subclass gets introduced into the mix, you need to add a ton of checks
all over the place when you cast the superclass pointer to the subclass.
The connector/encoder funcs you do have to pass to
drm_writeback_connector_init() can't use any of the shared driver
infrastructure that assume a certain inheritance.
See also my reply to Laurent [1].
> It well might be that we all misunderstand your design. Do you have a
> complete intel drm_writeback implementation based on this patchset? It
> would be easier to judge if the approach is correct seeing your
> target.
That would be up to Suraj Kandpal.
BR,
Jani.
[1] https://lore.kernel.org/r/87v8xxs2hz.fsf@xxxxxxxxx
>
>>
>> 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.
--
Jani Nikula, Intel Open Source Graphics Center
