| Message ID | 1424956829-22892-10-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 26, 2015 at 03:20:17PM +0200, Tomi Valkeinen wrote: > omapdrm doesn't check if the width of the framebuffer and the color > format's bits-per-pixel match. > > For example, using a display with a width of 1280, and a buffer > allocated with using 32 bits per pixel (i.e. 1280*4 = 5120 bytes), with > a 24 bits per pixel color format, leads to the following mismatch: > 5120/3 = 1706.666... bytes. This causes bad colors and a tilt on the > screen. > > Add a check into omapdrm to return an error if the user tries to use > such a combination. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/omapdrm/omap_fb.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c > index 2975096abdf5..bf98580223d0 100644 > --- a/drivers/gpu/drm/omapdrm/omap_fb.c > +++ b/drivers/gpu/drm/omapdrm/omap_fb.c > @@ -463,6 +463,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev, > goto fail; > } > > + if (mode_cmd->width % format->planes[i].stride_bpp != 0) { width is in pixels. No idea what you're trying to check here, but this probably isn't it. Also drm checks that things fit into the specified pitch (which is in bytes), see the pichtes[i] < width * cpp check in framebuffer_check. Cheers, Daniel > + dev_err(dev->dev, > + "buffer width (%d) is not a multiple of pixel width (%d)\n", > + mode_cmd->width, format->planes[i].stride_bpp); > + ret = -EINVAL; > + goto fail; > + } > + > size = pitch * mode_cmd->height / format->planes[i].sub_y; > > if (size > (omap_gem_mmap_size(bos[i]) - mode_cmd->offsets[i])) { > -- > 2.3.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi, On 27 February 2015 at 13:01, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Feb 26, 2015 at 03:20:17PM +0200, Tomi Valkeinen wrote: >> omapdrm doesn't check if the width of the framebuffer and the color s/width/pitch/ >> format's bits-per-pixel match. s/match/are compatible/ >> For example, using a display with a width of 1280, and a buffer >> allocated with using 32 bits per pixel (i.e. 1280*4 = 5120 bytes), with Might be clearer to say 'i.e. byte stride of ...', and also s/with using/for/. >> a 24 bits per pixel color format, leads to the following mismatch: >> 5120/3 = 1706.666... bytes. This causes bad colors and a tilt on the s/bytes/pixels/ >> screen. >> >> Add a check into omapdrm to return an error if the user tries to use >> such a combination. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> >> --- >> drivers/gpu/drm/omapdrm/omap_fb.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c >> index 2975096abdf5..bf98580223d0 100644 >> --- a/drivers/gpu/drm/omapdrm/omap_fb.c >> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c >> @@ -463,6 +463,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev, >> goto fail; >> } >> >> + if (mode_cmd->width % format->planes[i].stride_bpp != 0) { > > width is in pixels. No idea what you're trying to check here, but this > probably isn't it. stride_bpp is very misnamed: it is the bits per pixel for that plane, and not stride at all. I think the check should in fact be be (pitch % format->planes[i].stride_bpp), which would achieve the desired result, i.e. that the stride can be expressed as an integer number of pixels. > Also drm checks that things fit into the specified pitch (which is in > bytes), see the pichtes[i] < width * cpp check in framebuffer_check. This isn't that check. At some stages, OMAP IIRC requires pitch to be specified in pixels rather than bytes, so this makes sure that's possible to express. Cheers, Daniel
On Fri, Feb 27, 2015 at 02:40:20PM +0000, Daniel Stone wrote: > On 27 February 2015 at 13:01, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Thu, Feb 26, 2015 at 03:20:17PM +0200, Tomi Valkeinen wrote: > >> omapdrm doesn't check if the width of the framebuffer and the color > >> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c > >> index 2975096abdf5..bf98580223d0 100644 > >> --- a/drivers/gpu/drm/omapdrm/omap_fb.c > >> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c > >> @@ -463,6 +463,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev, > >> goto fail; > >> } > >> > >> + if (mode_cmd->width % format->planes[i].stride_bpp != 0) { > > > > width is in pixels. No idea what you're trying to check here, but this > > probably isn't it. > > stride_bpp is very misnamed: it is the bits per pixel for that plane, > and not stride at all. I think the check should in fact be be (pitch % > format->planes[i].stride_bpp), which would achieve the desired result, > i.e. that the stride can be expressed as an integer number of pixels. I meant that mode_cmd->width is in pixels and so totally not what you want to check here. It probably should be mode_cmd->pitches[i]. -Daniel
On 27/02/15 16:40, Daniel Stone wrote: > Hi, > > On 27 February 2015 at 13:01, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Thu, Feb 26, 2015 at 03:20:17PM +0200, Tomi Valkeinen wrote: >>> omapdrm doesn't check if the width of the framebuffer and the color > > s/width/pitch/ > >>> format's bits-per-pixel match. > > s/match/are compatible/ > >>> For example, using a display with a width of 1280, and a buffer >>> allocated with using 32 bits per pixel (i.e. 1280*4 = 5120 bytes), with > > Might be clearer to say 'i.e. byte stride of ...', and also s/with using/for/. Above you said pitch, here you say stride. They are the same thing, right? >>> a 24 bits per pixel color format, leads to the following mismatch: >>> 5120/3 = 1706.666... bytes. This causes bad colors and a tilt on the > > s/bytes/pixels/ > >>> screen. >>> >>> Add a check into omapdrm to return an error if the user tries to use >>> such a combination. >>> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> >>> --- >>> drivers/gpu/drm/omapdrm/omap_fb.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c >>> index 2975096abdf5..bf98580223d0 100644 >>> --- a/drivers/gpu/drm/omapdrm/omap_fb.c >>> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c >>> @@ -463,6 +463,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev, >>> goto fail; >>> } >>> >>> + if (mode_cmd->width % format->planes[i].stride_bpp != 0) { >> >> width is in pixels. No idea what you're trying to check here, but this >> probably isn't it. Yep, I don't know what I was smoking when writing the patch... > stride_bpp is very misnamed: it is the bits per pixel for that plane, > and not stride at all. I think the check should in fact be be (pitch % I don't know why Rob named it like that. "The bpp of the stride"? Shrug. > format->planes[i].stride_bpp), which would achieve the desired result, > i.e. that the stride can be expressed as an integer number of pixels. Yes, that looks correct. >> Also drm checks that things fit into the specified pitch (which is in >> bytes), see the pichtes[i] < width * cpp check in framebuffer_check. > > This isn't that check. At some stages, OMAP IIRC requires pitch to be > specified in pixels rather than bytes, so this makes sure that's > possible to express. Right, that's what this patch was trying to achieve. However... After thinking about this and going through some of the DISPC code, I think that's not a strict requirement. We do calculate all the configs using pixels as units, so at the moment the stride has to be an integer number of pixels. But the hardware actually takes the row-inc and pix-inc as bytes. That said, the HW supports features like rotation and whatnot, and it was not clear with a quick study if there are corner cases where the hardware also requires the stride to be an integer number of pixels. Also, the HW documentation only talks about pixels in this context, even if the final value written to the registers is in bytes. I don't know if that's just to make the documentation simpler, or if there's some reasoning to only use pixel units. So I think for the time being I'll just fix this patch, and look at the possibility of allowing any stride size in the future. Thanks for the review! Tomi
Hi, On 2 March 2015 at 09:50, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > On 27/02/15 16:40, Daniel Stone wrote: >> On 27 February 2015 at 13:01, Daniel Vetter <daniel@ffwll.ch> wrote: >>> On Thu, Feb 26, 2015 at 03:20:17PM +0200, Tomi Valkeinen wrote: >>>> omapdrm doesn't check if the width of the framebuffer and the color >> >> s/width/pitch/ >> >>>> format's bits-per-pixel match. >> >> s/match/are compatible/ >> >>>> For example, using a display with a width of 1280, and a buffer >>>> allocated with using 32 bits per pixel (i.e. 1280*4 = 5120 bytes), with >> >> Might be clearer to say 'i.e. byte stride of ...', and also s/with using/for/. > > Above you said pitch, here you say stride. They are the same thing, right? Yeah, they're interchangeable. In theory, I think it's supposed to be that pitch is in pixels and stride in bpp, but they're so often interchanged that they've lost that distinction. Still, using one consistently is always good - and since KMS uses pitch exclusively, that might be a good one to settle on. >> stride_bpp is very misnamed: it is the bits per pixel for that plane, >> and not stride at all. I think the check should in fact be be (pitch % > > I don't know why Rob named it like that. "The bpp of the stride"? Shrug. It's just the bpp of the pixel format; it's not at all associated with the stride? >> This isn't that check. At some stages, OMAP IIRC requires pitch to be >> specified in pixels rather than bytes, so this makes sure that's >> possible to express. > > Right, that's what this patch was trying to achieve. > > However... After thinking about this and going through some of the DISPC > code, I think that's not a strict requirement. We do calculate all the > configs using pixels as units, so at the moment the stride has to be an > integer number of pixels. But the hardware actually takes the row-inc > and pix-inc as bytes. > > That said, the HW supports features like rotation and whatnot, and it > was not clear with a quick study if there are corner cases where the > hardware also requires the stride to be an integer number of pixels. > Also, the HW documentation only talks about pixels in this context, even > if the final value written to the registers is in bytes. I don't know if > that's just to make the documentation simpler, or if there's some > reasoning to only use pixel units. Indeed that was my impression from a quick look, but my OMAP is extremely rusty these days, so wasn't quite sure ... :) Specifying pixel units isn't totally unheard of, but bytes is definitely more common. Cheers, Daniel
diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c index 2975096abdf5..bf98580223d0 100644 --- a/drivers/gpu/drm/omapdrm/omap_fb.c +++ b/drivers/gpu/drm/omapdrm/omap_fb.c @@ -463,6 +463,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev, goto fail; } + if (mode_cmd->width % format->planes[i].stride_bpp != 0) { + dev_err(dev->dev, + "buffer width (%d) is not a multiple of pixel width (%d)\n", + mode_cmd->width, format->planes[i].stride_bpp); + ret = -EINVAL; + goto fail; + } + size = pitch * mode_cmd->height / format->planes[i].sub_y; if (size > (omap_gem_mmap_size(bos[i]) - mode_cmd->offsets[i])) {
omapdrm doesn't check if the width of the framebuffer and the color format's bits-per-pixel match. For example, using a display with a width of 1280, and a buffer allocated with using 32 bits per pixel (i.e. 1280*4 = 5120 bytes), with a 24 bits per pixel color format, leads to the following mismatch: 5120/3 = 1706.666... bytes. This causes bad colors and a tilt on the screen. Add a check into omapdrm to return an error if the user tries to use such a combination. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/omapdrm/omap_fb.c | 8 ++++++++ 1 file changed, 8 insertions(+)