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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel