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

[Danilo Krummrich <dakr@redhat.com>]

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@redhat.com>
>> ---
>>   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
>>
>