RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support

From: Biju Das <biju.das.jz@bp.renesas.com>
To: Maxime Ripard <mripard@kernel.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>,
	Jacopo Mondi <jacopo.mondi@ideasonboard.com>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Magnus Damm <magnus.damm@gmail.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"linux-renesas-soc@vger.kernel.org"
	<linux-renesas-soc@vger.kernel.org>,
	Fabrizio Castro <fabrizio.castro.jz@renesas.com>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	biju.das.au <biju.das.au@gmail.com>
Subject: RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Date: Fri, 15 Dec 2023 11:19:16 +0000	[thread overview]
Message-ID: <TYCPR01MB11269E63B4CBB97576DBF03F68693A@TYCPR01MB11269.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <jxjqwytay24p5g7rl23asjucbwswpjyw3znrbu6z7eigrvl7jy@5vxwqtgpg35g>

Hi Maxime Ripard,

> -----Original Message-----
> From: Maxime Ripard <mripard@kernel.org>
> Sent: Friday, December 15, 2023 9:25 AM
> Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> 
> On Fri, Dec 15, 2023 at 07:47:07AM +0000, Biju Das wrote:
> > Hi Maxime Ripard,
> >
> > > -----Original Message-----
> > > From: Biju Das
> > > Sent: Thursday, December 14, 2023 8:50 PM
> > > Subject: RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> > >
> > > Hi Maxime Ripard,
> > >
> > >
> > > > -----Original Message-----
> > > > From: Biju Das
> > > > Sent: Thursday, December 14, 2023 3:24 PM
> > > > Subject: RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> > > >
> > > > >
> > > > > > +
> > > > > > +	for (i = 0; i < num_planes; ++i) {
> > > > > > +		enum drm_plane_type type = i < num_crtcs
> > > > > > +					 ? DRM_PLANE_TYPE_PRIMARY
> > > > > > +					 : DRM_PLANE_TYPE_OVERLAY;
> > > > > > +		struct rzg2l_du_vsp_plane *plane = &vsp->planes[i];
> > > > > > +
> > > > > > +		plane->vsp = vsp;
> > > > > > +		plane->index = i;
> > > > > > +		ret = drm_universal_plane_init(&rcdu->ddev, &plane-
> > > >plane,
> > > > > > +					       crtcs,
> > > &rzg2l_du_vsp_plane_funcs,
> > > > > > +					       rzg2l_du_vsp_formats,
> > > > > > +
> > > ARRAY_SIZE(rzg2l_du_vsp_formats),
> > > > > > +					       NULL, type, NULL);
> > > > > > +		if (ret < 0)
> > > > > > +			return ret;
> > > > >
> > > > > you need to use drmm variant here too.
> > > >
> > > > I did rebased to latest drm_misc_next and I don't find the
> > > > drmm_universal_plane_init()
> > > >
> > > > Can you please point me to the API?
> > >
> > > We cannot use drmm_universal_plane_alloc() in this architecture.
> > >
> > > rzg2l_du_vsps_init() stores the VSP pointer and pipe index from DT
> first.
> > >
> > > Then all the planes are created using rzg2l_du_vsp_init()
> > >
> > > CRTC uses VSP pointer and pipe_index to set the
> > > plane(rzg2l_du_crtc_create()).
> > >
> > > CRTC->vsp->planes[rcrtc->vsp_pipe].plane
> > >
> >
> > Thinking again, it is possible to use drmm_universal_plane_alloc()
> > here
> >
> > Introduce a linked list [1] and use an API [2] to retrieve plane by
> > matching vsp_pipe index against plane->index.
> >
> > With this we don't need drmm_kcalloc() any more.
> >
> > I will implement and test this and send v16, if there are no comments.
> >
> >
> > [1]
> > struct rzg2l_du_vsp_plane {
> > 	struct drm_plane plane;
> > 	struct rzg2l_du_vsp *vsp;
> > 	unsigned int index;
> > 	struct rzg2l_du_vsp_plane *next;
> > };
> >
> > [2]
> > struct drm_plane *rzg2l_du_vsp_get_drm_plane(struct rzg2l_du_crtc *crtc,
> > 					 unsigned int pipe_index);
> 
> Why do you need a linked list for? There's one already: the DRM device
> will store all the planes registered in the DRM device, and you can
> already iterate over all the planes with drm_for_each_plane, or
> drm_for_each_plane_mask if you want to reduce the scope of the iterator.

Thanks for the pointer, it is simple now.

Vsp->planes[] aswell as linked list is not required using drm_for_each_plane()

Cheers,
Biju