Re: [PATCH v2] drm/msm/dpu: always program dsc active bits

[Abhinav Kumar]

On 4/14/2023 1:58 PM, Dmitry Baryshkov wrote:
> On Fri, 14 Apr 2023 at 21:55, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> 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.
>
> It looks like there is some misunderstanding here. I think Marijn
> doesn't question his R-B (which was present), but tries to point out
> that Kuogee might want to adjust his git-send-email invocation. By
> default (and that's a good practice, which we should follow),
> git-send-email will CC people mentioned in such tags. Marijn didn't
> get this email. So, it seems, for some reason this Cc: _mail_ header
> was suppressed. Probably git-send-email invocation should be changed
> to prevent suppression of adding mentioned people to CC lists.

Yeah I understood that part. There were two issues here:

1) My r-b got dropped and that was because am wasn't used to 
automatically retain tags from prev version.

If you dont add the r-bs either manually or by am, then folks wont be 
part of CC either

2) I synced with kuogee. his git version seems to be quite old which is 
not adding the folks from r-b to cc. So there was nothing wrong with 
invocation, just versioning.


> > > 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.
> > >
> > > > > > ---
> > > > > >     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