Re: [PATCH v3 02/15] drm: Centralize format information

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

Hello Daniel and Ville,

On Thursday 09 Jun 2016 17:13:15 Ville Syrjälä wrote:
> On Thu, Jun 09, 2016 at 03:29:10PM +0200, Daniel Vetter wrote:
> > On Thu, Jun 09, 2016 at 04:05:11PM +0300, Ville Syrjälä wrote:
> >> On Thu, Jun 09, 2016 at 02:40:28PM +0200, Daniel Vetter wrote:
> >>> On Thu, Jun 09, 2016 at 03:23:17PM +0300, Ville Syrjälä wrote:
> >>>> On Thu, Jun 09, 2016 at 10:52:23AM +0200, Daniel Vetter wrote:
> >>>>> On Thu, Jun 09, 2016 at 02:32:06AM +0300, Laurent Pinchart wrote:
> >>>>>> Various pieces of information about DRM formats (number of
> >>>>>> planes, color depth, chroma subsampling, ...) are scattered
> >>>>>> across different helper functions in the DRM core. Callers of
> >>>>>> those functions often need to access more than a single
> >>>>>> parameter of the format, leading to
> >>>>>> inefficiencies due to multiple lookups.
> >>>>>> 
> >>>>>> Centralize all format information in a data structure and create
> >>>>>> a function to look up information based on the format 4CC.
> >>>>>> 
> >>>>>> Signed-off-by: Laurent Pinchart
> >>>>>> <laurent.pinchart@ideasonboard.com>
> >>>>>> ---
> >>>>>> 
> >>>>>>  drivers/gpu/drm/drm_fourcc.c | 84 ++++++++++++++++++++++++++++
> >>>>>>  include/drm/drm_fourcc.h     | 19 ++++++++++
> >>>>>>  2 files changed, 103 insertions(+)
> >>>>>> 
> >>>>>> diff --git a/drivers/gpu/drm/drm_fourcc.c
> >>>>>> b/drivers/gpu/drm/drm_fourcc.c
> >>>>>> index 0645c85d5f95..47b9abd743be 100644
> >>>>>> --- a/drivers/gpu/drm/drm_fourcc.c
> >>>>>> +++ b/drivers/gpu/drm/drm_fourcc.c
> >>>>>> @@ -62,6 +62,90 @@ const char *drm_get_format_name(uint32_t
> >>>>>> format)
> >>>>>> 
> >>>>>>  EXPORT_SYMBOL(drm_get_format_name);
> >>>>>>  
> >>>>>>  /**
> >>>>>> + * drm_format_info - query information for a given format
> >>>>>> + * @format: pixel format (DRM_FORMAT_*)
> >>>>>> + *
> >>>>>> + * Returns:
> >>>>>> + * The instance of struct drm_format_info that describes the pixel
> >>>>>> format, or
> >>>>>> + * NULL if the format is unsupported.
> >>>>>> + */
> >>>>>> +const struct drm_format_info *drm_format_info(u32 format)
> >>>>> 
> >>>>> Bikeshed on your pixel format description table.

As much as I like a good bikeshed myself, we haven't reached a conclusion 
here, so I'll keep the current approach until someone proposes something 
better :-)

> >>>>> I think the
> >>>>> approach I've seen in gallium/mesa to describe pixel formats is a
> >>>>> lot more generic, and we might as well adopt it when we change.
> >>>>> Idea is to have a block size measure in pixels (both h and v), and
> >>>>> then cpp is bytes_per_block. This is essentially what you have
> >>>>> with hsub and vsub already, except confusing names, more ill-
> >>>>> defined (since it only makes sense for yuv) and less generic. A
> >>>>> few examples:
> >>>>
> >>>> I think you have your confusion backwards. Calling something a block
> >>>> in planar formats would be more confusing. The only thing that
> >>>> really matters is the relative position of the samples between the
> >>>> planes. So there really is no "block" in there.
> >>> 
> >>> Atm U/V planes have a cpp of 1, which is definitely not true. There's
> >>> much less than 1 byte per visible pixel in those planes. And that's
> >>> the part that annoys me.
> >> 
> >> That's exactly as it should be. The cpp value isn't some average thing
> >> for the whole, it's per-plane.
> > 
> > On a 4x subsampled U or V plane you have 1 byte for 4 pixels. That's just
> > plain not 1 character-per-pixel, even when you just look at that plane.
> 
> OK. So let's stop calling it a pixel and call it a sample instead.
> It's 1 byte per sample, which is the only thing that should matter
> to anyone.
> 
> >>> block here is an entirely free-standing concept that just means "group
> >>> of pixels over which the bytes-per-group is counted in each group".
> >>> It's a concept stolen from gallium and makes a lot more sense when
> >>> talking about compressed formats. But I think it also makes sense when
> >>> talking about yuv formats.
> >> 
> >> For packed YUV formats the usual term I've heard is macropixel, and
> >> there it does make sense. I could live with calling it a block. So I
> >> guess eg. for 422 packed formats we'd have h_block_size=2
> >> v_block_size=1, and bytes_per_block=4.
> >> 
> >> For planar formats, each plane should be considered individually,
> >> and trying to come up with some kind of cpp value etc. for the whole
> >> thing is pointless. I think eg. with for all the NVxx formats the
> >> chroma plane should have h_block_size=2 v_block_size=1 bytes_per_block=2
> >> regardless the sub-sampling factor.
> 
> Actually meant to write h_block_size=1 here obviously. 1 sample of
> each chroma component, 2 bytes in total.
> 
> > Hm yeah NV12 is a case where 2 have 2 bytes per 2 pixel block (since
> > they're together in 1 2ndary plane), but still a subsampling of 2x. Maybe
> > we'd need both? And then perhaps define subsampling per color channel, but
> > block size and bytes-per-block per plane?
> 
> Well given all the formats we have today, it's always chroma that's
> sub-sampled. I guess if we were to have a more complicated mix of
> planes that may or may not be sub-sampled then it might make sense to
> define things in a more complicated way. Right now I don't see any
> need for that though.

-- 
Regards,

Laurent Pinchart

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