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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
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

Powered by Openwall GNU/*/Linux Powered by OpenVZ