[v4,1/5] msm/drm/dsi: Round up DSC hdisplay calculation

  Currently, when compression is enabled, hdisplay is reduced via integer
division. This causes issues for modes where the original hdisplay is
not a multiple of 3.

To fix this, use DIV_ROUND_UP to divide hdisplay.

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
Suggested-by: Marijn Suijten <marijn.suijten@somainline.org>
Fixes: 08802f515c3cf ("drm/msm/dsi: Add support for DSC configuration")
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

On 2023-05-22 13:30:20, Jessica Zhang wrote:
> Currently, when compression is enabled, hdisplay is reduced via integer
> division. This causes issues for modes where the original hdisplay is
> not a multiple of 3.
> 
> To fix this, use DIV_ROUND_UP to divide hdisplay.
> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> Suggested-by: Marijn Suijten <marijn.suijten@somainline.org>

Nit: probably these should go in the opposite order.  And if they're
all supposed to be chronological, I think it is:

    Suggested-by:
    Fixes:
    Signed-off-by:
    Reviewed-by:

But unsure if that's a hard requirement, or even correct at all.

- Marijn

> Fixes: 08802f515c3cf ("drm/msm/dsi: Add support for DSC configuration")
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 9223d7ec5a73..18d38b90eb28 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -952,7 +952,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>  		 * pulse width same
>  		 */
>  		h_total -= hdisplay;
> -		hdisplay = msm_dsc_get_bytes_per_line(msm_host->dsc) / 3;
> +		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
>  		h_total += hdisplay;
>  		ha_end = ha_start + hdisplay;
>  	}
> 
> -- 
> 2.40.1
>
  

On 22.05.2023 22:44, Marijn Suijten wrote:
> On 2023-05-22 13:30:20, Jessica Zhang wrote:
>> Currently, when compression is enabled, hdisplay is reduced via integer
>> division. This causes issues for modes where the original hdisplay is
>> not a multiple of 3.
>>
>> To fix this, use DIV_ROUND_UP to divide hdisplay.
>>
>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>> Suggested-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
> Nit: probably these should go in the opposite order.  And if they're
> all supposed to be chronological, I think it is:
> 
>     Suggested-by:
>     Fixes:
>     Signed-off-by:
>     Reviewed-by:
> 
> But unsure if that's a hard requirement, or even correct at all.
> 
> - Marijn
Or you can rely on b4 to pick that up if it comes from others

Konrad
> 
>> Fixes: 08802f515c3cf ("drm/msm/dsi: Add support for DSC configuration")
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>  drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 9223d7ec5a73..18d38b90eb28 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -952,7 +952,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>  		 * pulse width same
>>  		 */
>>  		h_total -= hdisplay;
>> -		hdisplay = msm_dsc_get_bytes_per_line(msm_host->dsc) / 3;
>> +		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
>>  		h_total += hdisplay;
>>  		ha_end = ha_start + hdisplay;
>>  	}
>>
>> -- 
>> 2.40.1
>>
  

On 2023-05-22 22:52:40, Konrad Dybcio wrote:
> 
> 
> On 22.05.2023 22:44, Marijn Suijten wrote:
> > On 2023-05-22 13:30:20, Jessica Zhang wrote:
> >> Currently, when compression is enabled, hdisplay is reduced via integer
> >> division. This causes issues for modes where the original hdisplay is
> >> not a multiple of 3.
> >>
> >> To fix this, use DIV_ROUND_UP to divide hdisplay.
> >>
> >> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> >> Suggested-by: Marijn Suijten <marijn.suijten@somainline.org>
> > 
> > Nit: probably these should go in the opposite order.  And if they're
> > all supposed to be chronological, I think it is:
> > 
> >     Suggested-by:
> >     Fixes:
> >     Signed-off-by:
> >     Reviewed-by:
> > 
> > But unsure if that's a hard requirement, or even correct at all.
> > 
> > - Marijn
> Or you can rely on b4 to pick that up if it comes from others

The problem is that somewhat stupidly, b4 (trailers -u) puts them in the
wrong (not chronological) order, so it's pretty much useless for this.

Unless there's a required ordering specified somewhere in the docs, that
is *not* chronological, and that b4 is abiding by?  (that is my question
above)

- Marijn

> 
> Konrad
<snip>
  

On 5/22/2023 1:44 PM, Marijn Suijten wrote:
> On 2023-05-22 13:30:20, Jessica Zhang wrote:
>> Currently, when compression is enabled, hdisplay is reduced via integer
>> division. This causes issues for modes where the original hdisplay is
>> not a multiple of 3.
>>
>> To fix this, use DIV_ROUND_UP to divide hdisplay.
>>
>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>> Suggested-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
> Nit: probably these should go in the opposite order.  And if they're
> all supposed to be chronological, I think it is:
> 
>      Suggested-by:
>      Fixes:
>      Signed-off-by:
>      Reviewed-by:
> 
> But unsure if that's a hard requirement, or even correct at all.

Hi Marijn,

I don't see any explicit documentation on the order of R-b tags. FWIW, I 
see in the git log that S-o-b always goes at the bottom of the commit 
message.

I would prefer the S-o-b to always be at the bottom (as it helps me 
avoid duplicate S-o-b's when doing `git commit -s`), though I can flip 
the order of the R-b and suggested-by tags.

Thanks,

Jessica Zhang

> 
> - Marijn
> 
>> Fixes: 08802f515c3cf ("drm/msm/dsi: Add support for DSC configuration")
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 9223d7ec5a73..18d38b90eb28 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -952,7 +952,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>   		 * pulse width same
>>   		 */
>>   		h_total -= hdisplay;
>> -		hdisplay = msm_dsc_get_bytes_per_line(msm_host->dsc) / 3;
>> +		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
>>   		h_total += hdisplay;
>>   		ha_end = ha_start + hdisplay;
>>   	}
>>
>> -- 
>> 2.40.1
>>
  

On 5/22/2023 2:45 PM, Jessica Zhang wrote:
> 
> 
> On 5/22/2023 1:44 PM, Marijn Suijten wrote:
>> On 2023-05-22 13:30:20, Jessica Zhang wrote:
>>> Currently, when compression is enabled, hdisplay is reduced via integer
>>> division. This causes issues for modes where the original hdisplay is
>>> not a multiple of 3.
>>>
>>> To fix this, use DIV_ROUND_UP to divide hdisplay.
>>>
>>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>>> Suggested-by: Marijn Suijten <marijn.suijten@somainline.org>
>>
>> Nit: probably these should go in the opposite order.  And if they're
>> all supposed to be chronological, I think it is:
>>
>>      Suggested-by:
>>      Fixes:
>>      Signed-off-by:
>>      Reviewed-by:
>>
>> But unsure if that's a hard requirement, or even correct at all.
> 
> Hi Marijn,
> 
> I don't see any explicit documentation on the order of R-b tags. FWIW, I 
> see in the git log that S-o-b always goes at the bottom of the commit 
> message.
> 
> I would prefer the S-o-b to always be at the bottom (as it helps me 
> avoid duplicate S-o-b's when doing `git commit -s`), though I can flip 
> the order of the R-b and suggested-by tags.

Correction -- I can reorder the tags so that it's:

Suggested-by:
Fixes:
Reviewed-by:
Signed-off-by:

Thanks,

Jessica Zhang

> 
> Thanks,
> 
> Jessica Zhang
> 
>>
>> - Marijn
>>
>>> Fixes: 08802f515c3cf ("drm/msm/dsi: Add support for DSC configuration")
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> index 9223d7ec5a73..18d38b90eb28 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> @@ -952,7 +952,7 @@ static void dsi_timing_setup(struct msm_dsi_host 
>>> *msm_host, bool is_bonded_dsi)
>>>            * pulse width same
>>>            */
>>>           h_total -= hdisplay;
>>> -        hdisplay = msm_dsc_get_bytes_per_line(msm_host->dsc) / 3;
>>> +        hdisplay = 
>>> DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
>>>           h_total += hdisplay;
>>>           ha_end = ha_start + hdisplay;
>>>       }
>>>
>>> -- 
>>> 2.40.1
>>>
  

