Re: [PATCH v2] drm/msm/dpu: always program dsc active bits
From: Abhinav Kumar
Date: Fri Apr 14 2023 - 15:09:41 EST
On 4/14/2023 11:55 AM, Abhinav Kumar wrote:
On 4/14/2023 10:28 AM, Marijn Suijten wrote:
On 2023-04-14 08:41:37, Abhinav Kumar wrote:
On 4/14/2023 12:48 AM, Marijn Suijten wrote:
Capitalize DSC in the title, as discussed in v1.
On 2023-04-13 08:56:41, Kuogee Hsieh wrote:
In current code, the DSC active bits are written only if cfg->dsc
is set.
However, for displays which are hot-pluggable, there can be a use-case
of disconnecting a DSC supported sink and connecting a non-DSC sink.
For those cases we need to clear DSC active bits during tear down.
Changes in V2:
1) correct commit text as suggested
2) correct Fixes commit id
3) add FIXME comment
Fixes: 77f6da90487c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl")
Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
Reviewed-by: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx>
By default git send-email should pick this up in the CC line... but I
had to download this patch from lore once again.
Yes, I think what happened here is, he didnt git am the prev rev and
make changes on top of that so git send-email didnt pick up. We should
fix that process.
The mail was sent so it must have gone through git send-email, unless a
different mail client was used to send the .patch file. I think you are
confusing this with git am (which doesn't need to be used if editing a
commit on a local branch) and subsequently git format-patch, which takes
a commit from a git repository and turns it into a .patch file: neither
of these "converts" r-b's (and other tags) to cc, that's happening in
git send-email (see `--suppress-cc` documentation in `man
git-send-email`).
Yes, ofcourse git send-email was used to send the patch, not any other
mail client.
Yes i am also aware that send-email converts rb to CC.
But if you keep working on the local branch, then you would have to
manually add the r-bs. If you use am of the prev version and develop on
that, it will automatically add the r-bs.
just a minor point, in case you didnt notice, my r-b was dropped too :)
due to manual propagation.
I can recommend b4: it has lots of useful features including
automatically picking up reviews and processing revisions. It even
requires a changelog to be edited ;). However, finding the right flags
and trusting it'll "do as ordered" is a bit daunting at first.
Ack.
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
index bbdc95c..1651cd7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
@@ -541,10 +541,10 @@ static void dpu_hw_ctl_intf_cfg_v1(struct
dpu_hw_ctl *ctx,
if (cfg->merge_3d)
DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE,
BIT(cfg->merge_3d - MERGE_3D_0));
- if (cfg->dsc) {
- DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
- DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
- }
+
+ /* FIXME: fix reset_intf_cfg to handle teardown of dsc */
There's more wrong than just moving (not "fix"ing) this bit of code
into
reset_intf_cfg. And this will have to be re-wrapped in `if (cfg->dsc)`
again by reverting this patch. Perhaps that can be explained, or link
to Abhinav's explanation to make it clear to readers what this FIXME
actually means? Let's wait for Abhinav and Dmitry to confirm the
desired communication here.
https://lore.kernel.org/linux-arm-msm/ec045d6b-4ffd-0f8c-4011-8db45edc6978@xxxxxxxxxxx/
Yes, I am fine with linking this explanation in the commit text and
mentioning that till thats fixed, we need to go with this solution. The
FIXME itself is fine, I will work on it and I remember this context
well.
Looks like it was removed entirely in v3, in favour of only describing
it in the patch body. The wording seems a bit off but that's fine by me
if you're picking this up soon anyway.
- Marijn