[PATCH 05/23] drm: omapdrm: fb: Simplify mode command checks when creating framebuffer

[Tomi Valkeinen]

On 26/04/16 23:35, Laurent Pinchart wrote:
> Checks can be simplified based on the requirement that pitches must be
> identical for all planes.

Your code also presumes there are only 1 or 2 planes, I think that
should be mentioned too.

> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_fb.c | 51 ++++++++++++++++++++-------------------
>  1 file changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
> index 015b6a50c581..8629ba6ff9d7 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -407,6 +407,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>  	struct omap_framebuffer *omap_fb = NULL;
>  	struct drm_framebuffer *fb = NULL;
>  	const struct format *format = NULL;
> +	unsigned int pitch = mode_cmd->pitches[0];
>  	int ret, i;
>  
>  	DBG("create framebuffer: dev=%p, mode_cmd=%p (%dx%d@%4.4s)",
> @@ -437,38 +438,38 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>  	omap_fb->format = format;
>  	mutex_init(&omap_fb->lock);
>  
> -	for (i = 0; i < format->num_planes; i++) {
> -		struct plane *plane = &omap_fb->planes[i];
> -		int size, pitch = mode_cmd->pitches[i];
> +	if (format->num_planes == 2 && pitch != mode_cmd->pitches[1]) {
> +		dev_err(dev->dev, "pitches differ between planes 0 and 1\n");
> +		ret = -EINVAL;
> +		goto fail;
> +	}
>  
> -		if (pitch < (mode_cmd->width * format->stride_bpp)) {
> -			dev_err(dev->dev, "provided buffer pitch is too small! %d < %d\n",
> -					pitch, mode_cmd->width * format->stride_bpp);
> -			ret = -EINVAL;
> -			goto fail;
> -		}
> +	if (pitch < mode_cmd->width * format->stride_bpp) {
> +		dev_err(dev->dev,
> +			"provided buffer pitch is too small! %u < %u\n",
> +			pitch, mode_cmd->width * format->stride_bpp);
> +		ret = -EINVAL;
> +		goto fail;
> +	}
>  
> -		if (pitch % format->stride_bpp != 0) {
> -			dev_err(dev->dev,
> -				"buffer pitch (%d bytes) is not a multiple of pixel size (%d bytes)\n",
> -				pitch, format->stride_bpp);
> -			ret = -EINVAL;
> -			goto fail;
> -		}
> +	if (pitch % format->stride_bpp != 0) {
> +		dev_err(dev->dev,
> +			"buffer pitch (%u bytes) is not a multiple of pixel size (%u bytes)\n",
> +			pitch, format->stride_bpp);
> +		ret = -EINVAL;
> +		goto fail;
> +	}
> +
> +	for (i = 0; i < format->num_planes; i++) {
> +		struct plane *plane = &omap_fb->planes[i];
> +		unsigned int size;
>  
>  		size = pitch * mode_cmd->height / format->sub_y[i];
>  
>  		if (size > (omap_gem_mmap_size(bos[i]) - mode_cmd->offsets[i])) {
> -			dev_err(dev->dev, "provided buffer object is too small! %d < %d\n",
> -					bos[i]->size - mode_cmd->offsets[i], size);
> -			ret = -EINVAL;
> -			goto fail;
> -		}
> -
> -		if (i > 0 && pitch != mode_cmd->pitches[i - 1]) {
>  			dev_err(dev->dev,
> -				"pitches are not the same between framebuffer planes %d != %d\n",
> -				pitch, mode_cmd->pitches[i - 1]);
> +				"provided buffer object is too small! %d < %d\n",
> +				bos[i]->size - mode_cmd->offsets[i], size);
>  			ret = -EINVAL;
>  			goto fail;
>  		}

So, hmm... I think the current code is actually not right, even if it
works right: I think the DSS's requirement is actually that the width in
pixels is the same between planes, not stride.

At the moment the only two plane format, NV12, has the same pixel size
for both planes, but an upcoming DSS might have a format that has a
separate 8 byte A plane. I don't know if that ever realizes or if we
want to support the mode, but after thinking about this, it makes more
sense that the pixel width has to be the same between planes, not the
byte width.

 Tomi

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20160509/814e1b35/attachment.sig>