Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <ed66711a-4c24-40d1-8361-40325ae915f5@starfivetech.com>
Date:   Wed, 6 Dec 2023 20:53:12 +0800
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [v3 4/6] drm/vs: Add KMS crtc&plane
Content-Language: en-US
To:     Maxime Ripard <mripard@kernel.org>
CC:     "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-riscv@lists.infradead.org" <linux-riscv@lists.infradead.org>,
        "tzimmermann@suse.de" <tzimmermann@suse.de>,
        "airlied@gmail.com" <airlied@gmail.com>,
        "krzysztof.kozlowski+dt@linaro.org" 
        <krzysztof.kozlowski+dt@linaro.org>,
        William Qiu <william.qiu@starfivetech.com>,
        Xingyu Wu <xingyu.wu@starfivetech.com>,
        "paul.walmsley@sifive.com" <paul.walmsley@sifive.com>,
        "aou@eecs.berkeley.edu" <aou@eecs.berkeley.edu>,
        "palmer@dabbelt.com" <palmer@dabbelt.com>,
        "p.zabel@pengutronix.de" <p.zabel@pengutronix.de>,
        Shengyang Chen <shengyang.chen@starfivetech.com>,
        Jack Zhu <jack.zhu@starfivetech.com>,
        Changhuang Liang <changhuang.liang@starfivetech.com>,
        "maarten.lankhorst@linux.intel.com" 
        <maarten.lankhorst@linux.intel.com>,
        "suijingfeng@loongson.cn" <suijingfeng@loongson.cn>
References: <20231204123315.28456-1-keith.zhao@starfivetech.com>
 <20231204123315.28456-5-keith.zhao@starfivetech.com>
 <o4ica2jg2rqfx6znir6j7mkoojoqzejjuqvlwcnehanw5mvop6@a3t6vi7c55tz>
From:   Keith Zhao <keith.zhao@starfivetech.com>
In-Reply-To: <o4ica2jg2rqfx6znir6j7mkoojoqzejjuqvlwcnehanw5mvop6@a3t6vi7c55tz>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk


On 2023/12/6 16:55, Maxime Ripard wrote:
> On Mon, Dec 04, 2023 at 08:33:13PM +0800, Keith Zhao wrote:
>> +static const struct vs_plane_info dc_hw_planes_rev0[PLANE_NUM] = {
>> +	{
>> +		.name			= "Primary",
>> +		.id			= PRIMARY_PLANE_0,
>> +		.type			= DRM_PLANE_TYPE_PRIMARY,
>> +		.num_formats		= ARRAY_SIZE(primary_overlay_format0),
>> +		.formats		= primary_overlay_format0,
>> +		.num_modifiers		= ARRAY_SIZE(format_modifier0),
>> +		.modifiers		= format_modifier0,
>> +		.min_width		= 0,
>> +		.min_height		= 0,
>> +		.max_width		= 4096,
>> +		.max_height		= 4096,
>> +		.rotation		= DRM_MODE_ROTATE_0 |
>> +					  DRM_MODE_ROTATE_90 |
>> +					  DRM_MODE_ROTATE_180 |
>> +					  DRM_MODE_ROTATE_270 |
>> +					  DRM_MODE_REFLECT_X |
>> +					  DRM_MODE_REFLECT_Y,
>> +		.blend_mode		= BIT(DRM_MODE_BLEND_PIXEL_NONE) |
>> +					  BIT(DRM_MODE_BLEND_PREMULTI) |
>> +					  BIT(DRM_MODE_BLEND_COVERAGE),
>> +		.color_encoding		= BIT(DRM_COLOR_YCBCR_BT709) |
>> +					  BIT(DRM_COLOR_YCBCR_BT2020),
>> +		.degamma_size		= DEGAMMA_SIZE,
>> +		.min_scale		= FRAC_16_16(1, 3),
>> +		.max_scale		= FRAC_16_16(10, 1),
>> +		.zpos			= 0,
>> +		.watermark		= true,
>> +		.color_mgmt		= true,
>> +		.roi			= true,
>> +	},
>> +	{
>> +		.name			= "Overlay",
>> +		.id			= OVERLAY_PLANE_0,
>> +		.type			= DRM_PLANE_TYPE_OVERLAY,
>> +		.num_formats		= ARRAY_SIZE(primary_overlay_format0),
>> +		.formats		= primary_overlay_format0,
>> +		.num_modifiers		= ARRAY_SIZE(format_modifier0),
>> +		.modifiers		= format_modifier0,
>> +		.min_width		= 0,
>> +		.min_height		= 0,
>> +		.max_width		= 4096,
>> +		.max_height		= 4096,
>> +		.rotation		= DRM_MODE_ROTATE_0 |
>> +					  DRM_MODE_ROTATE_90 |
>> +					  DRM_MODE_ROTATE_180 |
>> +					  DRM_MODE_ROTATE_270 |
>> +					  DRM_MODE_REFLECT_X |
>> +					  DRM_MODE_REFLECT_Y,
>> +		.blend_mode		= BIT(DRM_MODE_BLEND_PIXEL_NONE) |
>> +					  BIT(DRM_MODE_BLEND_PREMULTI) |
>> +					  BIT(DRM_MODE_BLEND_COVERAGE),
>> +		.color_encoding		= BIT(DRM_COLOR_YCBCR_BT709) |
>> +					  BIT(DRM_COLOR_YCBCR_BT2020),
>> +		.degamma_size		= DEGAMMA_SIZE,
>> +		.min_scale		= FRAC_16_16(1, 3),
>> +		.max_scale		= FRAC_16_16(10, 1),
>> +		.zpos			= 1,
>> +		.watermark		= true,
>> +		.color_mgmt		= true,
>> +		.roi			= true,
>> +	},
>> +	{
>> +		.name			= "Overlay_1",
>> +		.id			= OVERLAY_PLANE_1,
>> +		.type			= DRM_PLANE_TYPE_OVERLAY,
>> +		.num_formats		= ARRAY_SIZE(primary_overlay_format0),
>> +		.formats		= primary_overlay_format0,
>> +		.num_modifiers		= ARRAY_SIZE(secondary_format_modifiers),
>> +		.modifiers		= secondary_format_modifiers,
>> +		.min_width		= 0,
>> +		.min_height		= 0,
>> +		.max_width		= 4096,
>> +		.max_height		= 4096,
>> +		.rotation		= 0,
>> +		.blend_mode		= BIT(DRM_MODE_BLEND_PIXEL_NONE) |
>> +					  BIT(DRM_MODE_BLEND_PREMULTI) |
>> +					  BIT(DRM_MODE_BLEND_COVERAGE),
>> +		.color_encoding		= BIT(DRM_COLOR_YCBCR_BT709) |
>> +					  BIT(DRM_COLOR_YCBCR_BT2020),
>> +		.degamma_size		= DEGAMMA_SIZE,
>> +		.min_scale		= DRM_PLANE_NO_SCALING,
>> +		.max_scale		= DRM_PLANE_NO_SCALING,
>> +		.zpos			= 2,
>> +		.watermark		= true,
>> +		.color_mgmt		= true,
>> +		.roi			= true,
>> +	},
>> +	{
>> +		.name			= "Primary_1",
>> +		.id			= PRIMARY_PLANE_1,
>> +		.type			= DRM_PLANE_TYPE_PRIMARY,
>> +		.num_formats		= ARRAY_SIZE(primary_overlay_format0),
>> +		.formats		= primary_overlay_format0,
>> +		.num_modifiers		= ARRAY_SIZE(format_modifier0),
>> +		.modifiers		= format_modifier0,
>> +		.min_width		= 0,
>> +		.min_height		= 0,
>> +		.max_width		= 4096,
>> +		.max_height		= 4096,
>> +		.rotation		= DRM_MODE_ROTATE_0 |
>> +					  DRM_MODE_ROTATE_90 |
>> +					  DRM_MODE_ROTATE_180 |
>> +					  DRM_MODE_ROTATE_270 |
>> +					  DRM_MODE_REFLECT_X |
>> +					  DRM_MODE_REFLECT_Y,
>> +		.blend_mode		= BIT(DRM_MODE_BLEND_PIXEL_NONE) |
>> +					  BIT(DRM_MODE_BLEND_PREMULTI) |
>> +					  BIT(DRM_MODE_BLEND_COVERAGE),
>> +		.color_encoding		= BIT(DRM_COLOR_YCBCR_BT709) |
>> +					  BIT(DRM_COLOR_YCBCR_BT2020),
>> +		.degamma_size		= DEGAMMA_SIZE,
>> +		.min_scale		= FRAC_16_16(1, 3),
>> +		.max_scale		= FRAC_16_16(10, 1),
>> +		.zpos			= 3,
>> +		.watermark		= true,
>> +		.color_mgmt		= true,
>> +		.roi			= true,
>> +	},
>> +	{
>> +		.name			= "Overlay_2",
>> +		.id			= OVERLAY_PLANE_2,
>> +		.type			= DRM_PLANE_TYPE_OVERLAY,
>> +		.num_formats		= ARRAY_SIZE(primary_overlay_format0),
>> +		.formats		= primary_overlay_format0,
>> +		.num_modifiers		= ARRAY_SIZE(format_modifier0),
>> +		.modifiers		= format_modifier0,
>> +		.min_width		= 0,
>> +		.min_height		= 0,
>> +		.max_width		= 4096,
>> +		.max_height		= 4096,
>> +		.rotation		= DRM_MODE_ROTATE_0 |
>> +					  DRM_MODE_ROTATE_90 |
>> +					  DRM_MODE_ROTATE_180 |
>> +					  DRM_MODE_ROTATE_270 |
>> +					  DRM_MODE_REFLECT_X |
>> +					  DRM_MODE_REFLECT_Y,
>> +		.blend_mode		= BIT(DRM_MODE_BLEND_PIXEL_NONE) |
>> +					  BIT(DRM_MODE_BLEND_PREMULTI) |
>> +					  BIT(DRM_MODE_BLEND_COVERAGE),
>> +		.color_encoding		= BIT(DRM_COLOR_YCBCR_BT709) |
>> +					  BIT(DRM_COLOR_YCBCR_BT2020),
>> +		.degamma_size		= DEGAMMA_SIZE,
>> +		.min_scale		= FRAC_16_16(1, 3),
>> +		.max_scale		= FRAC_16_16(10, 1),
>> +		.zpos			= 4,
>> +		.watermark		= true,
>> +		.color_mgmt		= true,
>> +		.roi			= true,
>> +	},
>> +	{
>> +		.name			= "Overlay_3",
>> +		.id			= OVERLAY_PLANE_3,
>> +		.type			= DRM_PLANE_TYPE_OVERLAY,
>> +		.num_formats		= ARRAY_SIZE(primary_overlay_format0),
>> +		.formats		= primary_overlay_format0,
>> +		.num_modifiers		= ARRAY_SIZE(secondary_format_modifiers),
>> +		.modifiers		= secondary_format_modifiers,
>> +		.min_width		= 0,
>> +		.min_height		= 0,
>> +		.max_width		= 4096,
>> +		.max_height		= 4096,
>> +		.rotation		= 0,
>> +		.blend_mode		= BIT(DRM_MODE_BLEND_PIXEL_NONE) |
>> +					  BIT(DRM_MODE_BLEND_PREMULTI) |
>> +					  BIT(DRM_MODE_BLEND_COVERAGE),
>> +		.color_encoding		= BIT(DRM_COLOR_YCBCR_BT709) |
>> +					  BIT(DRM_COLOR_YCBCR_BT2020),
>> +		.degamma_size		= DEGAMMA_SIZE,
>> +		.min_scale		= DRM_PLANE_NO_SCALING,
>> +		.max_scale		= DRM_PLANE_NO_SCALING,
>> +		.zpos			= 5,
>> +		.watermark		= true,
>> +		.color_mgmt		= true,
>> +		.roi			= true,
>> +	},
>> +	{
>> +		.name			= "Cursor",
>> +		.id			= CURSOR_PLANE_0,
>> +		.type			= DRM_PLANE_TYPE_CURSOR,
>> +		.num_formats		= ARRAY_SIZE(cursor_formats),
>> +		.formats		= cursor_formats,
>> +		.num_modifiers		= 0,
>> +		.modifiers		= NULL,
>> +		.min_width		= 32,
>> +		.min_height		= 32,
>> +		.max_width		= 64,
>> +		.max_height		= 64,
>> +		.rotation		= 0,
>> +		.degamma_size		= 0,
>> +		.min_scale		= DRM_PLANE_NO_SCALING,
>> +		.max_scale		= DRM_PLANE_NO_SCALING,
>> +		.zpos			= 255,
>> +		.watermark		= false,
>> +		.color_mgmt		= false,
>> +		.roi			= false,
>> +	},
>> +	{
>> +		.name			= "Cursor_1",
>> +		.id			= CURSOR_PLANE_1,
>> +		.type			= DRM_PLANE_TYPE_CURSOR,
>> +		.num_formats		= ARRAY_SIZE(cursor_formats),
>> +		.formats		= cursor_formats,
>> +		.num_modifiers		= 0,
>> +		.modifiers		= NULL,
>> +		.min_width		= 32,
>> +		.min_height		= 32,
>> +		.max_width		= 64,
>> +		.max_height		= 64,
>> +		.rotation		= 0,
>> +		.degamma_size		= 0,
>> +		.min_scale		= DRM_PLANE_NO_SCALING,
>> +		.max_scale		= DRM_PLANE_NO_SCALING,
>> +		.zpos			= 255,
>> +		.watermark		= false,
>> +		.color_mgmt		= false,
>> +		.roi			= false,
>> +	},
>> +};
>> +
>> +static const struct vs_dc_info dc8200_info = {
>> +	.name			= "DC8200",
>> +	.panel_num		= 2,
>> +	.plane_num		= 8,
>> +	.planes			= dc_hw_planes_rev0,
>> +	.layer_num		= 6,
>> +	.max_bpc		= 10,
>> +	.color_formats		= DRM_COLOR_FORMAT_RGB444 |
>> +				  DRM_COLOR_FORMAT_YCBCR444 |
>> +				  DRM_COLOR_FORMAT_YCBCR422 |
>> +				  DRM_COLOR_FORMAT_YCBCR420,
>> +	.gamma_size		= GAMMA_EX_SIZE,
>> +	.gamma_bits		= 12,
>> +	.pitch_alignment	= 128,
>> +	.pipe_sync		= false,
>> +	.background		= true,
>> +	.panel_sync		= true,
>> +	.cap_dec		= true,
>> +};
> 
> I really think that entire thing is to workaround a suboptimal device
> tree binding. You should have two CRTCs in the device tree, you'll probe
> twice, and you won't get to do that whole dance.
> 

i got you means “two CRTCs in the device tree，probe twice"
maybe also I can split the common ctrc helper fun into 2 , 

current the driver probe once and map 2 port as ctrc node ,

> 
>> +struct dc_hw_plane_reg {
>> +	u32 y_address;
>> +	u32 u_address;
>> +	u32 v_address;
>> +	u32 y_stride;
>> +	u32 u_stride;
>> +	u32 v_stride;
>> +	u32 size;
>> +	u32 top_left;
>> +	u32 bottom_right;
>> +	u32 scale_factor_x;
>> +	u32 scale_factor_y;
>> +	u32 h_filter_coef_index;
>> +	u32 h_filter_coef_data;
>> +	u32 v_filter_coef_index;
>> +	u32 v_filter_coef_data;
>> +	u32 init_offset;
>> +	u32 color_key;
>> +	u32 color_key_high;
>> +	u32 clear_value;
>> +	u32 color_table_index;
>> +	u32 color_table_data;
>> +	u32 scale_config;
>> +	u32 water_mark;
>> +	u32 degamma_index;
>> +	u32 degamma_data;
>> +	u32 degamma_ex_data;
>> +	u32 src_global_color;
>> +	u32 dst_global_color;
>> +	u32 blend_config;
>> +	u32 roi_origin;
>> +	u32 roi_size;
>> +	u32 yuv_to_rgb_coef0;
>> +	u32 yuv_to_rgb_coef1;
>> +	u32 yuv_to_rgb_coef2;
>> +	u32 yuv_to_rgb_coef3;
>> +	u32 yuv_to_rgb_coef4;
>> +	u32 yuv_to_rgb_coefd0;
>> +	u32 yuv_to_rgb_coefd1;
>> +	u32 yuv_to_rgb_coefd2;
>> +	u32 y_clamp_bound;
>> +	u32 uv_clamp_bound;
>> +	u32 rgb_to_rgb_coef0;
>> +	u32 rgb_to_rgb_coef1;
>> +	u32 rgb_to_rgb_coef2;
>> +	u32 rgb_to_rgb_coef3;
>> +	u32 rgb_to_rgb_coef4;
>> +};
> 
> That's your plane state.
> 
>> +struct dc_hw_fb {
>> +	u32 y_address;
>> +	u32 u_address;
>> +	u32 v_address;
>> +	u32 clear_value;
>> +	u32 water_mark;
>> +	u16 y_stride;
>> +	u16 u_stride;
>> +	u16 v_stride;
>> +	u16 width;
>> +	u16 height;
>> +	u8	format;
>> +	u8	tile_mode;
>> +	u8	rotation;
>> +	u8	yuv_color_space;
>> +	u8	swizzle;
>> +	u8	uv_swizzle;
>> +	u8	zpos;
>> +	u8	display_id;
>> +	bool	clear_enable;
>> +	bool	dec_enable;
>> +	bool	enable;
>> +	bool	dirty;
>> +};
> 
> Your framebuffer
> 
>> +struct dc_hw_scale {
>> +	u32 scale_factor_x;
>> +	u32 scale_factor_y;
>> +	bool	enable;
>> +	bool	dirty;
>> +};
>> +
>> +struct dc_hw_position {
>> +	u16 start_x;
>> +	u16 start_y;
>> +	u16 end_x;
>> +	u16 end_y;
>> +	bool	dirty;
>> +};
>> +
>> +struct dc_hw_blend {
>> +	u8	alpha;
>> +	u8	blend_mode;
>> +	bool	dirty;
>> +};
>> +
>> +struct dc_hw_colorkey {
>> +	u32 colorkey;
>> +	u32 colorkey_high;
>> +	u8	transparency;
>> +	bool dirty;
>> +};
> 
> Your CRTC state.
> 
>> +struct dc_hw_roi {
>> +	u16 x;
>> +	u16 y;
>> +	u16 width;
>> +	u16 height;
>> +	bool enable;
>> +	bool dirty;
>> +};
>> +
>> +struct dc_hw_cursor {
>> +	u32 address;
>> +	u16 x;
>> +	u16 y;
>> +	u16 hot_x;
>> +	u16 hot_y;
>> +	u8	size;
>> +	u8	display_id;
>> +	bool	enable;
>> +	bool	dirty;
>> +};
>> +
>> +struct dc_hw_display {
>> +	u32 bus_format;
>> +	u16 h_active;
>> +	u16 h_total;
>> +	u16 h_sync_start;
>> +	u16 h_sync_end;
>> +	u16 v_active;
>> +	u16 v_total;
>> +	u16 v_sync_start;
>> +	u16 v_sync_end;
>> +	u8	id;
>> +	bool	h_sync_polarity;
>> +	bool	v_sync_polarity;
>> +	bool	enable;
>> +};
> 
> Your display mode.
> 
> Really, you have the huge majority of those informations already
> available, there's no need to duplicate it. And chances are you'll
> create bugs in the process.
> Maxime
ok let me try it, 
This will be another relatively big change.