On Tue, 23 May 2023 at 00:45, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
>
>
> On 5/22/2023 1:44 PM, Marijn Suijten wrote:
> > On 2023-05-22 13:30:20, Jessica Zhang wrote:
> >> Currently, when compression is enabled, hdisplay is reduced via integer
> >> division. This causes issues for modes where the original hdisplay is
> >> not a multiple of 3.
> >>
> >> To fix this, use DIV_ROUND_UP to divide hdisplay.
> >>
> >> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> >> Suggested-by: Marijn Suijten <marijn.suijten@somainline.org>
> >
> > Nit: probably these should go in the opposite order.  And if they're
> > all supposed to be chronological, I think it is:
> >
> >      Suggested-by:
> >      Fixes:
> >      Signed-off-by:
> >      Reviewed-by:
> >
> > But unsure if that's a hard requirement, or even correct at all.
>
> Hi Marijn,
>
> I don't see any explicit documentation on the order of R-b tags. FWIW, I
> see in the git log that S-o-b always goes at the bottom of the commit
> message.
>
> I would prefer the S-o-b to always be at the bottom (as it helps me
> avoid duplicate S-o-b's when doing `git commit -s`), though I can flip
> the order of the R-b and suggested-by tags.

I'd second Jessica here. Consider these tags as a history or a transcript:

I would not vote on the particular order of the Suggested-by/Fixes
tags, I don't think that is important. These come first. Then the
patch goes through different cycles. of reviews, which gain
Reviewed-by tags.

In the same way Link/Patchwork/whatever other tags are added in the
historical order.

By having the submitter's S-o-b at the bottom, the submitter adds the
final signature under everything else being stated/recorded.

Of course, in a more complicated story, there might be other
developers taking part (Co-Developed-By + Signed-off-by), etc.

Note: all described is just my perception and might differ from the
BCP regarding the tags.

>
> Thanks,
>
> Jessica Zhang
>
> >
> > - Marijn
> >
> >> Fixes: 08802f515c3cf ("drm/msm/dsi: Add support for DSC configuration")
> >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> index 9223d7ec5a73..18d38b90eb28 100644
> >> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> @@ -952,7 +952,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> >>               * pulse width same
> >>               */
> >>              h_total -= hdisplay;
> >> -            hdisplay = msm_dsc_get_bytes_per_line(msm_host->dsc) / 3;
> >> +            hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
> >>              h_total += hdisplay;
> >>              ha_end = ha_start + hdisplay;
> >>      }
> >>
> >> --
> >> 2.40.1
> >>
  

