Re: [PATCH RESEND drm-misc-next 4/7] drm/arm/hdlcd: plane: use drm managed resources

[Danilo Krummrich]

Hi Liviu,

Thanks for having a look!

This is not about this patch, it's about patch 3/7 "drm/arm/hdlcd: crtc: 
use drmm_crtc_init_with_planes()".

And there it's the other way around. When using 
drmm_crtc_init_with_planes() we shouldn't have a destroy hook in place, 
that's the whole purpose of drmm_crtc_init_with_planes().

We should just drop patch 3/7 "drm/arm/hdlcd: crtc: use 
drmm_crtc_init_with_planes()", it's wrong.

Do you want me to send a v2 for that?

- Danilo



On 9/12/22 19:36, Liviu Dudau wrote:
> Hi Danilo,
>
> I have applied your patch series for HDLCD on top of drm-next (commit 213cb76ddc8b)
> and on start up I get a warning:
>
> [   12.882554] hdlcd 7ff50000.hdlcd: drm_WARN_ON(funcs && funcs->destroy)
> [   12.882596] WARNING: CPU: 1 PID: 211 at drivers/gpu/drm/drm_crtc.c:393 __drmm_crtc_init_with_planes+0x70/0xf0 [drm]
>
> It looks like the .destroy hook is still required or I'm missing some other required
> series where the WARN has been removed?
>
> Best regards,
> Liviu
>
> On Mon, Sep 05, 2022 at 05:27:16PM +0200, Danilo Krummrich wrote:
> > Use drm managed resource allocation (drmm_universal_plane_alloc()) in
> > order to get rid of the explicit destroy hook in struct drm_plane_funcs.
> >
> > Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/arm/hdlcd_crtc.c | 20 +++++++-------------
> >   1 file changed, 7 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
> > index c0a5ca7f578a..17d3ccf12245 100644
> > --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> > +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> > @@ -289,7 +289,6 @@ static const struct drm_plane_helper_funcs hdlcd_plane_helper_funcs = {
> >   static const struct drm_plane_funcs hdlcd_plane_funcs = {
> >   	.update_plane		= drm_atomic_helper_update_plane,
> >   	.disable_plane		= drm_atomic_helper_disable_plane,
> > -	.destroy		= drm_plane_cleanup,
> >   	.reset			= drm_atomic_helper_plane_reset,
> >   	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> >   	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
> > @@ -297,24 +296,19 @@ static const struct drm_plane_funcs hdlcd_plane_funcs = {
> >   
> >   static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
> >   {
> > -	struct hdlcd_drm_private *hdlcd = drm->dev_private;
> > +	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
> >   	struct drm_plane *plane = NULL;
> >   	u32 formats[ARRAY_SIZE(supported_formats)], i;
> > -	int ret;
> > -
> > -	plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
> > -	if (!plane)
> > -		return ERR_PTR(-ENOMEM);
> >   
> >   	for (i = 0; i < ARRAY_SIZE(supported_formats); i++)
> >   		formats[i] = supported_formats[i].fourcc;
> >   
> > -	ret = drm_universal_plane_init(drm, plane, 0xff, &hdlcd_plane_funcs,
> > -				       formats, ARRAY_SIZE(formats),
> > -				       NULL,
> > -				       DRM_PLANE_TYPE_PRIMARY, NULL);
> > -	if (ret)
> > -		return ERR_PTR(ret);
> > +	plane = drmm_universal_plane_alloc(drm, struct drm_plane, dev, 0xff,
> > +					   &hdlcd_plane_funcs,
> > +					   formats, ARRAY_SIZE(formats),
> > +					   NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> > +	if (IS_ERR(plane))
> > +		return plane;
> >   
> >   	drm_plane_helper_add(plane, &hdlcd_plane_helper_funcs);
> >   	hdlcd->plane = plane;
> > --
> > 2.37.2