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@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. 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
