Re: [Intel-gfx] [PATCH v2 1/3] drm: Make sure at least one plane supports the fb format

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Mar 05, 2018 at 11:41:02PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> To make life easier for drivers, let's have the core check that the
> requested pixel format is supported by at least one plane when creating
> a new framebuffer.
> 
> v2: Accept anyformat if the driver doesn't do planes (Eric)
>     s/planes_have_format/any_plane_has_format/ (Eric)
>     Check the modifier as well since we already have a function
>     that does both
> 
> Cc: Eric Anholt <eric@anholt.net>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_framebuffer.c | 43 +++++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index c0530a1af5e3..b9a33e3f13e9 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -152,12 +152,37 @@ static int fb_plane_height(int height,
>  	return DIV_ROUND_UP(height, format->vsub);
>  }
>  
> +static bool any_plane_has_format(struct drm_device *dev,
> +				 u32 format, u64 modifier)
> +{
> +	struct drm_plane *plane;
> +
> +	/* TODO: maybe maintain a device level format list? */
> +	drm_for_each_plane(plane, dev) {
> +		/*
> +		 * In case the driver doesn't really do
> +		 * planes we have to accept any format here.
> +		 */
> +		if (plane->format_default)
> +			return true;
> +
> +		if (drm_plane_check_pixel_format(plane, format, modifier) == 0)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  static int framebuffer_check(struct drm_device *dev,
>  			     const struct drm_mode_fb_cmd2 *r)
>  {
>  	const struct drm_format_info *info;
> +	u64 modifier = 0;
>  	int i;
>  
> +	if (r->flags & DRM_MODE_FB_MODIFIERS)
> +		modifier = r->modifier[0];

And of course this isn't quite correct. For the !modifiers case we're
now checking if any plane supports the format with the linear modifier,
but the actual modifier will only be picked by the driver later.

Not quite sure how I would write this in a way that really makes
sense. Might have to make it the driver's responsibility to call
any_plane_has_format() instead...

> +
>  	/* check if the format is supported at all */
>  	info = __drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
>  	if (!info) {
> @@ -168,6 +193,15 @@ static int framebuffer_check(struct drm_device *dev,
>  		return -EINVAL;
>  	}
>  
> +	if (!any_plane_has_format(dev, r->pixel_format, modifier)) {
> +		struct drm_format_name_buf format_name;
> +
> +		DRM_DEBUG_KMS("unsupported pixel format %s, modifier 0x%llx\n",
> +			      drm_get_format_name(r->pixel_format, &format_name),
> +			      modifier);
> +		return -EINVAL;
> +	}
> +
>  	/* now let the driver pick its own format info */
>  	info = drm_get_format_info(dev, r);
>  
> @@ -202,14 +236,7 @@ static int framebuffer_check(struct drm_device *dev,
>  			return -EINVAL;
>  		}
>  
> -		if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) {
> -			DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
> -				      r->modifier[i], i);
> -			return -EINVAL;
> -		}
> -
> -		if (r->flags & DRM_MODE_FB_MODIFIERS &&
> -		    r->modifier[i] != r->modifier[0]) {
> +		if (r->modifier[i] != modifier) {
>  			DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
>  				      r->modifier[i], i);
>  			return -EINVAL;
> -- 
> 2.16.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel