lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAA8EJpqdtK_GG-p1b4WNBAbmdnu-Nt5Z17HHnNeUyL25C0wW=w@mail.gmail.com> Date: Fri, 6 Sep 2024 20:58:51 +0300 From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org> To: Jessica Zhang <quic_jesszhan@...cinc.com> Cc: Rob Clark <robdclark@...il.com>, quic_abhinavk@...cinc.com, Sean Paul <sean@...rly.run>, Marijn Suijten <marijn.suijten@...ainline.org>, David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, quic_ebharadw@...cinc.com, linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org, linux-kernel@...r.kernel.org, Rob Clark <robdclark@...omium.org> Subject: Re: [PATCH 11/21] drm/msm/dpu: Add RM support for allocating CWB On Fri, 6 Sept 2024 at 19:53, Jessica Zhang <quic_jesszhan@...cinc.com> wrote: > > > > 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@...cinc.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@...cinc.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. Well, I'd suggest to check a separate single-function interface approach. The reason is pretty simple: for DPU 3.x/4.x we'd need a completely different programming approach. And if at some point we will consider going for an sblk / non-sblk implementation for DPU 8.x+ / 5.x-7.x we'd also need to change that. Thus I think it's better to have less ties between hw_wb and hw_cwb. > > [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 -- With best wishes Dmitry
Powered by blists - more mailing lists