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 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.
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
