On Wed, 02 Feb 2022, Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
On Wed, 2 Feb 2022 at 16:15, Jani Nikula jani.nikula@intel.com wrote:
On Wed, 02 Feb 2022, Laurent Pinchart laurent.pinchart@ideasonboard.com 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@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.
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@intel.com
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.