Re: [PATCH v2 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings
On 23/12/2024 22:31, Akhil P Oommen wrote: > On 12/23/2024 5:24 PM, Dmitry Baryshkov wrote: >> On Mon, Dec 23, 2024 at 12:31:27PM +0100, Konrad Dybcio wrote: >>> On 4.12.2024 7:18 PM, Akhil P Oommen wrote: On 11/16/2024 1:17 AM, Dmitry Baryshkov wrote: > On Fri, 15 Nov 2024 at 19:54, Akhil P Oommen > wrote: >> >> On 11/15/2024 3:54 AM, Dmitry Baryshkov wrote: >>> Hello Akhil, >>> >>> On Thu, 14 Nov 2024 at 20:50, Akhil P Oommen >>> wrote: On 11/1/2024 9:54 PM, Akhil P Oommen wrote: > On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote: >> On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote: >>> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote: On Mon, Oct 21, 2024 at 05:23:43PM +0530, Akhil P Oommen wrote: > Add a new schema which extends opp-v2 to support a new vendor > specific > property required for Adreno GPUs found in Qualcomm's SoCs. The > new > property called "qcom,opp-acd-level" carries a u32 value > recommended > for each opp needs to be shared to GMU during runtime. > > Cc: Rob Clark > Signed-off-by: Akhil P Oommen > --- > .../bindings/opp/opp-v2-qcom-adreno.yaml | 96 > ++ > 1 file changed, 96 insertions(+) > > diff --git > a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml > b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml > new file mode 100644 > index ..6d50c0405ef8 > --- /dev/null > +++ > b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml > @@ -0,0 +1,96 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm Adreno compatible OPP supply > + > +description: > + Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an > OPP specific > + ACD related information tailored for the specific chipset. > This binding > + provides the information needed to describe such a hardware > value. > + > +maintainers: > + - Rob Clark > + > +allOf: > + - $ref: opp-v2-base.yaml# > + > +properties: > + compatible: > +items: > + - const: operating-points-v2-adreno > + - const: operating-points-v2 > + > +patternProperties: > + '^opp-?[0-9]+$': '-' should not be optional. opp1 is not expected name. >>> >>> Agree. Will change this to '^opp-[0-9]+$' >>> > +type: object > +additionalProperties: false > + > +properties: > + opp-hz: true > + > + opp-level: true > + > + opp-peak-kBps: true > + > + opp-supported-hw: true > + > + qcom,opp-acd-level: > +description: | > + A positive value representing the ACD (Adaptive Clock > Distribution, > + a fancy name for clk throttling during voltage droop) > level associated > + with this OPP node. This value is shared to a > co-processor inside GPU > + (called Graphics Management Unit a.k.a GMU) during > wake up. It may not > + be present for some OPPs and GMU will disable ACD > while transitioning > + to that OPP. This value encodes a voltage threshold > and few other knobs > + which are identified by characterization of the SoC. > So, it doesn't have > + any unit. Thanks for explanation and other updates. I am still not happy with this property. I do not see reason why DT should encode magic values in a quite generic piece of code. This creates poor ABI, difficult to maintain or understand. >>> >>> Configuring GPU ACD block with its respective value is a >>> requirement for each OPP. >>> So OPP node seems like the natural place for this data. >>> >>> If it helps to resolve
Re: [PATCH v3 10/23] drm/msm/dpu: Add dpu_hw_cwb abstraction for CWB block
On Wed, Oct 16, 2024 at 06:21:16PM -0700, Jessica Zhang wrote:
> The CWB mux has its own registers and set of operations. Add dpu_hw_cwb
> abstraction to allow driver to configure the CWB mux.
>
> Reviewed-by: Dmitry Baryshkov
> Signed-off-by: Jessica Zhang
> ---
> drivers/gpu/drm/msm/Makefile| 1 +
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cwb.c | 73
> +
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cwb.h | 70 +++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 5 +-
> 4 files changed, 148 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index
> de7cf60d206241ac03d7e744e048cbfd845f6cc9..26bacd93a148288849d5076c73ef4f294ff2c188
> 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -78,6 +78,7 @@ msm-display-$(CONFIG_DRM_MSM_DPU) += \
> disp/dpu1/dpu_hw_catalog.o \
> disp/dpu1/dpu_hw_cdm.o \
> disp/dpu1/dpu_hw_ctl.o \
> + disp/dpu1/dpu_hw_cwb.o \
> disp/dpu1/dpu_hw_dsc.o \
> disp/dpu1/dpu_hw_dsc_1_2.o \
> disp/dpu1/dpu_hw_interrupts.o \
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cwb.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cwb.c
> new file mode 100644
> index
> ..5fbf52906ea94847a8eb3fcaa372e582dce2357c
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cwb.c
> @@ -0,0 +1,73 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved
> + */
> +
> +#include
> +#include "dpu_hw_cwb.h"
> +
> +#define CWB_MUX 0x000
> +#define CWB_MODE 0x004
> +
> +/* CWB mux block bit definitions */
> +#define CWB_MUX_MASK GENMASK(3, 0)
> +#define CWB_MODE_MASKGENMASK(2, 0)
> +
> +static void dpu_hw_cwb_config(struct dpu_hw_cwb *ctx,
> + struct dpu_hw_cwb_setup_cfg *cwb_cfg)
> +{
> + struct dpu_hw_blk_reg_map *c = &ctx->hw;
> + int cwb_mux_cfg = 0xF;
> + enum dpu_pingpong pp;
> + enum cwb_mode_input input;
> +
> + if (!cwb_cfg)
> + return;
> +
> + input = cwb_cfg->input;
> + pp = cwb_cfg->pp_idx;
> +
> + if (input >= INPUT_MODE_MAX)
> + return;
> +
> + /*
> + * The CWB_MUX register takes the pingpong index for the real-time
> + * display
> + */
> + if ((pp != PINGPONG_NONE) && (pp < PINGPONG_MAX))
> + cwb_mux_cfg = FIELD_PREP(CWB_MUX_MASK, pp - PINGPONG_0);
> +
> + input = FIELD_PREP(CWB_MODE_MASK, input);
Without #include this triggers an error on some
platforms as reported by LKP. I'll fix that in place.
> +
> + DPU_REG_WRITE(c, CWB_MUX, cwb_mux_cfg);
> + DPU_REG_WRITE(c, CWB_MODE, input);
> +}
> +
--
With best wishes
Dmitry
Re: [PATCH v2 0/8] drm/msm/dpu: catalog corrections
On Fri, 20 Dec 2024 03:28:28 +0200, Dmitry Baryshkov wrote: > After checking the DSPP units in the catalog vs the vendor devicetrees, > link several DSPP units to the corresponding LM units. Each correction > is submitted separately in order to be able to track and apply / skip > them separately based on the feedback from Qualcomm. I think at this > point only CrOS compositor actually uses CTM, so these changes do not > need to be backported (none of the CrOS-enabled devices are touched by > these patches). > > [...] Applied, thanks! [1/8] drm/msm/dpu: provide DSPP and correct LM config for SDM670 https://gitlab.freedesktop.org/lumag/msm/-/commit/9a20f33495bf [2/8] drm/msm/dpu: link DSPP_2/_3 blocks on SM8150 https://gitlab.freedesktop.org/lumag/msm/-/commit/ac440a31e523 [3/8] drm/msm/dpu: link DSPP_2/_3 blocks on SC8180X https://gitlab.freedesktop.org/lumag/msm/-/commit/0986163245df [4/8] drm/msm/dpu: link DSPP_2/_3 blocks on SM8250 https://gitlab.freedesktop.org/lumag/msm/-/commit/8252028092f8 [5/8] drm/msm/dpu: link DSPP_2/_3 blocks on SM8350 https://gitlab.freedesktop.org/lumag/msm/-/commit/42323d3c9e04 [6/8] drm/msm/dpu: link DSPP_2/_3 blocks on SM8550 https://gitlab.freedesktop.org/lumag/msm/-/commit/e21f9d85b053 [7/8] drm/msm/dpu: link DSPP_2/_3 blocks on SM8650 https://gitlab.freedesktop.org/lumag/msm/-/commit/3d3ca0915aa3 [8/8] drm/msm/dpu: link DSPP_2/_3 blocks on X1E80100 https://gitlab.freedesktop.org/lumag/msm/-/commit/3a7a4bebe0db Best regards, -- Dmitry Baryshkov
Re: [PATCH v4 00/25] drm/msm/dpu: Add Concurrent Writeback Support for DPU 10.x+
On Mon, 16 Dec 2024 16:43:11 -0800, Jessica Zhang wrote: > DPU supports a single writeback session running concurrently with primary > display when the CWB mux is configured properly. This series enables > clone mode for DPU driver and adds support for programming the CWB mux > in cases where the hardware has dedicated CWB pingpong blocks. Currently, > the CWB hardware blocks have only been added to the SM8650 > hardware catalog and only DSI has been exposed as a possible_clone of WB. > > [...] Applied, thanks! [05/25] drm/msm/dpu: get rid of struct dpu_rm_requirements https://gitlab.freedesktop.org/lumag/msm/-/commit/835d10620445 [09/25] drm/msm/dpu: Add CWB entry to catalog for SM8650 https://gitlab.freedesktop.org/lumag/msm/-/commit/989412edae5b [10/25] drm/msm/dpu: Specify dedicated CWB pingpong blocks https://gitlab.freedesktop.org/lumag/msm/-/commit/d1fe88dd53ae [11/25] drm/msm/dpu: add devcoredumps for cwb registers https://gitlab.freedesktop.org/lumag/msm/-/commit/675c1edfa92d [12/25] drm/msm/dpu: Add dpu_hw_cwb abstraction for CWB block https://gitlab.freedesktop.org/lumag/msm/-/commit/aae8736426c6 [13/25] drm/msm/dpu: add CWB support to dpu_hw_wb https://gitlab.freedesktop.org/lumag/msm/-/commit/a31a610fd44b [14/25] drm/msm/dpu: Add RM support for allocating CWB https://gitlab.freedesktop.org/lumag/msm/-/commit/a5463629299b Best regards, -- Dmitry Baryshkov
Re: [PATCH 0/4] dt-bindings: msm/dp: add support for pixel clock to driver another stream
On Mon, 02 Dec 2024 19:31:38 -0800, Abhinav Kumar wrote: > On some MSM chipsets, the display port controller is capable of supporting > two streams. To drive the second stream, the pixel clock for the corresponding > stream needs to be enabled. In order to add the bindings for the pixel clock > for the second stream, fixup the documentation of some of the bindings to > clarify exactly which stream they correspond to, then add the new bindings. > > In addition, to help out with reviews for dp-controller bindings, add myself > as the maintainter. > > [...] Applied, thanks! [4/4] dt-bindings: display: msm: dp: update maintainer entry https://gitlab.freedesktop.org/lumag/msm/-/commit/c36c60d1f742 Best regards, -- Dmitry Baryshkov
Re: [PATCH 0/3] drm/msm/mdp4: fix probe deferral issues
On Sat, 20 Apr 2024 05:33:00 +0300, Dmitry Baryshkov wrote: > While testing MDP4 LVDS support I noticed several issues (two are > related to probe deferral case and last one is a c&p error in LCDC > part). Fix those issues. > > Applied, thanks! [1/3] drm/msm: don't clean up priv->kms prematurely https://gitlab.freedesktop.org/lumag/msm/-/commit/ebc0deda3c29 [3/3] drm/msm/mdp4: correct LCDC regulator name https://gitlab.freedesktop.org/lumag/msm/-/commit/8aa337cbe7a6 Best regards, -- Dmitry Baryshkov
Re: [PATCH v4 00/16] drm/msm/dp: perform misc cleanups
On Mon, 16 Dec 2024 00:44:05 +0200, Dmitry Baryshkov wrote: > - Fix register programming in the dp_audio module > - Rework most of the register programming functions to be local to the > calling module rather than accessing everything through huge > dp_catalog monster. > > Applied, thanks! [01/16] drm/msm/dp: drop msm_dp_panel_dump_regs() and msm_dp_catalog_dump_regs() https://gitlab.freedesktop.org/lumag/msm/-/commit/ba3627bf82c1 [02/16] drm/msm/dp: use msm_dp_utils_pack_sdp_header() for audio packets https://gitlab.freedesktop.org/lumag/msm/-/commit/486de5eec0d8 [03/16] drm/msm/dp: drop obsolete audio headers access through catalog https://gitlab.freedesktop.org/lumag/msm/-/commit/c0caebf37960 [04/16] drm/msm/dp: drop struct msm_dp_panel_in https://gitlab.freedesktop.org/lumag/msm/-/commit/429783c22fe9 [05/16] drm/msm/dp: stop passing panel to msm_dp_audio_get() https://gitlab.freedesktop.org/lumag/msm/-/commit/c9261bcc1546 Best regards, -- Dmitry Baryshkov
Re: [PATCH] drm/msm: fix -Wformat-security warnings
On Mon, 16 Dec 2024 09:33:13 +0100, Arnd Bergmann wrote: > Passing a variable string as a printf style format is potentially > dangerous that -Wformat-security can warn about if enabled. A new > instance just got added: > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c: In function 'dpu_kms_mdp_snapshot': > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c:1046:49: error: format not a string > literal and no format arguments [-Werror=format-security] > 1046 | vbif->name); > | ^~ > > [...] Applied, thanks! [1/1] drm/msm: fix -Wformat-security warnings https://gitlab.freedesktop.org/lumag/msm/-/commit/49c2e01be19c Best regards, -- Dmitry Baryshkov
Re: [PATCH] drm/msm: Check return value of of_dma_configure()
On Mon, 04 Nov 2024 17:07:38 +0800, Sui Jingfeng wrote: > Because the of_dma_configure() will returns '-EPROBE_DEFER' if the probe > procedure of the specific platform IOMMU driver is not finished yet. It > can also return other error code for various reasons. > > Stop pretending that it will always suceess, quit if it fail. > > > [...] Applied, thanks! [1/1] drm/msm: Check return value of of_dma_configure() https://gitlab.freedesktop.org/lumag/msm/-/commit/b34a7401ffae Best regards, -- Dmitry Baryshkov
Re: [PATCH v4 06/25] drm/msm/dpu: switch RM to use crtc_id rather than enc_id for allocation
On Tue, Dec 24, 2024 at 06:45:07AM +0200, Dmitry Baryshkov wrote:
> On Mon, Dec 16, 2024 at 04:43:17PM -0800, Jessica Zhang wrote:
> > From: Dmitry Baryshkov
> >
> > Up to now the driver has been using encoder to allocate hardware
> > resources. Switch it to use CRTC id in preparation for the next step.
> >
> > Reviewed-by: Abhinav Kumar
> > Signed-off-by: Dmitry Baryshkov
> > Signed-off-by: Jessica Zhang
> > ---
> > Changes in v4 (due to rebase):
> >
> > - moved *_get_assigned_resources() changes for DSPP and LM from
> > encoder *_virt_atomic_mode_set() to *_assign_crtc_resources()
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 20 +--
> > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 12 +-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 189
> > ++--
> > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 7 +-
> > 4 files changed, 110 insertions(+), 118 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index
> > 5172ab4dea995a154cd88d05c3842d7425fc34ce..e6f930dd34566d01223823de82c922668e6be300
> > 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -715,11 +715,11 @@ static void dpu_encoder_assign_crtc_resources(struct
> > dpu_kms *dpu_kms,
> > memset(cstate->mixers, 0, sizeof(cstate->mixers));
> >
> > num_ctl = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
> > - drm_enc->base.id, DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl));
> > + drm_enc->crtc, DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl));
>
> And this crashes because drm_enc->crtc isn't set yet. Was this commit
> tested? Or was it only tested as a part of the patchset, after this code
> has been rewritten by the subsequent patches? I will replace this with
> crtc_state->crtc, but generatlly this looks bad.
Even with this in place, it fails the kms_color@ctm-max for pipe-B-eDP-1
on SC7180 (pipe-A-eDP-1 succeeds). See job log at [1].
corresponding excerpt from the log:
[IGT] kms_color: starting dynamic subtest pipe-B-eDP-1
[drm:_dpu_rm_check_lm_and_get_connected_blks] [dpu error]failed to get dspp on
lm 0
[drm:_dpu_rm_make_reservation] [dpu error]unable to find appropriate mixers
[drm:dpu_rm_reserve] [dpu error]failed to reserve hw resources: -119
[IGT] kms_color: finished subtest pipe-B-eDP-1, FAIL
FWIW it looks like during an attempt to use second CRTC the driver
doesn't release resources for the first one. I'll drop this patch for
now, leaving just HW_CWB patches in.
[1] https://gitlab.freedesktop.org/drm/msm/-/jobs/68614460/viewer
>
> > num_lm = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
> > - drm_enc->base.id, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm));
> > + drm_enc->crtc, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm));
> > num_dspp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
> > - drm_enc->base.id, DPU_HW_BLK_DSPP, hw_dspp,
> > + drm_enc->crtc, DPU_HW_BLK_DSPP, hw_dspp,
>
>
> > ARRAY_SIZE(hw_dspp));
> >
> > for (i = 0; i < num_lm; i++) {
> > @@ -796,11 +796,11 @@ static int dpu_encoder_virt_atomic_check(
> > * Dont allocate when active is false.
> > */
> > if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> > - dpu_rm_release(global_state, drm_enc);
> > + dpu_rm_release(global_state, crtc_state->crtc);
> >
> > if (!crtc_state->active_changed || crtc_state->enable)
> > ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
> > - drm_enc, crtc_state, &topology);
> > + crtc_state->crtc, &topology);
> > if (!ret)
> > dpu_encoder_assign_crtc_resources(dpu_kms, drm_enc,
> > global_state,
> > crtc_state);
> > @@ -1244,17 +1244,17 @@ static void dpu_encoder_virt_atomic_mode_set(struct
> > drm_encoder *drm_enc,
> >
> > /* Query resource that have been reserved in atomic check step. */
> > num_pp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
> > - drm_enc->base.id, DPU_HW_BLK_PINGPONG, hw_pp,
> > + drm_enc->crtc, DPU_HW_BLK_PINGPONG, hw_pp,
> > ARRAY_SIZE(hw_pp));
> > num_ctl = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
> > - drm_enc->base.id, DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl));
> > + drm_enc->crtc, DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl));
> >
> > for (i = 0; i < MAX_CHANNELS_PER_ENC; i++)
> > dpu_enc->hw_pp[i] = i < num_pp ? to_dpu_hw_pingpong(hw_pp[i])
> > : NULL;
> >
> > num_dsc = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
> > - drm_enc->base.id,
> > DPU_HW_BLK_DSC,
> > +
