Re: [PATCH 3/9] drm/verisilicon: Add basic drm driver

[Keith Zhao <keith.zhao@starfivetech.com>]

On 2023/7/25 11:12, Keith Zhao wrote:
> 
> 
> On 2023/6/7 16:53, Lucas Stach wrote:
>> Hi Keith,
>> 
>> Am Freitag, dem 02.06.2023 um 15:40 +0800 schrieb Keith Zhao:
>>> Add a basic platform driver of the DRM driver for JH7110 SoC.
>>> 
>>> Signed-off-by: Keith Zhao <keith.zhao@starfivetech.com>
>>> ---
>>>  MAINTAINERS                          |   2 +
>>>  drivers/gpu/drm/Kconfig              |   2 +
>>>  drivers/gpu/drm/Makefile             |   1 +
>>>  drivers/gpu/drm/verisilicon/Kconfig  |  13 ++
>>>  drivers/gpu/drm/verisilicon/Makefile |   6 +
>>>  drivers/gpu/drm/verisilicon/vs_drv.c | 284 +++++++++++++++++++++++++++
>>>  drivers/gpu/drm/verisilicon/vs_drv.h |  48 +++++
>>>  include/uapi/drm/drm_fourcc.h        |  83 ++++++++
>>>  include/uapi/drm/vs_drm.h            |  50 +++++
>>>  9 files changed, 489 insertions(+)
>>>  create mode 100644 drivers/gpu/drm/verisilicon/Kconfig
>>>  create mode 100644 drivers/gpu/drm/verisilicon/Makefile
>>>  create mode 100644 drivers/gpu/drm/verisilicon/vs_drv.c
>>>  create mode 100644 drivers/gpu/drm/verisilicon/vs_drv.h
>>>  create mode 100644 include/uapi/drm/vs_drm.h
>>> 
>>> 
>>> [...]
>>> +#endif /* __VS_DRV_H__ */
>>> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
>>> index de703c6be969..af4fb50f9207 100644
>>> --- a/include/uapi/drm/drm_fourcc.h
>>> +++ b/include/uapi/drm/drm_fourcc.h
>>> @@ -419,6 +419,7 @@ extern "C" {
>>>  #define DRM_FORMAT_MOD_VENDOR_ARM     0x08
>>>  #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09
>>>  #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a
>>> +#define DRM_FORMAT_MOD_VENDOR_VS      0x0b
>>>  
>>>  /* add more to the end as needed */
>>>  
>>> @@ -1519,6 +1520,88 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64 modifier)
>>>  #define AMD_FMT_MOD_CLEAR(field) \
>>>  	(~((__u64)AMD_FMT_MOD_##field##_MASK << AMD_FMT_MOD_##field##_SHIFT))
>>>  
>>> +#define DRM_FORMAT_MOD_VS_TYPE_NORMAL        0x00
>>> +#define DRM_FORMAT_MOD_VS_TYPE_COMPRESSED    0x01
>>> +#define DRM_FORMAT_MOD_VS_TYPE_CUSTOM_10BIT  0x02
>>> +#define DRM_FORMAT_MOD_VS_TYPE_MASK     ((__u64)0x3 << 54)
>>> +
>>> +#define fourcc_mod_vs_code(type, val) \
>>> +	fourcc_mod_code(VS, ((((__u64)type) << 54) | (val)))
>>> +
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_MODE_MASK    0x3F
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_8X8_XMAJOR   0x00
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_8X8_YMAJOR   0x01
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_16X4     0x02
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_8X4      0x03
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_4X8      0x04
>>> +#define DRM_FORMAT_MOD_VS_DEC_RASTER_16X4   0x06
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_64X4     0x07
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_32X4     0x08
>>> +#define DRM_FORMAT_MOD_VS_DEC_RASTER_256X1  0x09
>>> +#define DRM_FORMAT_MOD_VS_DEC_RASTER_128X1  0x0A
>>> +#define DRM_FORMAT_MOD_VS_DEC_RASTER_64X4   0x0B
>>> +#define DRM_FORMAT_MOD_VS_DEC_RASTER_256X2  0x0C
>>> +#define DRM_FORMAT_MOD_VS_DEC_RASTER_128X2  0x0D
>>> +#define DRM_FORMAT_MOD_VS_DEC_RASTER_128X4  0x0E
>>> +#define DRM_FORMAT_MOD_VS_DEC_RASTER_64X1   0x0F
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_16X8     0x10
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_8X16     0x11
>>> +#define DRM_FORMAT_MOD_VS_DEC_RASTER_512X1  0x12
>>> +#define DRM_FORMAT_MOD_VS_DEC_RASTER_32X4   0x13
>>> +#define DRM_FORMAT_MOD_VS_DEC_RASTER_64X2   0x14
>>> +#define DRM_FORMAT_MOD_VS_DEC_RASTER_32X2   0x15
>>> +#define DRM_FORMAT_MOD_VS_DEC_RASTER_32X1   0x16
>>> +#define DRM_FORMAT_MOD_VS_DEC_RASTER_16X1   0x17
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_128X4    0x18
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_256X4    0x19
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_512X4    0x1A
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_16X16    0x1B
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_32X16    0x1C
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_64X16    0x1D
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_128X8    0x1E
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_8X4_S    0x1F
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_16X4_S   0x20
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_32X4_S   0x21
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_16X4_LSB 0x22
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_32X4_LSB 0x23
>>> +#define DRM_FORMAT_MOD_VS_DEC_TILE_32X8     0x24
>>> +
>>> +#define DRM_FORMAT_MOD_VS_DEC_ALIGN_32      (0x01 << 6)
>>> +#define DRM_FORMAT_MOD_VS_DEC_ALIGN_64      (0x01 << 7)
>>> +
>>> +#define fourcc_mod_vs_dec_code(tile, align) \
>>> +	fourcc_mod_vs_code(DRM_FORMAT_MOD_VS_TYPE_COMPRESSED, \
>>> +				((tile) | (align)))
>>> +
>>> +#define DRM_FORMAT_MOD_VS_NORM_MODE_MASK        0x1F
>>> +#define DRM_FORMAT_MOD_VS_LINEAR                0x00
>>> +#define DRM_FORMAT_MOD_VS_TILED4x4              0x01
>>> +#define DRM_FORMAT_MOD_VS_SUPER_TILED_XMAJOR    0x02
>>> +#define DRM_FORMAT_MOD_VS_SUPER_TILED_YMAJOR    0x03
>>> +#define DRM_FORMAT_MOD_VS_TILE_8X8              0x04
>>> +#define DRM_FORMAT_MOD_VS_TILE_MODE1            0x05
>>> +#define DRM_FORMAT_MOD_VS_TILE_MODE2            0x06
>>> +#define DRM_FORMAT_MOD_VS_TILE_8X4              0x07
>>> +#define DRM_FORMAT_MOD_VS_TILE_MODE4            0x08
>>> +#define DRM_FORMAT_MOD_VS_TILE_MODE5            0x09
>>> +#define DRM_FORMAT_MOD_VS_TILE_MODE6            0x0A
>>> +#define DRM_FORMAT_MOD_VS_SUPER_TILED_XMAJOR_8X4    0x0B
>>> +#define DRM_FORMAT_MOD_VS_SUPER_TILED_YMAJOR_4X8    0x0C
>>> +#define DRM_FORMAT_MOD_VS_TILE_Y                0x0D
>>> +#define DRM_FORMAT_MOD_VS_TILE_128X1            0x0F
>>> +#define DRM_FORMAT_MOD_VS_TILE_256X1            0x10
>>> +#define DRM_FORMAT_MOD_VS_TILE_32X1             0x11
>>> +#define DRM_FORMAT_MOD_VS_TILE_64X1             0x12
>>> +#define DRM_FORMAT_MOD_VS_TILE_MODE4X4          0x15
>>> +
>>> +#define fourcc_mod_vs_norm_code(tile) \
>>> +	fourcc_mod_vs_code(DRM_FORMAT_MOD_VS_TYPE_NORMAL, \
>>> +				(tile))
>>> +
>>> +#define fourcc_mod_vs_custom_code(tile) \
>>> +	fourcc_mod_vs_code(DRM_FORMAT_MOD_VS_TYPE_CUSTOM_10BIT, \
>>> +				(tile))
>>> +
>> 
>> You are opening a new namespace for what is effectively the VIVANTE
>> tiling. While your list seems much more exhaustive than the (reverse
>> engineered) list provided under the VIVANTE namespace, this is still
>> unacceptable as it adds new aliases for existing modifiers.
>> 
> hi Lucas:
> I got what you mean , I will check the whether the current existence can be reused.
> In principle, can existing modefiers cover my definition?
> 

hello Lucas:
I made it a little simpler: I removed what I didn't use in the code .
Keep only these:

#define DRM_FORMAT_MOD_VS_TYPE_NORMAL        0x00
#define DRM_FORMAT_MOD_VS_TYPE_COMPRESSED    0x01
#define DRM_FORMAT_MOD_VS_TYPE_CUSTOM_10BIT  0x02
#define DRM_FORMAT_MOD_VS_TYPE_MASK     ((__u64)0x3 << 54)

#define fourcc_mod_vs_code(type, val) \
	fourcc_mod_code(VS, ((((__u64)type) << 54) | (val)))

#define DRM_FORMAT_MOD_VS_NORM_MODE_MASK        0x1F
#define DRM_FORMAT_MOD_VS_LINEAR                0x00
#define DRM_FORMAT_MOD_VS_SUPER_TILED_XMAJOR    0x02
#define DRM_FORMAT_MOD_VS_SUPER_TILED_YMAJOR    0x03
#define DRM_FORMAT_MOD_VS_TILE_8X8              0x04
#define DRM_FORMAT_MOD_VS_TILE_8X4              0x07
#define DRM_FORMAT_MOD_VS_SUPER_TILED_XMAJOR_8X4    0x0B
#define DRM_FORMAT_MOD_VS_SUPER_TILED_YMAJOR_4X8    0x0C
#define DRM_FORMAT_MOD_VS_TILE_MODE4X4          0x15

#define fourcc_mod_vs_norm_code(tile) \
	fourcc_mod_vs_code(DRM_FORMAT_MOD_VS_TYPE_NORMAL, \
				(tile))

#define fourcc_mod_vs_custom_code(tile) \
	fourcc_mod_vs_code(DRM_FORMAT_MOD_VS_TYPE_CUSTOM_10BIT, \
				(tile))

I need you help to check what modifiers it should be (for existing modifiers) 

DRM_FORMAT_MOD_VS_LINEAR			----> DRM_FORMAT_MOD_LINEAR
DRM_FORMAT_MOD_VS_SUPER_TILED_XMAJOR 		----> ?
DRM_FORMAT_MOD_VS_SUPER_TILED_YMAJOR 		----> ?
DRM_FORMAT_MOD_VS_TILE_8X8           		----> ?  
DRM_FORMAT_MOD_VS_TILE_8X4              	----> ?                    
DRM_FORMAT_MOD_VS_SUPER_TILED_XMAJOR_8X4    	----> ?
DRM_FORMAT_MOD_VS_SUPER_TILED_YMAJOR_4X8    	----> ?
DRM_FORMAT_MOD_VS_TILE_MODE4X4          	----> DRM_FORMAT_MOD_VIVANTE_TILED

Thanks a million!!
Keith
>> Also any modifier additions should be in a separate patch and not
>> buried in another change.
>> 
> ok , no problem
>> Regards,
>> Lucas