From: Abhinav Kumar <quic_abhinavk@quicinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>,
Marijn Suijten <marijn.suijten@somainline.org>
Cc: Stephen Boyd <swboyd@chromium.org>,
David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
Bjorn Andersson <andersson@kernel.org>,
<linux-arm-msm@vger.kernel.org>,
<dri-devel@lists.freedesktop.org>,
<freedreno@lists.freedesktop.org>
Subject: Re: [PATCH v4 8/9] drm/msm/dpu: merge DPU_SSPP_SCALER_QSEED3, QSEED3LITE, QSEED4
Date: Fri, 15 Sep 2023 16:01:48 -0700 [thread overview]
Message-ID: <a2c778b0-43c4-3485-e7c6-e40484e451dc@quicinc.com> (raw)
In-Reply-To: <20230911214521.787453-9-dmitry.baryshkov@linaro.org>
On 9/11/2023 2:45 PM, Dmitry Baryshkov wrote:
> Three different features, DPU_SSPP_SCALER_QSEED3, QSEED3LITE and QSEED4
> are all related to different versions of the same HW scaling block.
> Corresponding driver parts use scaler_blk.version to identify the
> correct way to program the hardware. In order to simplify the driver
> codepath, merge these three feature bits.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
I am okay with some parts of this change but not all.
Please see below.
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 4 ++--
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 6 +-----
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 9 ++-------
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 4 +---
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 3 +--
> 5 files changed, 7 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> index b37b4076e53a..67d66319a825 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -31,10 +31,10 @@
> (VIG_SDM845_MASK | BIT(DPU_SSPP_SMART_DMA_V2))
>
> #define VIG_SC7180_MASK \
> - (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | BIT(DPU_SSPP_SCALER_QSEED4))
> + (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | BIT(DPU_SSPP_SCALER_QSEED3))
>
> #define VIG_SM6125_MASK \
> - (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | BIT(DPU_SSPP_SCALER_QSEED3LITE))
> + (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | BIT(DPU_SSPP_SCALER_QSEED3))
>
This is like a half-n-half solution. This is telling that SC7180 and
SM6125 have a scaler blk version of 3.1 but are still qseed3. That gives
a misleading picture.
> #define VIG_SC7180_MASK_SDMA \
> (VIG_SC7180_MASK | BIT(DPU_SSPP_SMART_DMA_V2))
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> index fc5027b0123a..ba262b3f0bdc 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -51,9 +51,7 @@ enum {
> /**
> * SSPP sub-blocks/features
> * @DPU_SSPP_SCALER_QSEED2, QSEED2 algorithm support
> - * @DPU_SSPP_SCALER_QSEED3, QSEED3 alogorithm support
> - * @DPU_SSPP_SCALER_QSEED3LITE, QSEED3 Lite alogorithm support
> - * @DPU_SSPP_SCALER_QSEED4, QSEED4 algorithm support
> + * @DPU_SSPP_SCALER_QSEED3, QSEED3 alogorithm support (also QSEED3LITE and QSEED4)
> * @DPU_SSPP_SCALER_RGB, RGB Scaler, supported by RGB pipes
> * @DPU_SSPP_CSC, Support of Color space converion
> * @DPU_SSPP_CSC_10BIT, Support of 10-bit Color space conversion
> @@ -72,8 +70,6 @@ enum {
> enum {
> DPU_SSPP_SCALER_QSEED2 = 0x1,
> DPU_SSPP_SCALER_QSEED3,
> - DPU_SSPP_SCALER_QSEED3LITE,
> - DPU_SSPP_SCALER_QSEED4,
> DPU_SSPP_SCALER_RGB,
> DPU_SSPP_CSC,
> DPU_SSPP_CSC_10BIT,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> index 7e9c87088e17..d1b70cf72eef 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> @@ -594,9 +594,7 @@ static void _setup_layer_ops(struct dpu_hw_sspp *c,
> test_bit(DPU_SSPP_SMART_DMA_V2, &c->cap->features))
> c->ops.setup_multirect = dpu_hw_sspp_setup_multirect;
>
> - if (test_bit(DPU_SSPP_SCALER_QSEED3, &features) ||
> - test_bit(DPU_SSPP_SCALER_QSEED3LITE, &features) ||
> - test_bit(DPU_SSPP_SCALER_QSEED4, &features))
> + if (test_bit(DPU_SSPP_SCALER_QSEED3, &features))
> c->ops.setup_scaler = _dpu_hw_sspp_setup_scaler3;
>
any reason we cannot replace this with sblk->scaler_blk.version >= 1.2?
> if (test_bit(DPU_SSPP_CDP, &features))
> @@ -629,10 +627,7 @@ int _dpu_hw_sspp_init_debugfs(struct dpu_hw_sspp *hw_pipe, struct dpu_kms *kms,
> cfg->len,
> kms);
>
> - if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
> - cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
> - cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) ||
> - cfg->features & BIT(DPU_SSPP_SCALER_QSEED4))
> + if (sblk->scaler_blk.len)
> dpu_debugfs_create_regset32("scaler_blk", 0400,
> debugfs_root,
> sblk->scaler_blk.base + cfg->base,
This part LGTM.
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> index ca02f86c94ed..b157ed7da065 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> @@ -26,9 +26,7 @@ struct dpu_hw_sspp;
> */
> #define DPU_SSPP_SCALER (BIT(DPU_SSPP_SCALER_RGB) | \
> BIT(DPU_SSPP_SCALER_QSEED2) | \
> - BIT(DPU_SSPP_SCALER_QSEED3) | \
> - BIT(DPU_SSPP_SCALER_QSEED3LITE) | \
> - BIT(DPU_SSPP_SCALER_QSEED4))
> + BIT(DPU_SSPP_SCALER_QSEED3))
>
I am not seeing DPU_SSPP_SCALER_RGB being set by any chipset in the
catalog? So we can drop it in a separate change and then just use
sblk->scaler_blk.len in the place of this macro and drop this macro.
> /*
> * Define all CSC feature bits in catalog
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index c2aaaded07ed..109355275ec5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -438,8 +438,7 @@ static void _dpu_plane_setup_scaler3(struct dpu_hw_sspp *pipe_hw,
> scale_cfg->src_height[i] /= chroma_subsmpl_v;
> }
>
> - if (pipe_hw->cap->features &
> - BIT(DPU_SSPP_SCALER_QSEED4)) {
> + if (pipe_hw->cap->sblk->scaler_blk.version >= 0x3000) {
> scale_cfg->preload_x[i] = DPU_QSEED4_DEFAULT_PRELOAD_H;
> scale_cfg->preload_y[i] = DPU_QSEED4_DEFAULT_PRELOAD_V;
> } else {
This part LGTM.
next prev parent reply other threads:[~2023-09-15 23:02 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-11 21:45 [PATCH v4 0/9] drm/msm/dpu: simplify DPU sub-blocks info Dmitry Baryshkov
2023-09-11 21:45 ` [PATCH v4 1/9] drm/msm/dpu: populate SSPP scaler block version Dmitry Baryshkov
2023-09-11 21:45 ` [PATCH v4 2/9] drm/msm/dpu: Drop unused get_scaler_ver callback from SSPP Dmitry Baryshkov
2023-09-11 23:03 ` Abhinav Kumar
2023-09-11 21:45 ` [PATCH v4 3/9] drm/msm/dpu: Drop unused qseed_type from catalog dpu_caps Dmitry Baryshkov
2023-09-11 23:04 ` Abhinav Kumar
2023-09-11 21:45 ` [PATCH v4 4/9] drm/msm/dpu: drop the `id' field from DPU_HW_SUBBLK_INFO Dmitry Baryshkov
2023-09-11 23:17 ` Abhinav Kumar
2023-09-11 21:45 ` [PATCH v4 5/9] drm/msm/dpu: drop the `smart_dma_priority' field from struct dpu_sspp_sub_blks Dmitry Baryshkov
2023-09-11 23:26 ` Abhinav Kumar
2023-09-11 21:45 ` [PATCH v4 6/9] drm/msm/dpu: deduplicate some (most) of SSPP sub-blocks Dmitry Baryshkov
2023-09-15 21:49 ` Abhinav Kumar
2023-09-11 21:45 ` [PATCH v4 7/9] drm/msm/dpu: drop DPU_HW_SUBBLK_INFO macro Dmitry Baryshkov
2023-09-15 21:51 ` Abhinav Kumar
2023-09-11 21:45 ` [PATCH v4 8/9] drm/msm/dpu: merge DPU_SSPP_SCALER_QSEED3, QSEED3LITE, QSEED4 Dmitry Baryshkov
2023-09-15 23:01 ` Abhinav Kumar [this message]
2023-09-16 0:27 ` Dmitry Baryshkov
2023-09-11 21:45 ` [PATCH v4 9/9] drm/msm/gpu: drop duplicating VIG feature masks Dmitry Baryshkov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a2c778b0-43c4-3485-e7c6-e40484e451dc@quicinc.com \
--to=quic_abhinavk@quicinc.com \
--cc=airlied@gmail.com \
--cc=andersson@kernel.org \
--cc=daniel@ffwll.ch \
--cc=dmitry.baryshkov@linaro.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=freedreno@lists.freedesktop.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=marijn.suijten@somainline.org \
--cc=robdclark@gmail.com \
--cc=sean@poorly.run \
--cc=swboyd@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox