Re: [PATCH v5 1/8] drm/fourcc: Add modifier definitions for describing Amlogic Video Framebuffer Compression

[Neil Armstrong]

On 17/04/2020 17:07, Daniel Vetter wrote:
> On Thu, Apr 16, 2020 at 05:24:53PM +0200, Neil Armstrong wrote:
>> Amlogic uses a proprietary lossless image compression protocol and format
>> for their hardware video codec accelerators, either video decoders or
>> video input encoders.
>>
>> It considerably reduces memory bandwidth while writing and reading
>> frames in memory.
>>
>> The underlying storage is considered to be 3 components, 8bit or 10-bit
>> per component, YCbCr 420, single plane :
>> - DRM_FORMAT_YUV420_8BIT
>> - DRM_FORMAT_YUV420_10BIT
>>
>> This modifier will be notably added to DMA-BUF frames imported from the V4L2
>> Amlogic VDEC decoder.
>>
>> This introduces the basic layout composed of:
>> - a body content organized in 64x32 superblocks with 4096 bytes per
>>   superblock in default mode.
>> - a 32 bytes per 128x64 header block
>>
>> This layout is tranferrable between Amlogic SoCs supporting this modifier.
>>
>> Tested-by: Kevin Hilman <khilman@xxxxxxxxxxxx>
>> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>> ---
>>  include/uapi/drm/drm_fourcc.h | 39 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>>
>> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
>> index 8bc0b31597d8..a1b163a5641f 100644
>> --- a/include/uapi/drm/drm_fourcc.h
>> +++ b/include/uapi/drm/drm_fourcc.h
>> @@ -309,6 +309,7 @@ extern "C" {
>>  #define DRM_FORMAT_MOD_VENDOR_BROADCOM 0x07
>>  #define DRM_FORMAT_MOD_VENDOR_ARM     0x08
>>  #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09
>> +#define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a
>>  
>>  /* add more to the end as needed */
>>  
>> @@ -804,6 +805,44 @@ extern "C" {
>>   */
>>  #define DRM_FORMAT_MOD_ALLWINNER_TILED fourcc_mod_code(ALLWINNER, 1)
>>  
>> +/*
>> + * Amlogic Video Framebuffer Compression modifiers
>> + *
>> + * Amlogic uses a proprietary lossless image compression protocol and format
>> + * for their hardware video codec accelerators, either video decoders or
>> + * video input encoders.
>> + *
>> + * It considerably reduces memory bandwidth while writing and reading
>> + * frames in memory.
>> + *
>> + * The underlying storage is considered to be 3 components, 8bit or 10-bit
>> + * per component YCbCr 420, single plane :
>> + * - DRM_FORMAT_YUV420_8BIT
>> + * - DRM_FORMAT_YUV420_10BIT
>> + *
>> + * The first 8 bits of the mode defines the layout, then the following 8 bits
>> + * defines the options changing the layout.
> 
> None of the modifiers you're doing seem to have these other 8 bits
> defined anywhere. And it's not encoded in your modifiers. Can't we just
> enumerate the ones we have/need and done?

It's introduced in patch 5

> 
>> + *
>> + * Not all combinations are valid, and different SoCs may support different
>> + * combinations of layout and options.
>> + */
>> +#define DRM_FORMAT_MOD_AMLOGIC_FBC(__modes) fourcc_mod_code(AMLOGIC, __modes)
>> +
>> +/* Amlogic FBC Layouts */
>> +#define DRM_FORMAT_MOD_AMLOGIC_FBC_LAYOUT_MASK		(0xf << 0)
>> +
>> +/*
>> + * Amlogic FBC Basic Layout
>> + *
>> + * The basic layout is composed of:
>> + * - a body content organized in 64x32 superblocks with 4096 bytes per
>> + *   superblock in default mode.
>> + * - a 32 bytes per 128x64 header block
>> + *
>> + * This layout is transferrable between Amlogic SoCs supporting this modifier.
>> + */
>> +#define DRM_FORMAT_MOD_AMLOGIC_FBC_LAYOUT_BASIC		(1ULL << 0)
> 
> This is kinda confusing, since this isn't actually the modifier, but the
> mode of the modifer. Generally what we do is only define the former, with
> maybe some macros to extract stuff.
> 
> To make this more mistake-proof I'd only define the full modifier code.
> Definitely don't add a #define with the DRM_FORMAT_MOD_ prefix which isn't
> actually a full modifier code.

Exact, I'll use the same scheme as AFBC: AMLOGIC_FBC_FORMAT_ ...

Neil

> -Daniel
> 
>> +
>>  #if defined(__cplusplus)
>>  }
>>  #endif
>> -- 
>> 2.22.0
>>
>