[PATCH 11/21] drm/msm/dpu: Add RM support for allocating CWB
Jessica Zhang
quic_jesszhan at quicinc.com
Fri Sep 6 16:53:13 UTC 2024
On 9/5/2024 6:30 AM, Dmitry Baryshkov wrote:
> On Tue, Sep 03, 2024 at 06:04:13PM GMT, Jessica Zhang wrote:
>>
>>
>> On 8/30/2024 3:16 PM, Dmitry Baryshkov wrote:
>>> On Fri, 30 Aug 2024 at 22:28, Jessica Zhang <quic_jesszhan at quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 8/30/2024 10:18 AM, Dmitry Baryshkov wrote:
>>>>> On Thu, Aug 29, 2024 at 01:48:32PM GMT, Jessica Zhang wrote:
>>>>>> Add support for allocating the concurrent writeback mux as part of the
>>>>>> WB allocation
>>>>>>
>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan at quicinc.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 5 ++++-
>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 30 +++++++++++++++++++++++++++--
>>>>>> 2 files changed, 32 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>>>>>> index c17d2d356f7a..c43cb55fe1d2 100644
>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>>>>>> @@ -1,5 +1,7 @@
>>>>>> /* SPDX-License-Identifier: GPL-2.0-only */
>>>>>> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>>>>>> +/*
>>>>>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>>>>>> + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>>>>>> */
>>>>>>
>>>>>> #ifndef _DPU_HW_MDSS_H
>>>>>> @@ -352,6 +354,7 @@ struct dpu_mdss_color {
>>>>>> #define DPU_DBG_MASK_DSPP (1 << 10)
>>>>>> #define DPU_DBG_MASK_DSC (1 << 11)
>>>>>> #define DPU_DBG_MASK_CDM (1 << 12)
>>>>>> +#define DPU_DBG_MASK_CWB (1 << 13)
>>>>>>
>>>>>> /**
>>>>>> * struct dpu_hw_tear_check - Struct contains parameters to configure
>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>>>> index bc99b04eae3a..738e9a081b10 100644
>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>>>> @@ -1,9 +1,10 @@
>>>>>> // SPDX-License-Identifier: GPL-2.0-only
>>>>>> /*
>>>>>> * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
>>>>>> - * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>>>>>> + * Copyright (c) 2023-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>>>>>> */
>>>>>>
>>>>>> +#include <drm/drm_managed.h>
>>>>>> #include "msm_drv.h"
>>>>>> #define pr_fmt(fmt) "[drm:%s] " fmt, __func__
>>>>>> #include "dpu_kms.h"
>>>>>> @@ -34,6 +35,7 @@ int dpu_rm_init(struct drm_device *dev,
>>>>>> void __iomem *mmio)
>>>>>> {
>>>>>> int rc, i;
>>>>>> + struct dpu_hw_blk_reg_map *cwb_reg_map;
>>>>>>
>>>>>> if (!rm || !cat || !mmio) {
>>>>>> DPU_ERROR("invalid kms\n");
>>>>>> @@ -100,11 +102,35 @@ int dpu_rm_init(struct drm_device *dev,
>>>>>> rm->hw_intf[intf->id - INTF_0] = hw;
>>>>>> }
>>>>>>
>>>>>> + if (cat->cwb_count > 0) {
>>>>>> + cwb_reg_map = drmm_kzalloc(dev,
>>>>>> + sizeof(*cwb_reg_map) * cat->cwb_count,
>>>>>> + GFP_KERNEL);
>>>>>
>>>>> Please move CWB block pointers to dpu_rm. There is no need to allocate a
>>>>> separate array.
>>>>
>>>> Hi Dmitry,
>>>>
>>>> Sorry, I'm not sure what you mean here. Can you clarify your comment?
>>>>
>>>> This is just allocating an array of the CWB register addresses so that
>>>> the hw_wb block can use it to configure the CWB mux registers.
>>>
>>> Excuse me. I asked to make the cwb_reg_map array a part of the
>>> existing dpu_rm structure. This way other subblocks can access it
>>> through dpu_rm API.
>>
>> Got it, thanks for the clarification. Just wondering, is the intent here to
>> add CWB to rm's get_assigned_resourced?
>>
>> The CWB registers will be handled by hw_wb and isn't referenced anywhere
>> outside of hw_wb (aside from when it's being allocated and passed into
>> hw_wb_init) so I'm not sure what's the benefit of adding it to the dpu_rm
>> struct.
>
> To have a single point where all the blocks are handled, pretty much
> like we have a single catalog where all blocks are allocated. Note how
> e.g. how MERGE_3D is handled. Or how we return harware instances for
> INTF or WB.
Got it, seems like you're leaning towards having CWB as a completely
independent hardware block with its own dpu_hw_cwb file and struct.
FWIW, we did consider this approach at the very beginning, but decided
to go with having the CWB registers configured by dpu_hw_wb under the
hood because we thought it would be overkill to create a completely new
struct just to program 2 registers via 1 function op.
We ended up adding the CWB mux programming to dpu_hw_wb because CWB is
closely tied with WB and it mirrored how downstream code was programming
CWB mux [1]
If you prefer to have CWB mux completely independent, I can switch to
that instead.
[1]
https://android.googlesource.com/kernel/msm-extra/display-drivers/+/e18d8e759a344ad4d86b31bbf8160cfe4c65b772/msm/sde/sde_hw_wb.c#265
>
>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>> Jessica Zhang
>>>>
>>>>>
>>>>>> +
>>>>>> + if (!cwb_reg_map) {
>>>>>> + DPU_ERROR("failed cwb object creation\n");
>>>>>> + return -ENOMEM;
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> +
>>>>>> + for (i = 0; i < cat->cwb_count; i++) {
>>>>>> + struct dpu_hw_blk_reg_map *cwb = &cwb_reg_map[i];
>>>>>> +
>>>>>> + cwb->blk_addr = mmio + cat->cwb[i].base;
>>>>>> + cwb->log_mask = DPU_DBG_MASK_CWB;
>>>>>> + }
>>>>>> +
>>>>>> for (i = 0; i < cat->wb_count; i++) {
>>>>>> struct dpu_hw_wb *hw;
>>>>>> const struct dpu_wb_cfg *wb = &cat->wb[i];
>>>>>>
>>>>>> - hw = dpu_hw_wb_init(dev, wb, mmio, cat->mdss_ver);
>>>>>> + if (cat->cwb)
>>>>>> + hw = dpu_hw_wb_init_with_cwb(dev, wb, mmio,
>>>>>> + cat->mdss_ver, cwb_reg_map);
>>>>>> + else
>>>>>> + hw = dpu_hw_wb_init(dev, wb, mmio, cat->mdss_ver);
>>>>>> +
>>>>>> if (IS_ERR(hw)) {
>>>>>> rc = PTR_ERR(hw);
>>>>>> DPU_ERROR("failed wb object creation: err %d\n", rc);
>>>>>>
>>>>>> --
>>>>>> 2.34.1
>>>>>>
>>>>>
>>>>> --
>>>>> With best wishes
>>>>> Dmitry
>>>
>>>
>>>
>>> --
>>> With best wishes
>>> Dmitry
>
> --
> With best wishes
> Dmitry
More information about the Freedreno
mailing list