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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Tomi,

On Monday 09 May 2016 18:15:10 Tomi Valkeinen wrote:
> 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.

I'll update the commit message and add a comment to the code.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@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.

Given that the code is currently wrong anyway, how about implementing correct 
generic support for formats with more than two planes when hardware supporting 
those formats appear ?

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel