Re: [PATCH v2 2/2] drm/vkms: return early if compose_plane fails

[Melissa Wen <mwen@igalia.com>]

[-- Attachment #1: Type: text/plain, Size: 3655 bytes --]

On 04/15, Tales Lelo da Aparecida wrote:
> Do not exit quietly from compose_plane. If any plane has an invalid
> map, propagate the error value upwards. While here, add log messages
> for the invalid index.
> 
> Signed-off-by: Tales Lelo da Aparecida <tales.aparecida@gmail.com>
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c | 30 ++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index b47ac170108c..c0a3b53cd155 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -149,16 +149,16 @@ static void blend(void *vaddr_dst, void *vaddr_src,
>  	}
>  }
>  
> -static void compose_plane(struct vkms_composer *primary_composer,
> -			  struct vkms_composer *plane_composer,
> -			  void *vaddr_out)
> +static int compose_plane(struct vkms_composer *primary_composer,
> +			 struct vkms_composer *plane_composer,
> +			 void *vaddr_out)
>  {
>  	struct drm_framebuffer *fb = &plane_composer->fb;
>  	void *vaddr;
>  	void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
>  
>  	if (WARN_ON(iosys_map_is_null(&plane_composer->map[0])))
> -		return;
> +		return -EINVAL;
>  
>  	vaddr = plane_composer->map[0].vaddr;
>  
> @@ -168,6 +168,8 @@ static void compose_plane(struct vkms_composer *primary_composer,
>  		pixel_blend = &x_blend;
>  
>  	blend(vaddr_out, vaddr, primary_composer, plane_composer, pixel_blend);
> +
> +	return 0;
>  }
>  
>  static int compose_active_planes(void **vaddr_out,
> @@ -177,7 +179,7 @@ static int compose_active_planes(void **vaddr_out,
>  	struct drm_framebuffer *fb = &primary_composer->fb;
>  	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
>  	const void *vaddr;
> -	int i;
> +	int i, ret;
>  
>  	if (!*vaddr_out) {
>  		*vaddr_out = kzalloc(gem_obj->size, GFP_KERNEL);
> @@ -187,8 +189,10 @@ static int compose_active_planes(void **vaddr_out,
>  		}
>  	}
>  
> -	if (WARN_ON(iosys_map_is_null(&primary_composer->map[0])))
> +	if (WARN_ON(iosys_map_is_null(&primary_composer->map[0]))) {
> +		DRM_DEBUG_DRIVER("Failed to compose. Invalid map in the primary plane.");
>  		return -EINVAL;
yes, good to have a debug msg here. I would say `mapping`
> +	}
>  
>  	vaddr = primary_composer->map[0].vaddr;
>  
> @@ -198,10 +202,16 @@ static int compose_active_planes(void **vaddr_out,
>  	 * planes should be in z-order and compose them associatively:
>  	 * ((primary <- overlay) <- cursor)
>  	 */
> -	for (i = 1; i < crtc_state->num_active_planes; i++)
> -		compose_plane(primary_composer,
> -			      crtc_state->active_planes[i]->composer,
> -			      *vaddr_out);
> +	for (i = 1; i < crtc_state->num_active_planes; i++) {
> +		ret = compose_plane(primary_composer,
> +				    crtc_state->active_planes[i]->composer,
> +				    *vaddr_out);

tbh, I'm not sure on changing compose_plane behaviour here to
invalidate all composition in case of a invalid active plane mapping.

Warning about an inconsistent composition makes sense to me, but it
would be better if we can prevent all resources consumption around each plane
composition by checking the issue as soon as possible. One idea would be doing
this validation at the time of active_plane selection. Another would be just
check all active_plane mapping right before this loop.

What do you think?

> +		if (ret) {
> +			DRM_DEBUG_DRIVER("Failed to compose. Invalid map in the active_planes[%d].",
> +					 i);
> +			return ret;
> +		}
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.35.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]