Hi Heiko,
At 2025-01-06 05:50:30, "Heiko Stübner" <he...@sntech.de> wrote:
>Hi Andy,
>
>Am Dienstag, 31. Dezember 2024, 10:07:45 CET schrieb Andy Yan:
>> From: Andy Yan <andy....@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....@rock-chips.com>
>> Tested-by: Michael Riesch <michael.rie...@wolfvision.net> # on RK3568
>> Tested-by: Detlev Casanova <detlev.casan...@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-d08c776da...@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 <he...@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-d08c776da...@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 <he...@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
>
>
>
>
>
