| Message ID | 1461702945-14185-6-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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
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 ?
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; }
Checks can be simplified based on the requirement that pitches must be identical for all planes. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/gpu/drm/omapdrm/omap_fb.c | 51 ++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 25 deletions(-)