Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3dd360e2-7521-6305-c543-58830cd60ec0@starfivetech.com>
Date:   Tue, 25 Jul 2023 19:23:45 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH 3/9] drm/verisilicon: Add basic drm driver
Content-Language: en-US
From:   Keith Zhao <keith.zhao@starfivetech.com>
To:     Lucas Stach <l.stach@pengutronix.de>,
        <dri-devel@lists.freedesktop.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-riscv@lists.infradead.org>,
        <linux-media@vger.kernel.org>, <linaro-mm-sig@lists.linaro.org>
CC:     Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Emil Renner Berthing <kernel@esmil.dk>,
        "Shengyang Chen" <shengyang.chen@starfivetech.com>,
        Conor Dooley <conor+dt@kernel.org>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Jagan Teki <jagan@edgeble.ai>,
        Rob Herring <robh+dt@kernel.org>,
        Chris Morgan <macromorgan@hotmail.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        "Bjorn Andersson" <andersson@kernel.org>,
        Changhuang Liang <changhuang.liang@starfivetech.com>,
        Jack Zhu <jack.zhu@starfivetech.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Shawn Guo <shawnguo@kernel.org>, <christian.koenig@amd.com>
References: <20230602074043.33872-1-keith.zhao@starfivetech.com>
 <20230602074043.33872-4-keith.zhao@starfivetech.com>
 <e0eeae3b35e8efac7c577ca3159abcf7f43d5082.camel@pengutronix.de>
 <b8e51384-4781-2710-e94f-38a88f43b801@starfivetech.com>
In-Reply-To: <b8e51384-4781-2710-e94f-38a88f43b801@starfivetech.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk


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