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.
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.
>
> 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.
--
With best wishes
Dmitry