On 2023-05-23 01:14:40, Dmitry Baryshkov wrote:
> On Tue, 23 May 2023 at 00:45, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> >
> >
> >
> > On 5/22/2023 1:44 PM, Marijn Suijten wrote:
> > > On 2023-05-22 13:30:20, Jessica Zhang wrote:
> > >> Currently, when compression is enabled, hdisplay is reduced via integer
> > >> division. This causes issues for modes where the original hdisplay is
> > >> not a multiple of 3.
> > >>
> > >> To fix this, use DIV_ROUND_UP to divide hdisplay.
> > >>
> > >> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> > >> Suggested-by: Marijn Suijten <marijn.suijten@somainline.org>
> > >
> > > Nit: probably these should go in the opposite order.  And if they're
> > > all supposed to be chronological, I think it is:
> > >
> > >      Suggested-by:
> > >      Fixes:
> > >      Signed-off-by:
> > >      Reviewed-by:
> > >
> > > But unsure if that's a hard requirement, or even correct at all.
> >
> > Hi Marijn,
> >
> > I don't see any explicit documentation on the order of R-b tags. FWIW, I
> > see in the git log that S-o-b always goes at the bottom of the commit
> > message.
> >
> > I would prefer the S-o-b to always be at the bottom (as it helps me
> > avoid duplicate S-o-b's when doing `git commit -s`), though I can flip
> > the order of the R-b and suggested-by tags.
> 
> I'd second Jessica here. Consider these tags as a history or a transcript:
> 
> I would not vote on the particular order of the Suggested-by/Fixes
> tags, I don't think that is important. These come first. Then the
> patch goes through different cycles. of reviews, which gain
> Reviewed-by tags.
> 
> In the same way Link/Patchwork/whatever other tags are added in the
> historical order.
> 
> By having the submitter's S-o-b at the bottom, the submitter adds the
> final signature under everything else being stated/recorded.

Correct, so the s-o-b can always be kept / moved back to the bottom on a
resend, stating that they sign off on "all that was written previously"
including picking up reviews.

However, for the rest of your reply about "history / transcript", you
seem to agree exactly with my point of keeping (or rather, simply
appending) these in chronological order?

- Marijn

> 
> Of course, in a more complicated story, there might be other
> developers taking part (Co-Developed-By + Signed-off-by), etc.
> 
> Note: all described is just my perception and might differ from the
> BCP regarding the tags.

<snip>
  

On 23/05/2023 01:18, Marijn Suijten wrote:
> On 2023-05-23 01:14:40, Dmitry Baryshkov wrote:
>> On Tue, 23 May 2023 at 00:45, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>>
>>>
>>>
>>> On 5/22/2023 1:44 PM, Marijn Suijten wrote:
>>>> On 2023-05-22 13:30:20, Jessica Zhang wrote:
>>>>> Currently, when compression is enabled, hdisplay is reduced via integer
>>>>> division. This causes issues for modes where the original hdisplay is
>>>>> not a multiple of 3.
>>>>>
>>>>> To fix this, use DIV_ROUND_UP to divide hdisplay.
>>>>>
>>>>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>>> Suggested-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>>
>>>> Nit: probably these should go in the opposite order.  And if they're
>>>> all supposed to be chronological, I think it is:
>>>>
>>>>       Suggested-by:
>>>>       Fixes:
>>>>       Signed-off-by:
>>>>       Reviewed-by:
>>>>
>>>> But unsure if that's a hard requirement, or even correct at all.
>>>
>>> Hi Marijn,
>>>
>>> I don't see any explicit documentation on the order of R-b tags. FWIW, I
>>> see in the git log that S-o-b always goes at the bottom of the commit
>>> message.
>>>
>>> I would prefer the S-o-b to always be at the bottom (as it helps me
>>> avoid duplicate S-o-b's when doing `git commit -s`), though I can flip
>>> the order of the R-b and suggested-by tags.
>>
>> I'd second Jessica here. Consider these tags as a history or a transcript:
>>
>> I would not vote on the particular order of the Suggested-by/Fixes
>> tags, I don't think that is important. These come first. Then the
>> patch goes through different cycles. of reviews, which gain
>> Reviewed-by tags.
>>
>> In the same way Link/Patchwork/whatever other tags are added in the
>> historical order.
>>
>> By having the submitter's S-o-b at the bottom, the submitter adds the
>> final signature under everything else being stated/recorded.
> 
> Correct, so the s-o-b can always be kept / moved back to the bottom on a
> resend, stating that they sign off on "all that was written previously"
> including picking up reviews.
> 
> However, for the rest of your reply about "history / transcript", you
> seem to agree exactly with my point of keeping (or rather, simply
> appending) these in chronological order?

Yes.

> 
> - Marijn
> 
>>
>> Of course, in a more complicated story, there might be other
>> developers taking part (Co-Developed-By + Signed-off-by), etc.
>>
>> Note: all described is just my perception and might differ from the
>> BCP regarding the tags.
> 
> <snip>