[PATCH v8 2/9] drm/rockchip: vop2: Add platform specific callback

[Andy Yan]

Hi Heiko,

At 2025-01-06 05:50:30, "Heiko Stübner" <heiko at sntech.de> wrote:
>Hi Andy,
>
>Am Dienstag, 31. Dezember 2024, 10:07:45 CET schrieb Andy Yan:
>> From: Andy Yan <andy.yan at rock-chips.com>
>> 
>> The VOP interface mux, overlay, background delay cycle configuration
>> of different SOC are much different. Add platform specific callback
>> ops to let the core driver look cleaner and more refined.
>> 
>> Signed-off-by: Andy Yan <andy.yan at rock-chips.com>
>> Tested-by: Michael Riesch <michael.riesch at wolfvision.net> # on RK3568
>> Tested-by: Detlev Casanova <detlev.casanova at collabora.com>
>
>>  static int vop2_cluster_init(struct vop2_win *win)
>>  {
>>  	struct vop2 *vop2 = win->vop2;
>>  	struct reg_field *cluster_regs;
>>  	int ret, i;
>>  
>> -	cluster_regs = kmemdup(vop2_cluster_regs, sizeof(vop2_cluster_regs),
>> +	cluster_regs = kmemdup(vop2->data->cluster_reg,
>> +			       sizeof(struct reg_field) * vop2->data->nr_cluster_regs,
>>  			       GFP_KERNEL);
>>  	if (!cluster_regs)
>>  		return -ENOMEM;
>>  
>> -	for (i = 0; i < ARRAY_SIZE(vop2_cluster_regs); i++)
>> +	for (i = 0; i < vop2->data->nr_cluster_regs; i++)
>>  		if (cluster_regs[i].reg != 0xffffffff)
>>  			cluster_regs[i].reg += win->offset;
>>  
>>  	ret = devm_regmap_field_bulk_alloc(vop2->dev, vop2->map, win->reg,
>>  					   cluster_regs,
>> -					   ARRAY_SIZE(vop2_cluster_regs));
>> -
>> +					   vop2->data->nr_cluster_regs);
>>  	kfree(cluster_regs);
>>  
>>  	return ret;
>>  };
>
>Even the original code, makes checkpatch really unhappy nowadays :-( .
>
>As per
>https://lore.kernel.org/all/20240706-regmap-const-structs-v1-1-d08c776da787@weissschuh.net/
>reg_field should be considered const, so copying the original struct and
>then modifying it causes checkpatch warnings now.
>
>I've tried to adapt the function as in the patch below. This should
>contain the same functionality as before, just with keeping the reg_field
>const.
>
>As it's the weekend, I didn't have time to test that change, so it's more
>meant as an idea on how to proceed.

Thank you so much, I will try it in the following days.

>
>
>> +	/* afbc regs */
>> +	[VOP2_WIN_AFBC_FORMAT] = REG_FIELD(RK3568_CLUSTER_WIN_AFBCD_CTRL, 2, 6),
>> +	[VOP2_WIN_AFBC_RB_SWAP] = REG_FIELD(RK3568_CLUSTER_WIN_AFBCD_CTRL, 9, 9),
>> +	[VOP2_WIN_AFBC_UV_SWAP] = REG_FIELD(RK3568_CLUSTER_WIN_AFBCD_CTRL, 10, 10),
>> +	[VOP2_WIN_AFBC_AUTO_GATING_EN] = REG_FIELD(RK3568_CLUSTER_WIN_AFBCD_OUTPUT_CTRL, 4, 4),
>> +	[VOP2_WIN_AFBC_HALF_BLOCK_EN] = REG_FIELD(RK3568_CLUSTER_WIN_AFBCD_CTRL, 7, 7),
>> +	[VOP2_WIN_AFBC_BLOCK_SPLIT_EN] = REG_FIELD(RK3568_CLUSTER_WIN_AFBCD_CTRL, 8, 8),
>> +	[VOP2_WIN_AFBC_HDR_PTR] = REG_FIELD(RK3568_CLUSTER_WIN_AFBCD_HDR_PTR, 0, 31),
>> +	[VOP2_WIN_AFBC_PIC_SIZE] = REG_FIELD(RK3568_CLUSTER_WIN_AFBCD_PIC_SIZE, 0, 31),
>> +	[VOP2_WIN_AFBC_PIC_VIR_WIDTH] = REG_FIELD(RK3568_CLUSTER_WIN_AFBCD_VIR_WIDTH, 0, 15),
>> +	[VOP2_WIN_AFBC_TILE_NUM] = REG_FIELD(RK3568_CLUSTER_WIN_AFBCD_VIR_WIDTH, 16, 31),
>> +	[VOP2_WIN_AFBC_PIC_OFFSET] = REG_FIELD(RK3568_CLUSTER_WIN_AFBCD_PIC_OFFSET, 0, 31),
>> +	[VOP2_WIN_AFBC_DSP_OFFSET] = REG_FIELD(RK3568_CLUSTER_WIN_AFBCD_DSP_OFFSET, 0, 31),
>> +	[VOP2_WIN_AFBC_TRANSFORM_OFFSET] = REG_FIELD(RK3568_CLUSTER_WIN_AFBCD_TRANSFORM_OFFSET, 0, 31),
>
>exceeds the 100 char line length, so I think we should have a line break
>after RK3568_CLUSTER_WIN_AFBCD_TRANSFORM_OFFSET
>
>
>Thanks
>Heiko
>
>---------------- 8< ---------------
>From: Heiko Stuebner <heiko at sntech.de>
>Date: Sun, 5 Jan 2025 17:38:31 +0100
>Subject: [PATCH] drm/rockchip: vop2: use devm_regmap_field_alloc for cluster-regs
>
>Right now vop2_cluster_init() copies the base vop2_cluster_regs and adapts
>the reg value with the current window's offset before adding the fields to
>the regmap.
>
>This conflicts with the notion of reg_fields being const, see
>https://lore.kernel.org/all/20240706-regmap-const-structs-v1-1-d08c776da787@weissschuh.net/
>for reference, which now causes checkpatch to actually warn about that.
>
>So instead of creating one big copy and changing it afterwards, add the
>reg_fields individually using devm_regmap_field_alloc().
>
>Functional it is the same, just that the reg_field we're handling
>can stay const.
>
>Signed-off-by: Heiko Stuebner <heiko at sntech.de>
>---
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 30 +++++++++-----------
> 1 file changed, 14 insertions(+), 16 deletions(-)
>
>diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>index 17a98845fd31..c8da1ebb6013 100644
>--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>@@ -3443,25 +3443,23 @@ static struct reg_field vop2_cluster_regs[VOP2_WIN_MAX_REG] = {
> static int vop2_cluster_init(struct vop2_win *win)
> {
> 	struct vop2 *vop2 = win->vop2;
>-	struct reg_field *cluster_regs;
>-	int ret, i;
>-
>-	cluster_regs = kmemdup(vop2_cluster_regs, sizeof(vop2_cluster_regs),
>-			       GFP_KERNEL);
>-	if (!cluster_regs)
>-		return -ENOMEM;
>+	int i;
> 
>-	for (i = 0; i < ARRAY_SIZE(vop2_cluster_regs); i++)
>-		if (cluster_regs[i].reg != 0xffffffff)
>-			cluster_regs[i].reg += win->offset;
>+	for (i = 0; i < ARRAY_SIZE(vop2_cluster_regs); i++) {
>+		const struct reg_field field = {
>+			.reg = (vop2_cluster_regs[i].reg != 0xffffffff) ?
>+					vop2_cluster_regs[i].reg + win->offset :
>+					vop2_cluster_regs[i].reg,
>+			.lsb = vop2_cluster_regs[i].lsb,
>+			.msb = vop2_cluster_regs[i].msb
>+		};
> 
>-	ret = devm_regmap_field_bulk_alloc(vop2->dev, vop2->map, win->reg,
>-					   cluster_regs,
>-					   ARRAY_SIZE(vop2_cluster_regs));
>-
>-	kfree(cluster_regs);
>+		win->reg[i] = devm_regmap_field_alloc(vop2->dev, vop2->map, field);
>+		if (IS_ERR(win->reg[i]))
>+			return PTR_ERR(win->reg[i]);
>+	}
> 
>-	return ret;
>+	return 0;
> };
> 
> static struct reg_field vop2_esmart_regs[VOP2_WIN_MAX_REG] = {
>-- 
>2.45.2
>
>
>
>
>