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@xxxxxxxxxxxxxxxx> > > > > --- > > > > 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. > > 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. So if we start using the block size concept, I think that too should be per-plane. Anything else will just get really confusing. -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
