Message ID | 20231023221250.116500-1-robdclark@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:ce89:0:b0:403:3b70:6f57 with SMTP id p9csp1582284vqx; Mon, 23 Oct 2023 15:13:37 -0700 (PDT) X-Google-Smtp-Source: AGHT+IG0Rx58nLiwepjcyWaS3QJXU7b190HHeCXT0lE6/A3y2E4ekX612sB0yFxzoszX31B4Y/3X X-Received: by 2002:a05:6358:16d4:b0:168:e443:fbf4 with SMTP id r20-20020a05635816d400b00168e443fbf4mr3200217rwl.30.1698099216855; Mon, 23 Oct 2023 15:13:36 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1698099216; cv=none; d=google.com; s=arc-20160816; b=gh8EHRKvUz9kbpYaU3emFFLe1JuJHVstOeMAFbhQMwEkGxdN7BnarludK29plvSHjg SYwVCZrD+gxB+lCVsW1ITmOF2EEdQghnP0GDcrovw958tAo/gceG37IhQib/OJAeEinh FBTUJxQrA/GPliuCz0867nLvPiBvxVtpd6LQhkQa4RiU91EX+c2BbP0cgjl2585G4CC/ DquzqN1v2vA7JbI9ocfMlwNwkbnryahz4Yq1b+IBzg1Enc4wWqkhDF/XY3RJNJBon6H6 16zVFq/zvR7SmAGIm+Oz4VQRJ7sfKWy8lAg5dnx/gydEhEYeMt4K1K5w7xt6in0GR/6W Tlfw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=s3JBzSeEvFvVT5yIjIih/KnJNivc+rIAI/D1WIJlFoQ=; fh=vQBOSmt2wcszzA5CKuBcE3pkQbLFWtpWL/Norl0iHX8=; b=whqJ9JciRidLaWsgViHW17hQM658xqu9toQK30G624R+IX3WTMeVbwMh6bfWPexPe2 5C1cHRo2uQamVnOSajbYg4njsVtxFm7k4mYVgzIt3lchFWd4MR8Yyc4UUN32swUB21XA roC4YRHJAKpuBuEPyoO8mRBvuVQdqSp7NvZdmNgEieFAOoKES0Lr4ajLM7Rm9KoHtbRL j1vjoBzejFOXKx48U+4fibDPPoEgdDbgYzLPiB5Ua65Te6VvwHvqo0ca9ijsYUWn9rb6 C1vlgffl2iIZC/taeUJChZHMeDBnV5M6WqD7WGKRtHee+AygcmoaENuQz3D1PsSEsjoc 91LQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=XIzDcZ1F; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from lipwig.vger.email (lipwig.vger.email. [23.128.96.33]) by mx.google.com with ESMTPS id a23-20020a637f17000000b005b7c7eee133si6985457pgd.687.2023.10.23.15.13.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 23 Oct 2023 15:13:36 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=XIzDcZ1F; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by lipwig.vger.email (Postfix) with ESMTP id 7A673803112A; Mon, 23 Oct 2023 15:13:34 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at lipwig.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231680AbjJWWM6 (ORCPT <rfc822;aposhian.dev@gmail.com> + 27 others); Mon, 23 Oct 2023 18:12:58 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52832 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229657AbjJWWM5 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 23 Oct 2023 18:12:57 -0400 Received: from mail-pf1-x435.google.com (mail-pf1-x435.google.com [IPv6:2607:f8b0:4864:20::435]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id F237699; Mon, 23 Oct 2023 15:12:54 -0700 (PDT) Received: by mail-pf1-x435.google.com with SMTP id d2e1a72fcca58-6b1ef786b7fso3645607b3a.3; Mon, 23 Oct 2023 15:12:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1698099174; x=1698703974; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=s3JBzSeEvFvVT5yIjIih/KnJNivc+rIAI/D1WIJlFoQ=; b=XIzDcZ1FnkNj0M73Lf8QTvsi1wqLJHhOWaMSTGnSodghCA+l2OjZ4KUl8qUXXEqz2e IYXTCc8uq97H3dpohjpgv0zRRaTV60PyiPy/H13hk9cItx8ufKp7pAIwLCfiinrfWMWy lhqcCYDKp8hS+jnKb3TP5wbeeRsNLTWCgwSNpbKNFkiUwdaaDoxlOfRJ+NmzkXbr8m5N L+wHIKhw8oedZRPa86/IZ+aXDDLspuhHC1CIQCDM3rcRR3NXZ2cGUfzqT5mnCD3pk0F1 xHurENYR2vaPc4GEjkmZ3rxt3c/MouU972e211x4a8L83bsm0LHnKWB2S5Tf545Fp/Vq a1WQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698099174; x=1698703974; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=s3JBzSeEvFvVT5yIjIih/KnJNivc+rIAI/D1WIJlFoQ=; b=ImVLqKqlZHCaFluyLp+rcGF0U3Mpx/DsD6iL685XCLjdWUwd7AC+GI11jnzluLk5MK +irDQ+NBP6PGcjT2ehS8AUFhRvpzmm7TCXGHcmnN7h2w9YOsssD3+idQahE9IcMoHCXg OAJbV+e3+2EauJtv09DPD9UvJZoVNbYap53rrke5VapV7sogax3/u8E//vwCCLGFsVDG zgkmTQ2v7WfLurkdgrliLjW8a59E1MmkGdD6X3yfKXSCRk4OSRe2Sjq/v342xZ7fia3N c6QuP0OZ+7Y7R7bBToInMP/pC6HKZI+29AsAlb6F5aVKru01yPX1Tigdeq8gGbQkMKHT WP5g== X-Gm-Message-State: AOJu0YzvMsYinQCGws3J2xrjKk7ILi2Q1huqGMlHxYCT27n42Og+FvRB gbBGyyM9OnMWwpXznS50lQs= X-Received: by 2002:a05:6a20:8e0e:b0:129:d944:2e65 with SMTP id y14-20020a056a208e0e00b00129d9442e65mr1088606pzj.13.1698099174190; Mon, 23 Oct 2023 15:12:54 -0700 (PDT) Received: from localhost ([2a00:79e1:abd:4a00:6c80:7c10:75a0:44f4]) by smtp.gmail.com with ESMTPSA id u7-20020a17090282c700b001bbb25dd3a7sm6317920plz.187.2023.10.23.15.12.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 23 Oct 2023 15:12:53 -0700 (PDT) From: Rob Clark <robdclark@gmail.com> To: dri-devel@lists.freedesktop.org Cc: freedreno@lists.freedesktop.org, linux-arm-msm@vger.kernel.org, Abhinav Kumar <quic_abhinavk@quicinc.com>, Jessica Zhang <quic_jesszhan@quicinc.com>, Rob Clark <robdclark@chromium.org>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>, Marijn Suijten <marijn.suijten@somainline.org>, David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, Vinod Polimera <quic_vpolimer@quicinc.com>, Jiasheng Jiang <jiasheng@iscas.ac.cn>, Kalyan Thota <quic_kalyant@quicinc.com>, Kuogee Hsieh <quic_khsieh@quicinc.com>, Arnaud Vrac <rawoul@gmail.com>, Konrad Dybcio <konrad.dybcio@linaro.org>, Jeykumar Sankaran <quic_jeykumar@quicinc.com>, linux-kernel@vger.kernel.org (open list) Subject: [PATCH] drm/msm/dpu: Fix encoder CRC to account for CTM enablement Date: Mon, 23 Oct 2023 15:12:33 -0700 Message-ID: <20231023221250.116500-1-robdclark@gmail.com> X-Mailer: git-send-email 2.41.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.6 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Mon, 23 Oct 2023 15:13:34 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1780586084090284510 X-GMAIL-MSGID: 1780586084090284510 |
Series |
drm/msm/dpu: Fix encoder CRC to account for CTM enablement
|
|
Commit Message
Rob Clark
Oct. 23, 2023, 10:12 p.m. UTC
From: Rob Clark <robdclark@chromium.org> Seems like we need to pick INPUT_SEL=1 when CTM is enabled. But not otherwise. Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Signed-off-by: Rob Clark <robdclark@chromium.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 4 ++-- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 3 ++- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 ++-- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 5 ++++- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 3 ++- 8 files changed, 15 insertions(+), 10 deletions(-)
Comments
On Tue, 24 Oct 2023 at 01:12, Rob Clark <robdclark@gmail.com> wrote: > > From: Rob Clark <robdclark@chromium.org> > > Seems like we need to pick INPUT_SEL=1 when CTM is enabled. But not > otherwise. > > Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Signed-off-by: Rob Clark <robdclark@chromium.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 4 ++-- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 3 ++- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 ++-- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 2 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 2 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 5 ++++- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 3 ++- > 8 files changed, 15 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index 2b83a13b3aa9..d93a92ffd5df 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -134,7 +134,7 @@ static void dpu_crtc_setup_encoder_misr(struct drm_crtc *crtc) > struct drm_encoder *drm_enc; > > drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask) > - dpu_encoder_setup_misr(drm_enc); > + dpu_encoder_setup_misr(drm_enc, !!crtc->state->ctm); > } > > static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name) > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index b0a7908418ed..12ee7acb5ea6 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -241,7 +241,7 @@ int dpu_encoder_get_crc_values_cnt(const struct drm_encoder *drm_enc) > return num_intf; > } > > -void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc) > +void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc, bool has_ctm) > { > struct dpu_encoder_virt *dpu_enc; > > @@ -255,7 +255,7 @@ void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc) > if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr) > continue; > > - phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1); > + phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1, has_ctm); > } > } > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > index 4c05fd5e9ed1..510783b2fb24 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > @@ -169,8 +169,9 @@ int dpu_encoder_get_crc_values_cnt(const struct drm_encoder *drm_enc); > /** > * dpu_encoder_setup_misr - enable misr calculations > * @drm_enc: Pointer to previously created drm encoder structure > + * @has_ctm: Is CTM enabled > */ > -void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder); > +void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder, bool has_ctm); > > /** > * dpu_encoder_get_crc - get the crc value from interface blocks > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > index e8b8908d3e12..cb06f80cc671 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > @@ -318,9 +318,9 @@ static u32 dpu_hw_intf_get_line_count(struct dpu_hw_intf *intf) > return DPU_REG_READ(c, INTF_LINE_COUNT); > } > > -static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool enable, u32 frame_count) > +static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool enable, u32 frame_count, bool has_ctm) > { > - dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, enable, frame_count); > + dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, enable, frame_count, has_ctm); I'm not sure about the dpu_encoder and dpu_hw_intf interfaces. But dpu_hw_setup_misr definitely needs the `u8 input_sel` parameter instead of `bool has_ctm`. Most likely, I'd use u8 for dpu_hw_intf operation too. Could you please adjust? > } > > static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 *misr_value) > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h > index c539025c418b..95aafc4cf58e 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h > @@ -95,7 +95,7 @@ struct dpu_hw_intf_ops { > > void (*bind_pingpong_blk)(struct dpu_hw_intf *intf, > const enum dpu_pingpong pp); > - void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, u32 frame_count); > + void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, u32 frame_count, bool has_ctm); > int (*collect_misr)(struct dpu_hw_intf *intf, u32 *misr_value); > > // Tearcheck on INTF since DPU 5.0.0 > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c > index d1c3bd8379ea..2efe29396c6a 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c > @@ -83,7 +83,7 @@ static void dpu_hw_lm_setup_border_color(struct dpu_hw_mixer *ctx, > > static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx, bool enable, u32 frame_count) > { > - dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, enable, frame_count); > + dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, enable, frame_count, false); > } > > static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32 *misr_value) > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c > index 9d2273fd2fed..528b8439209f 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c > @@ -483,7 +483,7 @@ void _dpu_hw_setup_qos_lut(struct dpu_hw_blk_reg_map *c, u32 offset, > > void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, > u32 misr_ctrl_offset, > - bool enable, u32 frame_count) > + bool enable, u32 frame_count, bool has_ctm) > { > u32 config = 0; > > @@ -496,6 +496,9 @@ void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, > config = (frame_count & MISR_FRAME_COUNT_MASK) | > MISR_CTRL_ENABLE | MISR_CTRL_FREE_RUN_MASK; > > + if (!has_ctm) > + config |= 1 << 24; Please define MISR_CTRL_INPUT_SEL instead. > + > DPU_REG_WRITE(c, misr_ctrl_offset, config); > } else { > DPU_REG_WRITE(c, misr_ctrl_offset, 0); > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h > index 1f6079f47071..e42d9d00e40e 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h > @@ -360,7 +360,8 @@ void _dpu_hw_setup_qos_lut(struct dpu_hw_blk_reg_map *c, u32 offset, > void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, > u32 misr_ctrl_offset, > bool enable, > - u32 frame_count); > + u32 frame_count, > + bool has_ctm); > > int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c, > u32 misr_ctrl_offset, > -- > 2.41.0 >
On Mon, Oct 23, 2023 at 3:30 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Tue, 24 Oct 2023 at 01:12, Rob Clark <robdclark@gmail.com> wrote: > > > > From: Rob Clark <robdclark@chromium.org> > > > > Seems like we need to pick INPUT_SEL=1 when CTM is enabled. But not > > otherwise. > > > > Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +- > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 4 ++-- > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 3 ++- > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 ++-- > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 2 +- > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 2 +- > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 5 ++++- > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 3 ++- > > 8 files changed, 15 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > index 2b83a13b3aa9..d93a92ffd5df 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > @@ -134,7 +134,7 @@ static void dpu_crtc_setup_encoder_misr(struct drm_crtc *crtc) > > struct drm_encoder *drm_enc; > > > > drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask) > > - dpu_encoder_setup_misr(drm_enc); > > + dpu_encoder_setup_misr(drm_enc, !!crtc->state->ctm); > > } > > > > static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > index b0a7908418ed..12ee7acb5ea6 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > @@ -241,7 +241,7 @@ int dpu_encoder_get_crc_values_cnt(const struct drm_encoder *drm_enc) > > return num_intf; > > } > > > > -void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc) > > +void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc, bool has_ctm) > > { > > struct dpu_encoder_virt *dpu_enc; > > > > @@ -255,7 +255,7 @@ void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc) > > if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr) > > continue; > > > > - phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1); > > + phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1, has_ctm); > > } > > } > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > index 4c05fd5e9ed1..510783b2fb24 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > @@ -169,8 +169,9 @@ int dpu_encoder_get_crc_values_cnt(const struct drm_encoder *drm_enc); > > /** > > * dpu_encoder_setup_misr - enable misr calculations > > * @drm_enc: Pointer to previously created drm encoder structure > > + * @has_ctm: Is CTM enabled > > */ > > -void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder); > > +void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder, bool has_ctm); > > > > /** > > * dpu_encoder_get_crc - get the crc value from interface blocks > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > > index e8b8908d3e12..cb06f80cc671 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > > @@ -318,9 +318,9 @@ static u32 dpu_hw_intf_get_line_count(struct dpu_hw_intf *intf) > > return DPU_REG_READ(c, INTF_LINE_COUNT); > > } > > > > -static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool enable, u32 frame_count) > > +static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool enable, u32 frame_count, bool has_ctm) > > { > > - dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, enable, frame_count); > > + dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, enable, frame_count, has_ctm); > > I'm not sure about the dpu_encoder and dpu_hw_intf interfaces. But > dpu_hw_setup_misr definitely needs the `u8 input_sel` parameter > instead of `bool has_ctm`. That seems a bit premature without knowing what the other values are. (And I also question a bit the whole abstraction layer thing if it is taking directly register bitfield enum's..) BR, -R > Most likely, I'd use u8 for dpu_hw_intf operation too. > > Could you please adjust? > > > } > > > > static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 *misr_value) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h > > index c539025c418b..95aafc4cf58e 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h > > @@ -95,7 +95,7 @@ struct dpu_hw_intf_ops { > > > > void (*bind_pingpong_blk)(struct dpu_hw_intf *intf, > > const enum dpu_pingpong pp); > > - void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, u32 frame_count); > > + void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, u32 frame_count, bool has_ctm); > > int (*collect_misr)(struct dpu_hw_intf *intf, u32 *misr_value); > > > > // Tearcheck on INTF since DPU 5.0.0 > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c > > index d1c3bd8379ea..2efe29396c6a 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c > > @@ -83,7 +83,7 @@ static void dpu_hw_lm_setup_border_color(struct dpu_hw_mixer *ctx, > > > > static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx, bool enable, u32 frame_count) > > { > > - dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, enable, frame_count); > > + dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, enable, frame_count, false); > > } > > > > static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32 *misr_value) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c > > index 9d2273fd2fed..528b8439209f 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c > > @@ -483,7 +483,7 @@ void _dpu_hw_setup_qos_lut(struct dpu_hw_blk_reg_map *c, u32 offset, > > > > void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, > > u32 misr_ctrl_offset, > > - bool enable, u32 frame_count) > > + bool enable, u32 frame_count, bool has_ctm) > > { > > u32 config = 0; > > > > @@ -496,6 +496,9 @@ void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, > > config = (frame_count & MISR_FRAME_COUNT_MASK) | > > MISR_CTRL_ENABLE | MISR_CTRL_FREE_RUN_MASK; > > > > + if (!has_ctm) > > + config |= 1 << 24; > > Please define MISR_CTRL_INPUT_SEL instead. > > > + > > DPU_REG_WRITE(c, misr_ctrl_offset, config); > > } else { > > DPU_REG_WRITE(c, misr_ctrl_offset, 0); > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h > > index 1f6079f47071..e42d9d00e40e 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h > > @@ -360,7 +360,8 @@ void _dpu_hw_setup_qos_lut(struct dpu_hw_blk_reg_map *c, u32 offset, > > void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, > > u32 misr_ctrl_offset, > > bool enable, > > - u32 frame_count); > > + u32 frame_count, > > + bool has_ctm); > > > > int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c, > > u32 misr_ctrl_offset, > > -- > > 2.41.0 > > > > > -- > With best wishes > Dmitry
On Tue, 24 Oct 2023 at 01:36, Rob Clark <robdclark@gmail.com> wrote: > > On Mon, Oct 23, 2023 at 3:30 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Tue, 24 Oct 2023 at 01:12, Rob Clark <robdclark@gmail.com> wrote: > > > > > > From: Rob Clark <robdclark@chromium.org> > > > > > > Seems like we need to pick INPUT_SEL=1 when CTM is enabled. But not > > > otherwise. > > > > > > Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > > --- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 4 ++-- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 3 ++- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 ++-- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 2 +- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 2 +- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 5 ++++- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 3 ++- > > > 8 files changed, 15 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > index 2b83a13b3aa9..d93a92ffd5df 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > @@ -134,7 +134,7 @@ static void dpu_crtc_setup_encoder_misr(struct drm_crtc *crtc) > > > struct drm_encoder *drm_enc; > > > > > > drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask) > > > - dpu_encoder_setup_misr(drm_enc); > > > + dpu_encoder_setup_misr(drm_enc, !!crtc->state->ctm); > > > } > > > > > > static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name) > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > index b0a7908418ed..12ee7acb5ea6 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > @@ -241,7 +241,7 @@ int dpu_encoder_get_crc_values_cnt(const struct drm_encoder *drm_enc) > > > return num_intf; > > > } > > > > > > -void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc) > > > +void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc, bool has_ctm) > > > { > > > struct dpu_encoder_virt *dpu_enc; > > > > > > @@ -255,7 +255,7 @@ void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc) > > > if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr) > > > continue; > > > > > > - phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1); > > > + phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1, has_ctm); > > > } > > > } > > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > > index 4c05fd5e9ed1..510783b2fb24 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > > @@ -169,8 +169,9 @@ int dpu_encoder_get_crc_values_cnt(const struct drm_encoder *drm_enc); > > > /** > > > * dpu_encoder_setup_misr - enable misr calculations > > > * @drm_enc: Pointer to previously created drm encoder structure > > > + * @has_ctm: Is CTM enabled > > > */ > > > -void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder); > > > +void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder, bool has_ctm); > > > > > > /** > > > * dpu_encoder_get_crc - get the crc value from interface blocks > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > > > index e8b8908d3e12..cb06f80cc671 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > > > @@ -318,9 +318,9 @@ static u32 dpu_hw_intf_get_line_count(struct dpu_hw_intf *intf) > > > return DPU_REG_READ(c, INTF_LINE_COUNT); > > > } > > > > > > -static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool enable, u32 frame_count) > > > +static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool enable, u32 frame_count, bool has_ctm) > > > { > > > - dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, enable, frame_count); > > > + dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, enable, frame_count, has_ctm); > > > > I'm not sure about the dpu_encoder and dpu_hw_intf interfaces. But > > dpu_hw_setup_misr definitely needs the `u8 input_sel` parameter > > instead of `bool has_ctm`. > > That seems a bit premature without knowing what the other values are. > (And I also question a bit the whole abstraction layer thing if it is > taking directly register bitfield enum's..) dpu_hw_intf and especially dpu_hw_util are not real abstractions. I always viewed them as useful low-level helpers. I think that has_ctm is valid at the dpu_encoder level, which selects which input to use. on the lower levels has_ctm doesn't make sense. IOW dpu_hw_setup_misr can be used to setup MISR for other blocks, where CTM doesn't exist. > > BR, > -R > > > Most likely, I'd use u8 for dpu_hw_intf operation too. > > > > Could you please adjust? > > > > > } > > > > > > static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 *misr_value) > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h > > > index c539025c418b..95aafc4cf58e 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h > > > @@ -95,7 +95,7 @@ struct dpu_hw_intf_ops { > > > > > > void (*bind_pingpong_blk)(struct dpu_hw_intf *intf, > > > const enum dpu_pingpong pp); > > > - void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, u32 frame_count); > > > + void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, u32 frame_count, bool has_ctm); > > > int (*collect_misr)(struct dpu_hw_intf *intf, u32 *misr_value); > > > > > > // Tearcheck on INTF since DPU 5.0.0 > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c > > > index d1c3bd8379ea..2efe29396c6a 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c > > > @@ -83,7 +83,7 @@ static void dpu_hw_lm_setup_border_color(struct dpu_hw_mixer *ctx, > > > > > > static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx, bool enable, u32 frame_count) > > > { > > > - dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, enable, frame_count); > > > + dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, enable, frame_count, false); > > > } > > > > > > static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32 *misr_value) > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c > > > index 9d2273fd2fed..528b8439209f 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c > > > @@ -483,7 +483,7 @@ void _dpu_hw_setup_qos_lut(struct dpu_hw_blk_reg_map *c, u32 offset, > > > > > > void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, > > > u32 misr_ctrl_offset, > > > - bool enable, u32 frame_count) > > > + bool enable, u32 frame_count, bool has_ctm) > > > { > > > u32 config = 0; > > > > > > @@ -496,6 +496,9 @@ void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, > > > config = (frame_count & MISR_FRAME_COUNT_MASK) | > > > MISR_CTRL_ENABLE | MISR_CTRL_FREE_RUN_MASK; > > > > > > + if (!has_ctm) > > > + config |= 1 << 24; > > > > Please define MISR_CTRL_INPUT_SEL instead. > > > > > + > > > DPU_REG_WRITE(c, misr_ctrl_offset, config); > > > } else { > > > DPU_REG_WRITE(c, misr_ctrl_offset, 0); > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h > > > index 1f6079f47071..e42d9d00e40e 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h > > > @@ -360,7 +360,8 @@ void _dpu_hw_setup_qos_lut(struct dpu_hw_blk_reg_map *c, u32 offset, > > > void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, > > > u32 misr_ctrl_offset, > > > bool enable, > > > - u32 frame_count); > > > + u32 frame_count, > > > + bool has_ctm); > > > > > > int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c, > > > u32 misr_ctrl_offset, > > > -- > > > 2.41.0 > > > > > > > > > -- > > With best wishes > > Dmitry
On 10/23/2023 4:03 PM, Dmitry Baryshkov wrote: > On Tue, 24 Oct 2023 at 01:36, Rob Clark <robdclark@gmail.com> wrote: >> >> On Mon, Oct 23, 2023 at 3:30 PM Dmitry Baryshkov >> <dmitry.baryshkov@linaro.org> wrote: >>> >>> On Tue, 24 Oct 2023 at 01:12, Rob Clark <robdclark@gmail.com> wrote: >>>> >>>> From: Rob Clark <robdclark@chromium.org> >>>> >>>> Seems like we need to pick INPUT_SEL=1 when CTM is enabled. But not >>>> otherwise. >>>> >>>> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> Signed-off-by: Rob Clark <robdclark@chromium.org> >>>> --- I cannot find anything in the docs which suggest this solution is correct. Different blocks in the DPU pipeline have their own CRC (MISR) registers like LM, intf etc. We dont need to change INPUT_SEL to tell DPU from which pipeline to take the CRC from as each of them have their own registers. INPUT_SEL is controlling whether the CRC needs to be calculated over the entire display timings or only the active pixels. I am unable to tell at the moment why this is making a difference in this use-case. Since I am unable to find any documentation proving this solution is correct so far, unfortunately I would hold this back till then. We will investigate this issue and report our findings on this thread on how to proceed. >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 4 ++-- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 3 ++- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 ++-- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 2 +- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 2 +- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 5 ++++- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 3 ++- >>>> 8 files changed, 15 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>> index 2b83a13b3aa9..d93a92ffd5df 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>> @@ -134,7 +134,7 @@ static void dpu_crtc_setup_encoder_misr(struct drm_crtc *crtc) >>>> struct drm_encoder *drm_enc; >>>> >>>> drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask) >>>> - dpu_encoder_setup_misr(drm_enc); >>>> + dpu_encoder_setup_misr(drm_enc, !!crtc->state->ctm); >>>> } >>>> >>>> static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name) >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> index b0a7908418ed..12ee7acb5ea6 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> @@ -241,7 +241,7 @@ int dpu_encoder_get_crc_values_cnt(const struct drm_encoder *drm_enc) >>>> return num_intf; >>>> } >>>> >>>> -void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc) >>>> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc, bool has_ctm) >>>> { >>>> struct dpu_encoder_virt *dpu_enc; >>>> >>>> @@ -255,7 +255,7 @@ void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc) >>>> if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr) >>>> continue; >>>> >>>> - phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1); >>>> + phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1, has_ctm); >>>> } >>>> } >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>> index 4c05fd5e9ed1..510783b2fb24 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>> @@ -169,8 +169,9 @@ int dpu_encoder_get_crc_values_cnt(const struct drm_encoder *drm_enc); >>>> /** >>>> * dpu_encoder_setup_misr - enable misr calculations >>>> * @drm_enc: Pointer to previously created drm encoder structure >>>> + * @has_ctm: Is CTM enabled >>>> */ >>>> -void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder); >>>> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder, bool has_ctm); >>>> >>>> /** >>>> * dpu_encoder_get_crc - get the crc value from interface blocks >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>> index e8b8908d3e12..cb06f80cc671 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>> @@ -318,9 +318,9 @@ static u32 dpu_hw_intf_get_line_count(struct dpu_hw_intf *intf) >>>> return DPU_REG_READ(c, INTF_LINE_COUNT); >>>> } >>>> >>>> -static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool enable, u32 frame_count) >>>> +static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool enable, u32 frame_count, bool has_ctm) >>>> { >>>> - dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, enable, frame_count); >>>> + dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, enable, frame_count, has_ctm); >>> >>> I'm not sure about the dpu_encoder and dpu_hw_intf interfaces. But >>> dpu_hw_setup_misr definitely needs the `u8 input_sel` parameter >>> instead of `bool has_ctm`. >> >> That seems a bit premature without knowing what the other values are. >> (And I also question a bit the whole abstraction layer thing if it is >> taking directly register bitfield enum's..) > > dpu_hw_intf and especially dpu_hw_util are not real abstractions. I > always viewed them as useful low-level helpers. > > I think that has_ctm is valid at the dpu_encoder level, which selects > which input to use. on the lower levels has_ctm doesn't make sense. > IOW dpu_hw_setup_misr can be used to setup MISR for other blocks, > where CTM doesn't exist. > >> >> BR, >> -R >> >>> Most likely, I'd use u8 for dpu_hw_intf operation too. >>> >>> Could you please adjust? >>> >>>> } >>>> >>>> static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 *misr_value) >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h >>>> index c539025c418b..95aafc4cf58e 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h >>>> @@ -95,7 +95,7 @@ struct dpu_hw_intf_ops { >>>> >>>> void (*bind_pingpong_blk)(struct dpu_hw_intf *intf, >>>> const enum dpu_pingpong pp); >>>> - void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, u32 frame_count); >>>> + void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, u32 frame_count, bool has_ctm); >>>> int (*collect_misr)(struct dpu_hw_intf *intf, u32 *misr_value); >>>> >>>> // Tearcheck on INTF since DPU 5.0.0 >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >>>> index d1c3bd8379ea..2efe29396c6a 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >>>> @@ -83,7 +83,7 @@ static void dpu_hw_lm_setup_border_color(struct dpu_hw_mixer *ctx, >>>> >>>> static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx, bool enable, u32 frame_count) >>>> { >>>> - dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, enable, frame_count); >>>> + dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, enable, frame_count, false); >>>> } >>>> >>>> static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32 *misr_value) >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >>>> index 9d2273fd2fed..528b8439209f 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >>>> @@ -483,7 +483,7 @@ void _dpu_hw_setup_qos_lut(struct dpu_hw_blk_reg_map *c, u32 offset, >>>> >>>> void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, >>>> u32 misr_ctrl_offset, >>>> - bool enable, u32 frame_count) >>>> + bool enable, u32 frame_count, bool has_ctm) >>>> { >>>> u32 config = 0; >>>> >>>> @@ -496,6 +496,9 @@ void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, >>>> config = (frame_count & MISR_FRAME_COUNT_MASK) | >>>> MISR_CTRL_ENABLE | MISR_CTRL_FREE_RUN_MASK; >>>> >>>> + if (!has_ctm) >>>> + config |= 1 << 24; >>> >>> Please define MISR_CTRL_INPUT_SEL instead. >>> >>>> + >>>> DPU_REG_WRITE(c, misr_ctrl_offset, config); >>>> } else { >>>> DPU_REG_WRITE(c, misr_ctrl_offset, 0); >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >>>> index 1f6079f47071..e42d9d00e40e 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >>>> @@ -360,7 +360,8 @@ void _dpu_hw_setup_qos_lut(struct dpu_hw_blk_reg_map *c, u32 offset, >>>> void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, >>>> u32 misr_ctrl_offset, >>>> bool enable, >>>> - u32 frame_count); >>>> + u32 frame_count, >>>> + bool has_ctm); >>>> >>>> int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c, >>>> u32 misr_ctrl_offset, >>>> -- >>>> 2.41.0 >>>> >>> >>> >>> -- >>> With best wishes >>> Dmitry > > >
On 10/24/2023 12:01 PM, Abhinav Kumar wrote: > > > On 10/23/2023 4:03 PM, Dmitry Baryshkov wrote: >> On Tue, 24 Oct 2023 at 01:36, Rob Clark <robdclark@gmail.com> wrote: >>> >>> On Mon, Oct 23, 2023 at 3:30 PM Dmitry Baryshkov >>> <dmitry.baryshkov@linaro.org> wrote: >>>> >>>> On Tue, 24 Oct 2023 at 01:12, Rob Clark <robdclark@gmail.com> wrote: >>>>> >>>>> From: Rob Clark <robdclark@chromium.org> >>>>> >>>>> Seems like we need to pick INPUT_SEL=1 when CTM is enabled. But not >>>>> otherwise. >>>>> >>>>> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>> Signed-off-by: Rob Clark <robdclark@chromium.org> >>>>> --- > > I cannot find anything in the docs which suggest this solution is correct. > > Different blocks in the DPU pipeline have their own CRC (MISR) registers > like LM, intf etc. > > We dont need to change INPUT_SEL to tell DPU from which pipeline to take > the CRC from as each of them have their own registers. > > INPUT_SEL is controlling whether the CRC needs to be calculated over the > entire display timings or only the active pixels. I am unable to tell at > the moment why this is making a difference in this use-case. > > Since I am unable to find any documentation proving this solution is > correct so far, unfortunately I would hold this back till then. > > We will investigate this issue and report our findings on this thread on > how to proceed. > Alright, we debugged and also found some more answers. The correct solution is indeed to set INPUT_SEL = 1 but let me explain why and what should be the correct way. INPUT_SEL was indeed telling whether to compute CRC over active pixels or active pixels + timings like I wrote before but this behavior changed since some chipsets. Now, INPUT_SEL = 0 means compute CRC *only* over timings and not the active area (and not display + timings like before) and like mentioned before this has nothing to do with what is the input to the CRC. Not covering the active area will not change the CRC at all as Rob reported but its not specific to CTM. Which means we should have been setting INPUT_SEL=1 whenever we use INTF CRC irrespective of whether CTM is used or not. What this also means is INTF CRC was not working correctly at all so far irrespecive of CTM or not because it was always computing CRC only on the timings (non-active area). This was not caught so far because it looks like IGT's kms_pipe_crc_basic test which was used to validate this only compares CRC between two frames of the same content to match if they were equal and not changing contents and comparing like kms_plane does. It will pass as CRC would not have changed. Now coming to the fix, the reset value of this register INTF_MISR_CTRL already sets the INPUT_SEL bit (or unsets it) correctly based on whichever DPU version is used so we should just change the dpu_hw_setup_misr() to a read on the register followed by ORing the required bits without touching INPUT_SEL and write. That will address this issue and also cover version control since the expected value of this bit has changed across DPU revisions. >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 4 ++-- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 3 ++- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 ++-- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 2 +- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 2 +- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 5 ++++- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 3 ++- >>>>> 8 files changed, 15 insertions(+), 10 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>>> index 2b83a13b3aa9..d93a92ffd5df 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>>> @@ -134,7 +134,7 @@ static void dpu_crtc_setup_encoder_misr(struct >>>>> drm_crtc *crtc) >>>>> struct drm_encoder *drm_enc; >>>>> >>>>> drm_for_each_encoder_mask(drm_enc, crtc->dev, >>>>> crtc->state->encoder_mask) >>>>> - dpu_encoder_setup_misr(drm_enc); >>>>> + dpu_encoder_setup_misr(drm_enc, !!crtc->state->ctm); >>>>> } >>>>> >>>>> static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const >>>>> char *src_name) >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>>> index b0a7908418ed..12ee7acb5ea6 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>>> @@ -241,7 +241,7 @@ int dpu_encoder_get_crc_values_cnt(const struct >>>>> drm_encoder *drm_enc) >>>>> return num_intf; >>>>> } >>>>> >>>>> -void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc) >>>>> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc, >>>>> bool has_ctm) >>>>> { >>>>> struct dpu_encoder_virt *dpu_enc; >>>>> >>>>> @@ -255,7 +255,7 @@ void dpu_encoder_setup_misr(const struct >>>>> drm_encoder *drm_enc) >>>>> if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr) >>>>> continue; >>>>> >>>>> - phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1); >>>>> + phys->hw_intf->ops.setup_misr(phys->hw_intf, true, >>>>> 1, has_ctm); >>>>> } >>>>> } >>>>> >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>> index 4c05fd5e9ed1..510783b2fb24 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>> @@ -169,8 +169,9 @@ int dpu_encoder_get_crc_values_cnt(const struct >>>>> drm_encoder *drm_enc); >>>>> /** >>>>> * dpu_encoder_setup_misr - enable misr calculations >>>>> * @drm_enc: Pointer to previously created drm encoder structure >>>>> + * @has_ctm: Is CTM enabled >>>>> */ >>>>> -void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder); >>>>> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder, >>>>> bool has_ctm); >>>>> >>>>> /** >>>>> * dpu_encoder_get_crc - get the crc value from interface blocks >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>>> index e8b8908d3e12..cb06f80cc671 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>>> @@ -318,9 +318,9 @@ static u32 dpu_hw_intf_get_line_count(struct >>>>> dpu_hw_intf *intf) >>>>> return DPU_REG_READ(c, INTF_LINE_COUNT); >>>>> } >>>>> >>>>> -static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool >>>>> enable, u32 frame_count) >>>>> +static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool >>>>> enable, u32 frame_count, bool has_ctm) >>>>> { >>>>> - dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, enable, >>>>> frame_count); >>>>> + dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, enable, >>>>> frame_count, has_ctm); >>>> >>>> I'm not sure about the dpu_encoder and dpu_hw_intf interfaces. But >>>> dpu_hw_setup_misr definitely needs the `u8 input_sel` parameter >>>> instead of `bool has_ctm`. >>> >>> That seems a bit premature without knowing what the other values are. >>> (And I also question a bit the whole abstraction layer thing if it is >>> taking directly register bitfield enum's..) >> >> dpu_hw_intf and especially dpu_hw_util are not real abstractions. I >> always viewed them as useful low-level helpers. >> >> I think that has_ctm is valid at the dpu_encoder level, which selects >> which input to use. on the lower levels has_ctm doesn't make sense. >> IOW dpu_hw_setup_misr can be used to setup MISR for other blocks, >> where CTM doesn't exist. >> >>> >>> BR, >>> -R >>> >>>> Most likely, I'd use u8 for dpu_hw_intf operation too. >>>> >>>> Could you please adjust? >>>> >>>>> } >>>>> >>>>> static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 >>>>> *misr_value) >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h >>>>> index c539025c418b..95aafc4cf58e 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h >>>>> @@ -95,7 +95,7 @@ struct dpu_hw_intf_ops { >>>>> >>>>> void (*bind_pingpong_blk)(struct dpu_hw_intf *intf, >>>>> const enum dpu_pingpong pp); >>>>> - void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, >>>>> u32 frame_count); >>>>> + void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, >>>>> u32 frame_count, bool has_ctm); >>>>> int (*collect_misr)(struct dpu_hw_intf *intf, u32 >>>>> *misr_value); >>>>> >>>>> // Tearcheck on INTF since DPU 5.0.0 >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >>>>> index d1c3bd8379ea..2efe29396c6a 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >>>>> @@ -83,7 +83,7 @@ static void dpu_hw_lm_setup_border_color(struct >>>>> dpu_hw_mixer *ctx, >>>>> >>>>> static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx, bool >>>>> enable, u32 frame_count) >>>>> { >>>>> - dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, enable, >>>>> frame_count); >>>>> + dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, enable, >>>>> frame_count, false); >>>>> } >>>>> >>>>> static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32 >>>>> *misr_value) >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >>>>> index 9d2273fd2fed..528b8439209f 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >>>>> @@ -483,7 +483,7 @@ void _dpu_hw_setup_qos_lut(struct >>>>> dpu_hw_blk_reg_map *c, u32 offset, >>>>> >>>>> void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, >>>>> u32 misr_ctrl_offset, >>>>> - bool enable, u32 frame_count) >>>>> + bool enable, u32 frame_count, bool has_ctm) >>>>> { >>>>> u32 config = 0; >>>>> >>>>> @@ -496,6 +496,9 @@ void dpu_hw_setup_misr(struct >>>>> dpu_hw_blk_reg_map *c, >>>>> config = (frame_count & MISR_FRAME_COUNT_MASK) | >>>>> MISR_CTRL_ENABLE | MISR_CTRL_FREE_RUN_MASK; >>>>> >>>>> + if (!has_ctm) >>>>> + config |= 1 << 24; >>>> >>>> Please define MISR_CTRL_INPUT_SEL instead. >>>> >>>>> + >>>>> DPU_REG_WRITE(c, misr_ctrl_offset, config); >>>>> } else { >>>>> DPU_REG_WRITE(c, misr_ctrl_offset, 0); >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >>>>> index 1f6079f47071..e42d9d00e40e 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >>>>> @@ -360,7 +360,8 @@ void _dpu_hw_setup_qos_lut(struct >>>>> dpu_hw_blk_reg_map *c, u32 offset, >>>>> void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, >>>>> u32 misr_ctrl_offset, >>>>> bool enable, >>>>> - u32 frame_count); >>>>> + u32 frame_count, >>>>> + bool has_ctm); >>>>> >>>>> int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c, >>>>> u32 misr_ctrl_offset, >>>>> -- >>>>> 2.41.0 >>>>> >>>> >>>> >>>> -- >>>> With best wishes >>>> Dmitry >> >> >>
On Tue, Nov 21, 2023 at 4:41 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 10/24/2023 12:01 PM, Abhinav Kumar wrote: > > > > > > On 10/23/2023 4:03 PM, Dmitry Baryshkov wrote: > >> On Tue, 24 Oct 2023 at 01:36, Rob Clark <robdclark@gmail.com> wrote: > >>> > >>> On Mon, Oct 23, 2023 at 3:30 PM Dmitry Baryshkov > >>> <dmitry.baryshkov@linaro.org> wrote: > >>>> > >>>> On Tue, 24 Oct 2023 at 01:12, Rob Clark <robdclark@gmail.com> wrote: > >>>>> > >>>>> From: Rob Clark <robdclark@chromium.org> > >>>>> > >>>>> Seems like we need to pick INPUT_SEL=1 when CTM is enabled. But not > >>>>> otherwise. > >>>>> > >>>>> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >>>>> Signed-off-by: Rob Clark <robdclark@chromium.org> > >>>>> --- > > > > I cannot find anything in the docs which suggest this solution is correct. > > > > Different blocks in the DPU pipeline have their own CRC (MISR) registers > > like LM, intf etc. > > > > We dont need to change INPUT_SEL to tell DPU from which pipeline to take > > the CRC from as each of them have their own registers. > > > > INPUT_SEL is controlling whether the CRC needs to be calculated over the > > entire display timings or only the active pixels. I am unable to tell at > > the moment why this is making a difference in this use-case. > > > > Since I am unable to find any documentation proving this solution is > > correct so far, unfortunately I would hold this back till then. > > > > We will investigate this issue and report our findings on this thread on > > how to proceed. > > > > Alright, we debugged and also found some more answers. > > The correct solution is indeed to set INPUT_SEL = 1 but let me explain > why and what should be the correct way. > > INPUT_SEL was indeed telling whether to compute CRC over active pixels > or active pixels + timings like I wrote before but this behavior changed > since some chipsets. > > Now, INPUT_SEL = 0 means compute CRC *only* over timings and not the > active area (and not display + timings like before) and like mentioned > before this has nothing to do with what is the input to the CRC. Not > covering the active area will not change the CRC at all as Rob reported > but its not specific to CTM. > > Which means we should have been setting INPUT_SEL=1 whenever we use INTF > CRC irrespective of whether CTM is used or not. > > What this also means is INTF CRC was not working correctly at all so far > irrespecive of CTM or not because it was always computing CRC only on > the timings (non-active area). > > This was not caught so far because it looks like IGT's > kms_pipe_crc_basic test which was used to validate this only compares > CRC between two frames of the same content to match if they were equal > and not changing contents and comparing like kms_plane does. It will > pass as CRC would not have changed. > > Now coming to the fix, the reset value of this register INTF_MISR_CTRL > already sets the INPUT_SEL bit (or unsets it) correctly based on > whichever DPU version is used so we should just change the > dpu_hw_setup_misr() to a read on the register followed by ORing the > required bits without touching INPUT_SEL and write. > > That will address this issue and also cover version control since the > expected value of this bit has changed across DPU revisions. Ok, thanks for following up on this. Mind posting a patch to supersede this one? BR, -R > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +- > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 4 ++-- > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 3 ++- > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 ++-- > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 2 +- > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 2 +- > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 5 ++++- > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 3 ++- > >>>>> 8 files changed, 15 insertions(+), 10 deletions(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > >>>>> index 2b83a13b3aa9..d93a92ffd5df 100644 > >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > >>>>> @@ -134,7 +134,7 @@ static void dpu_crtc_setup_encoder_misr(struct > >>>>> drm_crtc *crtc) > >>>>> struct drm_encoder *drm_enc; > >>>>> > >>>>> drm_for_each_encoder_mask(drm_enc, crtc->dev, > >>>>> crtc->state->encoder_mask) > >>>>> - dpu_encoder_setup_misr(drm_enc); > >>>>> + dpu_encoder_setup_misr(drm_enc, !!crtc->state->ctm); > >>>>> } > >>>>> > >>>>> static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const > >>>>> char *src_name) > >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >>>>> index b0a7908418ed..12ee7acb5ea6 100644 > >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >>>>> @@ -241,7 +241,7 @@ int dpu_encoder_get_crc_values_cnt(const struct > >>>>> drm_encoder *drm_enc) > >>>>> return num_intf; > >>>>> } > >>>>> > >>>>> -void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc) > >>>>> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc, > >>>>> bool has_ctm) > >>>>> { > >>>>> struct dpu_encoder_virt *dpu_enc; > >>>>> > >>>>> @@ -255,7 +255,7 @@ void dpu_encoder_setup_misr(const struct > >>>>> drm_encoder *drm_enc) > >>>>> if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr) > >>>>> continue; > >>>>> > >>>>> - phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1); > >>>>> + phys->hw_intf->ops.setup_misr(phys->hw_intf, true, > >>>>> 1, has_ctm); > >>>>> } > >>>>> } > >>>>> > >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >>>>> index 4c05fd5e9ed1..510783b2fb24 100644 > >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >>>>> @@ -169,8 +169,9 @@ int dpu_encoder_get_crc_values_cnt(const struct > >>>>> drm_encoder *drm_enc); > >>>>> /** > >>>>> * dpu_encoder_setup_misr - enable misr calculations > >>>>> * @drm_enc: Pointer to previously created drm encoder structure > >>>>> + * @has_ctm: Is CTM enabled > >>>>> */ > >>>>> -void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder); > >>>>> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder, > >>>>> bool has_ctm); > >>>>> > >>>>> /** > >>>>> * dpu_encoder_get_crc - get the crc value from interface blocks > >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > >>>>> index e8b8908d3e12..cb06f80cc671 100644 > >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > >>>>> @@ -318,9 +318,9 @@ static u32 dpu_hw_intf_get_line_count(struct > >>>>> dpu_hw_intf *intf) > >>>>> return DPU_REG_READ(c, INTF_LINE_COUNT); > >>>>> } > >>>>> > >>>>> -static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool > >>>>> enable, u32 frame_count) > >>>>> +static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool > >>>>> enable, u32 frame_count, bool has_ctm) > >>>>> { > >>>>> - dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, enable, > >>>>> frame_count); > >>>>> + dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, enable, > >>>>> frame_count, has_ctm); > >>>> > >>>> I'm not sure about the dpu_encoder and dpu_hw_intf interfaces. But > >>>> dpu_hw_setup_misr definitely needs the `u8 input_sel` parameter > >>>> instead of `bool has_ctm`. > >>> > >>> That seems a bit premature without knowing what the other values are. > >>> (And I also question a bit the whole abstraction layer thing if it is > >>> taking directly register bitfield enum's..) > >> > >> dpu_hw_intf and especially dpu_hw_util are not real abstractions. I > >> always viewed them as useful low-level helpers. > >> > >> I think that has_ctm is valid at the dpu_encoder level, which selects > >> which input to use. on the lower levels has_ctm doesn't make sense. > >> IOW dpu_hw_setup_misr can be used to setup MISR for other blocks, > >> where CTM doesn't exist. > >> > >>> > >>> BR, > >>> -R > >>> > >>>> Most likely, I'd use u8 for dpu_hw_intf operation too. > >>>> > >>>> Could you please adjust? > >>>> > >>>>> } > >>>>> > >>>>> static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 > >>>>> *misr_value) > >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h > >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h > >>>>> index c539025c418b..95aafc4cf58e 100644 > >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h > >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h > >>>>> @@ -95,7 +95,7 @@ struct dpu_hw_intf_ops { > >>>>> > >>>>> void (*bind_pingpong_blk)(struct dpu_hw_intf *intf, > >>>>> const enum dpu_pingpong pp); > >>>>> - void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, > >>>>> u32 frame_count); > >>>>> + void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, > >>>>> u32 frame_count, bool has_ctm); > >>>>> int (*collect_misr)(struct dpu_hw_intf *intf, u32 > >>>>> *misr_value); > >>>>> > >>>>> // Tearcheck on INTF since DPU 5.0.0 > >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c > >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c > >>>>> index d1c3bd8379ea..2efe29396c6a 100644 > >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c > >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c > >>>>> @@ -83,7 +83,7 @@ static void dpu_hw_lm_setup_border_color(struct > >>>>> dpu_hw_mixer *ctx, > >>>>> > >>>>> static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx, bool > >>>>> enable, u32 frame_count) > >>>>> { > >>>>> - dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, enable, > >>>>> frame_count); > >>>>> + dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, enable, > >>>>> frame_count, false); > >>>>> } > >>>>> > >>>>> static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32 > >>>>> *misr_value) > >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c > >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c > >>>>> index 9d2273fd2fed..528b8439209f 100644 > >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c > >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c > >>>>> @@ -483,7 +483,7 @@ void _dpu_hw_setup_qos_lut(struct > >>>>> dpu_hw_blk_reg_map *c, u32 offset, > >>>>> > >>>>> void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, > >>>>> u32 misr_ctrl_offset, > >>>>> - bool enable, u32 frame_count) > >>>>> + bool enable, u32 frame_count, bool has_ctm) > >>>>> { > >>>>> u32 config = 0; > >>>>> > >>>>> @@ -496,6 +496,9 @@ void dpu_hw_setup_misr(struct > >>>>> dpu_hw_blk_reg_map *c, > >>>>> config = (frame_count & MISR_FRAME_COUNT_MASK) | > >>>>> MISR_CTRL_ENABLE | MISR_CTRL_FREE_RUN_MASK; > >>>>> > >>>>> + if (!has_ctm) > >>>>> + config |= 1 << 24; > >>>> > >>>> Please define MISR_CTRL_INPUT_SEL instead. > >>>> > >>>>> + > >>>>> DPU_REG_WRITE(c, misr_ctrl_offset, config); > >>>>> } else { > >>>>> DPU_REG_WRITE(c, misr_ctrl_offset, 0); > >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h > >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h > >>>>> index 1f6079f47071..e42d9d00e40e 100644 > >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h > >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h > >>>>> @@ -360,7 +360,8 @@ void _dpu_hw_setup_qos_lut(struct > >>>>> dpu_hw_blk_reg_map *c, u32 offset, > >>>>> void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, > >>>>> u32 misr_ctrl_offset, > >>>>> bool enable, > >>>>> - u32 frame_count); > >>>>> + u32 frame_count, > >>>>> + bool has_ctm); > >>>>> > >>>>> int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c, > >>>>> u32 misr_ctrl_offset, > >>>>> -- > >>>>> 2.41.0 > >>>>> > >>>> > >>>> > >>>> -- > >>>> With best wishes > >>>> Dmitry > >> > >> > >>
On 11/21/2023 4:27 PM, Rob Clark wrote: > On Tue, Nov 21, 2023 at 4:41 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 10/24/2023 12:01 PM, Abhinav Kumar wrote: >>> >>> >>> On 10/23/2023 4:03 PM, Dmitry Baryshkov wrote: >>>> On Tue, 24 Oct 2023 at 01:36, Rob Clark <robdclark@gmail.com> wrote: >>>>> >>>>> On Mon, Oct 23, 2023 at 3:30 PM Dmitry Baryshkov >>>>> <dmitry.baryshkov@linaro.org> wrote: >>>>>> >>>>>> On Tue, 24 Oct 2023 at 01:12, Rob Clark <robdclark@gmail.com> wrote: >>>>>>> >>>>>>> From: Rob Clark <robdclark@chromium.org> >>>>>>> >>>>>>> Seems like we need to pick INPUT_SEL=1 when CTM is enabled. But not >>>>>>> otherwise. >>>>>>> >>>>>>> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>>>> Signed-off-by: Rob Clark <robdclark@chromium.org> >>>>>>> --- >>> >>> I cannot find anything in the docs which suggest this solution is correct. >>> >>> Different blocks in the DPU pipeline have their own CRC (MISR) registers >>> like LM, intf etc. >>> >>> We dont need to change INPUT_SEL to tell DPU from which pipeline to take >>> the CRC from as each of them have their own registers. >>> >>> INPUT_SEL is controlling whether the CRC needs to be calculated over the >>> entire display timings or only the active pixels. I am unable to tell at >>> the moment why this is making a difference in this use-case. >>> >>> Since I am unable to find any documentation proving this solution is >>> correct so far, unfortunately I would hold this back till then. >>> >>> We will investigate this issue and report our findings on this thread on >>> how to proceed. >>> >> >> Alright, we debugged and also found some more answers. >> >> The correct solution is indeed to set INPUT_SEL = 1 but let me explain >> why and what should be the correct way. >> >> INPUT_SEL was indeed telling whether to compute CRC over active pixels >> or active pixels + timings like I wrote before but this behavior changed >> since some chipsets. >> >> Now, INPUT_SEL = 0 means compute CRC *only* over timings and not the >> active area (and not display + timings like before) and like mentioned >> before this has nothing to do with what is the input to the CRC. Not >> covering the active area will not change the CRC at all as Rob reported >> but its not specific to CTM. >> >> Which means we should have been setting INPUT_SEL=1 whenever we use INTF >> CRC irrespective of whether CTM is used or not. >> >> What this also means is INTF CRC was not working correctly at all so far >> irrespecive of CTM or not because it was always computing CRC only on >> the timings (non-active area). >> >> This was not caught so far because it looks like IGT's >> kms_pipe_crc_basic test which was used to validate this only compares >> CRC between two frames of the same content to match if they were equal >> and not changing contents and comparing like kms_plane does. It will >> pass as CRC would not have changed. >> >> Now coming to the fix, the reset value of this register INTF_MISR_CTRL >> already sets the INPUT_SEL bit (or unsets it) correctly based on >> whichever DPU version is used so we should just change the >> dpu_hw_setup_misr() to a read on the register followed by ORing the >> required bits without touching INPUT_SEL and write. >> >> That will address this issue and also cover version control since the >> expected value of this bit has changed across DPU revisions. > > Ok, thanks for following up on this. Mind posting a patch to > supersede this one? > > BR, > -R > Yup, we will. Thanks Abhinav >>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +- >>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 4 ++-- >>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 3 ++- >>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 ++-- >>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 2 +- >>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 2 +- >>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 5 ++++- >>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 3 ++- >>>>>>> 8 files changed, 15 insertions(+), 10 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>>>>> index 2b83a13b3aa9..d93a92ffd5df 100644 >>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>>>>> @@ -134,7 +134,7 @@ static void dpu_crtc_setup_encoder_misr(struct >>>>>>> drm_crtc *crtc) >>>>>>> struct drm_encoder *drm_enc; >>>>>>> >>>>>>> drm_for_each_encoder_mask(drm_enc, crtc->dev, >>>>>>> crtc->state->encoder_mask) >>>>>>> - dpu_encoder_setup_misr(drm_enc); >>>>>>> + dpu_encoder_setup_misr(drm_enc, !!crtc->state->ctm); >>>>>>> } >>>>>>> >>>>>>> static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const >>>>>>> char *src_name) >>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>>>>> index b0a7908418ed..12ee7acb5ea6 100644 >>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>>>>> @@ -241,7 +241,7 @@ int dpu_encoder_get_crc_values_cnt(const struct >>>>>>> drm_encoder *drm_enc) >>>>>>> return num_intf; >>>>>>> } >>>>>>> >>>>>>> -void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc) >>>>>>> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc, >>>>>>> bool has_ctm) >>>>>>> { >>>>>>> struct dpu_encoder_virt *dpu_enc; >>>>>>> >>>>>>> @@ -255,7 +255,7 @@ void dpu_encoder_setup_misr(const struct >>>>>>> drm_encoder *drm_enc) >>>>>>> if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr) >>>>>>> continue; >>>>>>> >>>>>>> - phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1); >>>>>>> + phys->hw_intf->ops.setup_misr(phys->hw_intf, true, >>>>>>> 1, has_ctm); >>>>>>> } >>>>>>> } >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>>>> index 4c05fd5e9ed1..510783b2fb24 100644 >>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>>>> @@ -169,8 +169,9 @@ int dpu_encoder_get_crc_values_cnt(const struct >>>>>>> drm_encoder *drm_enc); >>>>>>> /** >>>>>>> * dpu_encoder_setup_misr - enable misr calculations >>>>>>> * @drm_enc: Pointer to previously created drm encoder structure >>>>>>> + * @has_ctm: Is CTM enabled >>>>>>> */ >>>>>>> -void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder); >>>>>>> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder, >>>>>>> bool has_ctm); >>>>>>> >>>>>>> /** >>>>>>> * dpu_encoder_get_crc - get the crc value from interface blocks >>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>>>>> index e8b8908d3e12..cb06f80cc671 100644 >>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>>>>> @@ -318,9 +318,9 @@ static u32 dpu_hw_intf_get_line_count(struct >>>>>>> dpu_hw_intf *intf) >>>>>>> return DPU_REG_READ(c, INTF_LINE_COUNT); >>>>>>> } >>>>>>> >>>>>>> -static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool >>>>>>> enable, u32 frame_count) >>>>>>> +static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool >>>>>>> enable, u32 frame_count, bool has_ctm) >>>>>>> { >>>>>>> - dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, enable, >>>>>>> frame_count); >>>>>>> + dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, enable, >>>>>>> frame_count, has_ctm); >>>>>> >>>>>> I'm not sure about the dpu_encoder and dpu_hw_intf interfaces. But >>>>>> dpu_hw_setup_misr definitely needs the `u8 input_sel` parameter >>>>>> instead of `bool has_ctm`. >>>>> >>>>> That seems a bit premature without knowing what the other values are. >>>>> (And I also question a bit the whole abstraction layer thing if it is >>>>> taking directly register bitfield enum's..) >>>> >>>> dpu_hw_intf and especially dpu_hw_util are not real abstractions. I >>>> always viewed them as useful low-level helpers. >>>> >>>> I think that has_ctm is valid at the dpu_encoder level, which selects >>>> which input to use. on the lower levels has_ctm doesn't make sense. >>>> IOW dpu_hw_setup_misr can be used to setup MISR for other blocks, >>>> where CTM doesn't exist. >>>> >>>>> >>>>> BR, >>>>> -R >>>>> >>>>>> Most likely, I'd use u8 for dpu_hw_intf operation too. >>>>>> >>>>>> Could you please adjust? >>>>>> >>>>>>> } >>>>>>> >>>>>>> static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 >>>>>>> *misr_value) >>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h >>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h >>>>>>> index c539025c418b..95aafc4cf58e 100644 >>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h >>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h >>>>>>> @@ -95,7 +95,7 @@ struct dpu_hw_intf_ops { >>>>>>> >>>>>>> void (*bind_pingpong_blk)(struct dpu_hw_intf *intf, >>>>>>> const enum dpu_pingpong pp); >>>>>>> - void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, >>>>>>> u32 frame_count); >>>>>>> + void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, >>>>>>> u32 frame_count, bool has_ctm); >>>>>>> int (*collect_misr)(struct dpu_hw_intf *intf, u32 >>>>>>> *misr_value); >>>>>>> >>>>>>> // Tearcheck on INTF since DPU 5.0.0 >>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >>>>>>> index d1c3bd8379ea..2efe29396c6a 100644 >>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c >>>>>>> @@ -83,7 +83,7 @@ static void dpu_hw_lm_setup_border_color(struct >>>>>>> dpu_hw_mixer *ctx, >>>>>>> >>>>>>> static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx, bool >>>>>>> enable, u32 frame_count) >>>>>>> { >>>>>>> - dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, enable, >>>>>>> frame_count); >>>>>>> + dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, enable, >>>>>>> frame_count, false); >>>>>>> } >>>>>>> >>>>>>> static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32 >>>>>>> *misr_value) >>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >>>>>>> index 9d2273fd2fed..528b8439209f 100644 >>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c >>>>>>> @@ -483,7 +483,7 @@ void _dpu_hw_setup_qos_lut(struct >>>>>>> dpu_hw_blk_reg_map *c, u32 offset, >>>>>>> >>>>>>> void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, >>>>>>> u32 misr_ctrl_offset, >>>>>>> - bool enable, u32 frame_count) >>>>>>> + bool enable, u32 frame_count, bool has_ctm) >>>>>>> { >>>>>>> u32 config = 0; >>>>>>> >>>>>>> @@ -496,6 +496,9 @@ void dpu_hw_setup_misr(struct >>>>>>> dpu_hw_blk_reg_map *c, >>>>>>> config = (frame_count & MISR_FRAME_COUNT_MASK) | >>>>>>> MISR_CTRL_ENABLE | MISR_CTRL_FREE_RUN_MASK; >>>>>>> >>>>>>> + if (!has_ctm) >>>>>>> + config |= 1 << 24; >>>>>> >>>>>> Please define MISR_CTRL_INPUT_SEL instead. >>>>>> >>>>>>> + >>>>>>> DPU_REG_WRITE(c, misr_ctrl_offset, config); >>>>>>> } else { >>>>>>> DPU_REG_WRITE(c, misr_ctrl_offset, 0); >>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >>>>>>> index 1f6079f47071..e42d9d00e40e 100644 >>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h >>>>>>> @@ -360,7 +360,8 @@ void _dpu_hw_setup_qos_lut(struct >>>>>>> dpu_hw_blk_reg_map *c, u32 offset, >>>>>>> void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, >>>>>>> u32 misr_ctrl_offset, >>>>>>> bool enable, >>>>>>> - u32 frame_count); >>>>>>> + u32 frame_count, >>>>>>> + bool has_ctm); >>>>>>> >>>>>>> int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c, >>>>>>> u32 misr_ctrl_offset, >>>>>>> -- >>>>>>> 2.41.0 >>>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> With best wishes >>>>>> Dmitry >>>> >>>> >>>>
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 2b83a13b3aa9..d93a92ffd5df 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -134,7 +134,7 @@ static void dpu_crtc_setup_encoder_misr(struct drm_crtc *crtc) struct drm_encoder *drm_enc; drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask) - dpu_encoder_setup_misr(drm_enc); + dpu_encoder_setup_misr(drm_enc, !!crtc->state->ctm); } static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index b0a7908418ed..12ee7acb5ea6 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -241,7 +241,7 @@ int dpu_encoder_get_crc_values_cnt(const struct drm_encoder *drm_enc) return num_intf; } -void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc) +void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc, bool has_ctm) { struct dpu_encoder_virt *dpu_enc; @@ -255,7 +255,7 @@ void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc) if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr) continue; - phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1); + phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1, has_ctm); } } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index 4c05fd5e9ed1..510783b2fb24 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -169,8 +169,9 @@ int dpu_encoder_get_crc_values_cnt(const struct drm_encoder *drm_enc); /** * dpu_encoder_setup_misr - enable misr calculations * @drm_enc: Pointer to previously created drm encoder structure + * @has_ctm: Is CTM enabled */ -void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder); +void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder, bool has_ctm); /** * dpu_encoder_get_crc - get the crc value from interface blocks diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c index e8b8908d3e12..cb06f80cc671 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c @@ -318,9 +318,9 @@ static u32 dpu_hw_intf_get_line_count(struct dpu_hw_intf *intf) return DPU_REG_READ(c, INTF_LINE_COUNT); } -static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool enable, u32 frame_count) +static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool enable, u32 frame_count, bool has_ctm) { - dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, enable, frame_count); + dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, enable, frame_count, has_ctm); } static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 *misr_value) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h index c539025c418b..95aafc4cf58e 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h @@ -95,7 +95,7 @@ struct dpu_hw_intf_ops { void (*bind_pingpong_blk)(struct dpu_hw_intf *intf, const enum dpu_pingpong pp); - void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, u32 frame_count); + void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, u32 frame_count, bool has_ctm); int (*collect_misr)(struct dpu_hw_intf *intf, u32 *misr_value); // Tearcheck on INTF since DPU 5.0.0 diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c index d1c3bd8379ea..2efe29396c6a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c @@ -83,7 +83,7 @@ static void dpu_hw_lm_setup_border_color(struct dpu_hw_mixer *ctx, static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx, bool enable, u32 frame_count) { - dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, enable, frame_count); + dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, enable, frame_count, false); } static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32 *misr_value) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c index 9d2273fd2fed..528b8439209f 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c @@ -483,7 +483,7 @@ void _dpu_hw_setup_qos_lut(struct dpu_hw_blk_reg_map *c, u32 offset, void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 misr_ctrl_offset, - bool enable, u32 frame_count) + bool enable, u32 frame_count, bool has_ctm) { u32 config = 0; @@ -496,6 +496,9 @@ void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, config = (frame_count & MISR_FRAME_COUNT_MASK) | MISR_CTRL_ENABLE | MISR_CTRL_FREE_RUN_MASK; + if (!has_ctm) + config |= 1 << 24; + DPU_REG_WRITE(c, misr_ctrl_offset, config); } else { DPU_REG_WRITE(c, misr_ctrl_offset, 0); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h index 1f6079f47071..e42d9d00e40e 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h @@ -360,7 +360,8 @@ void _dpu_hw_setup_qos_lut(struct dpu_hw_blk_reg_map *c, u32 offset, void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, u32 misr_ctrl_offset, bool enable, - u32 frame_count); + u32 frame_count, + bool has_ctm); int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c, u32 misr_ctrl_offset,