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

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

[Kuogee Hsieh]

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@quicinc.com>
Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 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 */
+	DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
+	DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
 }
 
 static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Marijn Suijten]

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@quicinc.com>
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

By default git send-email should pick this up in the CC line...  but I
had to download this patch from lore once again.

> ---
>  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@quicinc.com/

- Marijn

> +	DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
> +	DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
>  }
>  
>  static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

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

[Abhinav Kumar]

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@quicinc.com>
>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
> 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.

>> ---
>>   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@quicinc.com/
> 

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.

> - Marijn
> 
>> +	DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
>> +	DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
>>   }
>>   
>>   static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
>> -- 
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>

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

[Marijn Suijten]

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@quicinc.com>
> >> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> > 
> > 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`).

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@quicinc.com/
> > 
> 
> 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

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

[Abhinav Kumar]

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@quicinc.com>
>>>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>
>>> 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.


> 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@quicinc.com/
>>>
>>
>> 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

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

[Abhinav Kumar]

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@quicinc.com>
>>>>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>>
>>>> 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@quicinc.com/ 
>>>>
>>>>
>>>
>>> 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

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

[Dmitry Baryshkov]

On Fri, 14 Apr 2023 at 21:55, Abhinav Kumar <quic_abhinavk@quicinc.com> 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@quicinc.com>
> >>>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> >>>
> >>> 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.

>
>
> > 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@quicinc.com/
> >>>
> >>
> >> 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



-- 
With best wishes
Dmitry

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@quicinc.com> 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@quicinc.com>
>>>>>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>>>
>>>>> 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@quicinc.com/
>>>>>
>>>>
>>>> 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
> 
> 
> 

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

[Marijn Suijten]

On 2023-04-14 14:03:23, Abhinav Kumar wrote:
[..]
> >> 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.

I don't think git-am is smart enough to fetch additional replies from
lore and apply the reviewed-by (and other trailers).  This workflow
relies on downloading the mbox from a service that extracts and
accumulates the trailers such as patchwork, see for example:

	https://patchwork.freedesktop.org/api/1.0/series/116340/revisions/1/mbox/

Note that it also picked up one sentence starting with Fixes:

	Fixes: tag below seems extraneous.

On the other hand b4 (shaz)am is itself capable of downloading the whole
mail thread and appending all trailers, just like patchwork is doing
above, in one automated `b4 shazam <lore link>` command.

And to solve the problem of trailers arriving in your inbox after
working on the next revision in a local branch - or without ever even
redownloading the series from lore/patchwork¸ b4 has a command to
download and apply them to commits in your local branch:

	b4 trailers -uF <lore link>

Try it out, b4 is amazing :)

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

You are right.  The X-Mailer header of Kuogee's patch indicates git
2.7.4, while cc'ing of additional -by trailers (such as r-b's) was only
introduced in 2.20.0:

https://github.com/git/git/commit/ef0cc1df90f6b6c2987ab2db8e0ccf2cfc421edf

Fwiw 2.7.4 is from March 2016.

- Marijn

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

[Abhinav Kumar]

On 4/14/2023 3:52 PM, Marijn Suijten wrote:
> On 2023-04-14 14:03:23, Abhinav Kumar wrote:
> [..]
>>>> 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.
> 
> I don't think git-am is smart enough to fetch additional replies from
> lore and apply the reviewed-by (and other trailers).  This workflow
> relies on downloading the mbox from a service that extracts and
> accumulates the trailers such as patchwork, see for example:
> 
> 	https://patchwork.freedesktop.org/api/1.0/series/116340/revisions/1/mbox/
> 
> Note that it also picked up one sentence starting with Fixes:
> 
> 	Fixes: tag below seems extraneous.
> 

Yes, I usually use git-pw ap.

So perhaps, I should have been more specific to say that.

Some form of am is what i wanted to emphasize but just wrote git am by 
mistake.

> On the other hand b4 (shaz)am is itself capable of downloading the whole
> mail thread and appending all trailers, just like patchwork is doing
> above, in one automated `b4 shazam <lore link>` command.
> 
> And to solve the problem of trailers arriving in your inbox after
> working on the next revision in a local branch - or without ever even
> redownloading the series from lore/patchwork¸ b4 has a command to
> download and apply them to commits in your local branch:
> 
> 	b4 trailers -uF <lore link>
> 
> Try it out, b4 is amazing :)
> 

Yes, i recently started using it to send "applied" emails as suggested 
by Dmitry , for patch related work was still using git-pw.

Perhaps, I will switch completely to it.

> [..]
>> 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.
> 
> You are right.  The X-Mailer header of Kuogee's patch indicates git
> 2.7.4, while cc'ing of additional -by trailers (such as r-b's) was only
> introduced in 2.20.0:
> 
> https://github.com/git/git/commit/ef0cc1df90f6b6c2987ab2db8e0ccf2cfc421edf
> 
> Fwiw 2.7.4 is from March 2016.
> 
> - Marijn

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

[Dmitry Baryshkov]

On Sat, 15 Apr 2023 at 00:03, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 4/14/2023 1:58 PM, Dmitry Baryshkov wrote:
> > On Fri, 14 Apr 2023 at 21:55, Abhinav Kumar <quic_abhinavk@quicinc.com> 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@quicinc.com>
> >>>>>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> >>>>>
> >>>>> 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

Just as a note: there is nothing wrong with adding tags manually. I do
that for some of my patchsets (and sometimes I miss them too).

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

Ack. Thanks for updating it.

>
>
> >>
> >>
> >>> 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@quicinc.com/
> >>>>>
> >>>>
> >>>> 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
> >
> >
> >



-- 
With best wishes
Dmitry

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

[Abhinav Kumar]

On 4/14/2023 3:53 PM, Dmitry Baryshkov wrote:
> On Sat, 15 Apr 2023 at 00:03, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 4/14/2023 1:58 PM, Dmitry Baryshkov wrote:
>>> On Fri, 14 Apr 2023 at 21:55, Abhinav Kumar <quic_abhinavk@quicinc.com> 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@quicinc.com>
>>>>>>>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>>>>>
>>>>>>> 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
> 
> Just as a note: there is nothing wrong with adding tags manually. I do
> that for some of my patchsets (and sometimes I miss them too).
> 

Nothing wrong, especially when sometimes its given on IRC or something, 
I have to add it manually that time too.

But, just prone to forgetting. Like this case.

Next time, lets say Marijn again gives R-b, but someone else forgets to 
manually apply it, the same issue will happen even if proper git 
send-email is used.

>>
>> 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.
> 
> Ack. Thanks for updating it.
> 
>>
>>
>>>>
>>>>
>>>>> 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@quicinc.com/
>>>>>>>
>>>>>>
>>>>>> 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
>>>
>>>
>>>
> 
> 
>