| Message ID | 20200806021807.21863-5-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | kmsxx: Various fixes and improvements | expand |
On 06/08/2020 05:18, Laurent Pinchart wrote: > The BO pitches are unconditionally set to the frame buffer pitch, for > all planes. This is correct for semiplanar YUV formats, as they > subsample chroma horizontally by two but combined U and V in a single > plane, cancelling each other. For fully planar YUV formats, however, the > horizontal subsampling need to be taken into account to compute the > pitch. Fix it. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > kms++/src/dumbframebuffer.cpp | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/kms++/src/dumbframebuffer.cpp b/kms++/src/dumbframebuffer.cpp > index 18f3f152943d..4c3c03164a90 100644 > --- a/kms++/src/dumbframebuffer.cpp > +++ b/kms++/src/dumbframebuffer.cpp > @@ -42,6 +42,14 @@ DumbFramebuffer::DumbFramebuffer(Card& card, uint32_t width, uint32_t height, Pi > struct drm_mode_create_dumb creq = drm_mode_create_dumb(); > creq.width = width; > creq.height = height / pi.ysub; > + /* > + * For fully planar YUV buffers, the chroma planes don't combine > + * U and V components, their width must thus be divided by the > + * horizontal subsampling factor. > + */ > + if (format_info.type == PixelColorType::YUV && > + format_info.num_planes == 3) > + creq.width /= pi.xsub; This feels a bit of a hack... I think we should somehow have the relevant information in the PixelFormatInfo. Having the same plane info, { 8, 2, 2 }, for both NV12 UV plane and YUV420 U and V planes doesn't sound correct. Should NV12's UV plane be { 16, 2, 2 }? Subsampled formats are confusing... =) Tomi
Hi Tomi, On Thu, Aug 06, 2020 at 12:21:39PM +0300, Tomi Valkeinen wrote: > On 06/08/2020 05:18, Laurent Pinchart wrote: > > The BO pitches are unconditionally set to the frame buffer pitch, for > > all planes. This is correct for semiplanar YUV formats, as they > > subsample chroma horizontally by two but combined U and V in a single > > plane, cancelling each other. For fully planar YUV formats, however, the > > horizontal subsampling need to be taken into account to compute the > > pitch. Fix it. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > kms++/src/dumbframebuffer.cpp | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/kms++/src/dumbframebuffer.cpp b/kms++/src/dumbframebuffer.cpp > > index 18f3f152943d..4c3c03164a90 100644 > > --- a/kms++/src/dumbframebuffer.cpp > > +++ b/kms++/src/dumbframebuffer.cpp > > @@ -42,6 +42,14 @@ DumbFramebuffer::DumbFramebuffer(Card& card, uint32_t width, uint32_t height, Pi > > struct drm_mode_create_dumb creq = drm_mode_create_dumb(); > > creq.width = width; > > creq.height = height / pi.ysub; > > + /* > > + * For fully planar YUV buffers, the chroma planes don't combine > > + * U and V components, their width must thus be divided by the > > + * horizontal subsampling factor. > > + */ > > + if (format_info.type == PixelColorType::YUV && > > + format_info.num_planes == 3) > > + creq.width /= pi.xsub; > > This feels a bit of a hack... I think we should somehow have the > relevant information in the PixelFormatInfo. Having the same plane > info, { 8, 2, 2 }, for both NV12 UV plane and YUV420 U and V planes > doesn't sound correct. > > Should NV12's UV plane be { 16, 2, 2 }? Subsampled formats are > confusing... =) I'll give it a try. I however wonder if all drivers will expect 16bpp. The ones based on drm_gem_cma_dumb_create() will be fine, but other drivers may not expect this.
On 09/08/2020 01:14, Laurent Pinchart wrote: > Hi Tomi, > > On Thu, Aug 06, 2020 at 12:21:39PM +0300, Tomi Valkeinen wrote: >> On 06/08/2020 05:18, Laurent Pinchart wrote: >>> The BO pitches are unconditionally set to the frame buffer pitch, for >>> all planes. This is correct for semiplanar YUV formats, as they >>> subsample chroma horizontally by two but combined U and V in a single >>> plane, cancelling each other. For fully planar YUV formats, however, the >>> horizontal subsampling need to be taken into account to compute the >>> pitch. Fix it. >>> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> kms++/src/dumbframebuffer.cpp | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/kms++/src/dumbframebuffer.cpp b/kms++/src/dumbframebuffer.cpp >>> index 18f3f152943d..4c3c03164a90 100644 >>> --- a/kms++/src/dumbframebuffer.cpp >>> +++ b/kms++/src/dumbframebuffer.cpp >>> @@ -42,6 +42,14 @@ DumbFramebuffer::DumbFramebuffer(Card& card, uint32_t width, uint32_t height, Pi >>> struct drm_mode_create_dumb creq = drm_mode_create_dumb(); >>> creq.width = width; >>> creq.height = height / pi.ysub; >>> + /* >>> + * For fully planar YUV buffers, the chroma planes don't combine >>> + * U and V components, their width must thus be divided by the >>> + * horizontal subsampling factor. >>> + */ >>> + if (format_info.type == PixelColorType::YUV && >>> + format_info.num_planes == 3) >>> + creq.width /= pi.xsub; >> >> This feels a bit of a hack... I think we should somehow have the >> relevant information in the PixelFormatInfo. Having the same plane >> info, { 8, 2, 2 }, for both NV12 UV plane and YUV420 U and V planes >> doesn't sound correct. >> >> Should NV12's UV plane be { 16, 2, 2 }? Subsampled formats are >> confusing... =) > > I'll give it a try. I however wonder if all drivers will expect 16bpp. > The ones based on drm_gem_cma_dumb_create() will be fine, but other > drivers may not expect this. Oh, right, that number is passed to DRM_IOCTL_MODE_CREATE_DUMB. I was only thinking about how kms++ handles these internally. To be honest, I don't even quite know how subsampled formats are supposed to be handled in DRM. Above we pass width as it is, but divide height by ysub. And then we tune the bpp adjust to the fact that we didn't divide the width. For e.g. YUYV, the bpp tells the container size. But for NV12's second plane, bpp is not the container size. Well, I think at least omapdrm doesn't care, it just allocates the number of bytes according to w*h*bpp. But maybe it would be good to have a clear rule how to represent these in kms++, and then if DRM wants the values in some other way, adjust the values according to the format. Or maybe we already have all the numbers according to a clear rule, I'm just not sure what the rule is =). Tomi
On 10/08/2020 09:16, Tomi Valkeinen wrote: > To be honest, I don't even quite know how subsampled formats are supposed to be handled in DRM. > Above we pass width as it is, but divide height by ysub. And then we tune the bpp adjust to the fact > that we didn't divide the width. > > For e.g. YUYV, the bpp tells the container size. But for NV12's second plane, bpp is not the > container size. Ah, no it isn't... Not enough coffee yet. So the bpp for YUYV and NV12 is the "effective bpp", how many bits are consumed on that plane for each real pixel. Then, shouldn't bpp for YUV420's second and third plane be 4? Tomi
diff --git a/kms++/src/dumbframebuffer.cpp b/kms++/src/dumbframebuffer.cpp index 18f3f152943d..4c3c03164a90 100644 --- a/kms++/src/dumbframebuffer.cpp +++ b/kms++/src/dumbframebuffer.cpp @@ -42,6 +42,14 @@ DumbFramebuffer::DumbFramebuffer(Card& card, uint32_t width, uint32_t height, Pi struct drm_mode_create_dumb creq = drm_mode_create_dumb(); creq.width = width; creq.height = height / pi.ysub; + /* + * For fully planar YUV buffers, the chroma planes don't combine + * U and V components, their width must thus be divided by the + * horizontal subsampling factor. + */ + if (format_info.type == PixelColorType::YUV && + format_info.num_planes == 3) + creq.width /= pi.xsub; creq.bpp = pi.bitspp; r = drmIoctl(card.fd(), DRM_IOCTL_MODE_CREATE_DUMB, &creq); if (r)
The BO pitches are unconditionally set to the frame buffer pitch, for all planes. This is correct for semiplanar YUV formats, as they subsample chroma horizontally by two but combined U and V in a single plane, cancelling each other. For fully planar YUV formats, however, the horizontal subsampling need to be taken into account to compute the pitch. Fix it. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- kms++/src/dumbframebuffer.cpp | 8 ++++++++ 1 file changed, 8 insertions(+)