diff mbox series

[v6,6/7] drm/msm/dsi: update hdisplay calculation for dsi_timing_setup

Message ID 20230329-rfc-msm-dsc-helper-v6-6-cb7f59f0f7fb@quicinc.com (mailing list archive)
State Superseded
Headers show
Series Introduce MSM-specific DSC helpers | expand

Commit Message

Jessica Zhang April 12, 2023, 11:25 p.m. UTC 
hdisplay for compressed images should be calculated as bytes_per_slice *
slice_count. Thus, use MSM DSC helper to calculate hdisplay for
dsi_timing_setup instead of directly using mode->hdisplay.

Changes in v3:
- Split from previous patch
- Initialized hdisplay as uncompressed pclk per line at the beginning of
  dsi_timing_setup as to not break dual DSI calculations

Changes in v4:
- Moved pclk_per_intf calculations to DSC hdisplay adjustments

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Marijn Suijten May 4, 2023, 9:56 p.m. UTC | #1 
On 2023-04-12 16:25:20, Jessica Zhang wrote:
> hdisplay for compressed images should be calculated as bytes_per_slice *
> slice_count. Thus, use MSM DSC helper to calculate hdisplay for
> dsi_timing_setup instead of directly using mode->hdisplay.
> 
> Changes in v3:
> - Split from previous patch
> - Initialized hdisplay as uncompressed pclk per line at the beginning of
>   dsi_timing_setup as to not break dual DSI calculations
> 
> Changes in v4:
> - Moved pclk_per_intf calculations to DSC hdisplay adjustments
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  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 508577c596ff..ae966d4e349d 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 /= 3;
> +		hdisplay = msm_dsc_get_pclk_per_intf(msm_host->dsc) / 3;

This patch is unfortunately regressing the Sony Xperia XZ3 (sdm845,
single DSI), which will only show garbage when it is applied.

Are you sure this is correct, and the helper is returning the right
values?  I'll see if I can help review and validate those later, and
debug if necessary.

- Marijn

>  		h_total += hdisplay;
>  		ha_end = ha_start + hdisplay;
>  	}
> 
> -- 
> 2.40.0
>
Abhinav Kumar May 4, 2023, 10:05 p.m. UTC | #2 
On 5/4/2023 2:56 PM, Marijn Suijten wrote:
> On 2023-04-12 16:25:20, Jessica Zhang wrote:
>> hdisplay for compressed images should be calculated as bytes_per_slice *
>> slice_count. Thus, use MSM DSC helper to calculate hdisplay for
>> dsi_timing_setup instead of directly using mode->hdisplay.
>>
>> Changes in v3:
>> - Split from previous patch
>> - Initialized hdisplay as uncompressed pclk per line at the beginning of
>>    dsi_timing_setup as to not break dual DSI calculations
>>
>> Changes in v4:
>> - Moved pclk_per_intf calculations to DSC hdisplay adjustments
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   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 508577c596ff..ae966d4e349d 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 /= 3;
>> +		hdisplay = msm_dsc_get_pclk_per_intf(msm_host->dsc) / 3;
> 
> This patch is unfortunately regressing the Sony Xperia XZ3 (sdm845,
> single DSI), which will only show garbage when it is applied.
> 
> Are you sure this is correct, and the helper is returning the right
> values?  I'll see if I can help review and validate those later, and
> debug if necessary.
> 
> - Marijn

To help us debug these kind of issues, can you pls point us to your 
panel driver?

> 
>>   		h_total += hdisplay;
>>   		ha_end = ha_start + hdisplay;
>>   	}
>>
>> -- 
>> 2.40.0
>>
Jessica Zhang May 4, 2023, 10:34 p.m. UTC | #3 
On 5/4/2023 2:56 PM, Marijn Suijten wrote:
> On 2023-04-12 16:25:20, Jessica Zhang wrote:
>> hdisplay for compressed images should be calculated as bytes_per_slice *
>> slice_count. Thus, use MSM DSC helper to calculate hdisplay for
>> dsi_timing_setup instead of directly using mode->hdisplay.
>>
>> Changes in v3:
>> - Split from previous patch
>> - Initialized hdisplay as uncompressed pclk per line at the beginning of
>>    dsi_timing_setup as to not break dual DSI calculations
>>
>> Changes in v4:
>> - Moved pclk_per_intf calculations to DSC hdisplay adjustments
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   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 508577c596ff..ae966d4e349d 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 /= 3;
>> +		hdisplay = msm_dsc_get_pclk_per_intf(msm_host->dsc) / 3;
> 
> This patch is unfortunately regressing the Sony Xperia XZ3 (sdm845,
> single DSI), which will only show garbage when it is applied.
> 
> Are you sure this is correct, and the helper is returning the right
> values?  I'll see if I can help review and validate those later, and
> debug if necessary.

Hi Marijn,

Just checking, are you testing this with the DSI for DSC v1.2 changes? 
That series includes a fix to the word count calculation [1] needed to 
get DSC working.

Thanks,

Jessica Zhang

[1] https://patchwork.freedesktop.org/patch/535115/?series=117219&rev=1

> 
> - Marijn
> 
>>   		h_total += hdisplay;
>>   		ha_end = ha_start + hdisplay;
>>   	}
>>
>> -- 
>> 2.40.0
>>
Marijn Suijten May 7, 2023, 3:27 p.m. UTC | #4 
On 2023-05-04 15:05:15, Abhinav Kumar wrote:
> 
> 
> On 5/4/2023 2:56 PM, Marijn Suijten wrote:
> > On 2023-04-12 16:25:20, Jessica Zhang wrote:
> >> hdisplay for compressed images should be calculated as bytes_per_slice *
> >> slice_count. Thus, use MSM DSC helper to calculate hdisplay for
> >> dsi_timing_setup instead of directly using mode->hdisplay.
> >>
> >> Changes in v3:
> >> - Split from previous patch
> >> - Initialized hdisplay as uncompressed pclk per line at the beginning of
> >>    dsi_timing_setup as to not break dual DSI calculations
> >>
> >> Changes in v4:
> >> - Moved pclk_per_intf calculations to DSC hdisplay adjustments
> >>
> >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> >> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >> ---
> >>   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 508577c596ff..ae966d4e349d 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 /= 3;
> >> +		hdisplay = msm_dsc_get_pclk_per_intf(msm_host->dsc) / 3;
> > 
> > This patch is unfortunately regressing the Sony Xperia XZ3 (sdm845,
> > single DSI), which will only show garbage when it is applied.
> > 
> > Are you sure this is correct, and the helper is returning the right
> > values?  I'll see if I can help review and validate those later, and
> > debug if necessary.
> > 
> > - Marijn
> 
> To help us debug these kind of issues, can you pls point us to your 
> panel driver?

https://github.com/SoMainline/linux/commit/b154ea72e6c2ca0d4a33a28cc24e3a762dba4948

- Marijn
Marijn Suijten May 7, 2023, 3:30 p.m. UTC | #5 
On 2023-05-04 15:34:08, Jessica Zhang wrote:
> 
> 
> On 5/4/2023 2:56 PM, Marijn Suijten wrote:
> > On 2023-04-12 16:25:20, Jessica Zhang wrote:
> >> hdisplay for compressed images should be calculated as bytes_per_slice *
> >> slice_count. Thus, use MSM DSC helper to calculate hdisplay for
> >> dsi_timing_setup instead of directly using mode->hdisplay.
> >>
> >> Changes in v3:
> >> - Split from previous patch
> >> - Initialized hdisplay as uncompressed pclk per line at the beginning of
> >>    dsi_timing_setup as to not break dual DSI calculations
> >>
> >> Changes in v4:
> >> - Moved pclk_per_intf calculations to DSC hdisplay adjustments
> >>
> >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> >> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >> ---
> >>   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 508577c596ff..ae966d4e349d 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 /= 3;
> >> +		hdisplay = msm_dsc_get_pclk_per_intf(msm_host->dsc) / 3;
> > 
> > This patch is unfortunately regressing the Sony Xperia XZ3 (sdm845,
> > single DSI), which will only show garbage when it is applied.
> > 
> > Are you sure this is correct, and the helper is returning the right
> > values?  I'll see if I can help review and validate those later, and
> > debug if necessary.
> 
> Hi Marijn,
> 
> Just checking, are you testing this with the DSI for DSC v1.2 changes? 

Yes, all the series, including those that are implicitly/indirectly
required.  This specific patch is pointed out by git bisect.

> That series includes a fix to the word count calculation [1] needed to 
> get DSC working.

No, we cannot have this series introduce a bug and depend on *a future*
series to fix that, if that's what you're saying.

> Thanks,
> 
> Jessica Zhang
> 
> [1] https://patchwork.freedesktop.org/patch/535115/?series=117219&rev=1

That ""fix"" won't have any effect since slice_count is 1 for this
specific panel/device:

https://github.com/SoMainline/linux/commit/b154ea72e6c2ca0d4a33a28cc24e3a762dba4948

- Marijn

> 
> > 
> > - Marijn
> > 
> >>   		h_total += hdisplay;
> >>   		ha_end = ha_start + hdisplay;
> >>   	}
> >>
> >> -- 
> >> 2.40.0
> >>
Marijn Suijten May 7, 2023, 6:34 p.m. UTC | #6 
On 2023-05-07 17:27:33, Marijn Suijten wrote:
> On 2023-05-04 15:05:15, Abhinav Kumar wrote:
> > 
> > 
> > On 5/4/2023 2:56 PM, Marijn Suijten wrote:
> > > On 2023-04-12 16:25:20, Jessica Zhang wrote:
> > >> hdisplay for compressed images should be calculated as bytes_per_slice *
> > >> slice_count. Thus, use MSM DSC helper to calculate hdisplay for
> > >> dsi_timing_setup instead of directly using mode->hdisplay.
> > >>
> > >> Changes in v3:
> > >> - Split from previous patch
> > >> - Initialized hdisplay as uncompressed pclk per line at the beginning of
> > >>    dsi_timing_setup as to not break dual DSI calculations
> > >>
> > >> Changes in v4:
> > >> - Moved pclk_per_intf calculations to DSC hdisplay adjustments
> > >>
> > >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> > >> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > >> ---
> > >>   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 508577c596ff..ae966d4e349d 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 /= 3;
> > >> +		hdisplay = msm_dsc_get_pclk_per_intf(msm_host->dsc) / 3;
> > > 
> > > This patch is unfortunately regressing the Sony Xperia XZ3 (sdm845,
> > > single DSI), which will only show garbage when it is applied.
> > > 
> > > Are you sure this is correct, and the helper is returning the right
> > > values?  I'll see if I can help review and validate those later, and
> > > debug if necessary.
> > > 
> > > - Marijn
> > 
> > To help us debug these kind of issues, can you pls point us to your 
> > panel driver?
> 
> https://github.com/SoMainline/linux/commit/b154ea72e6c2ca0d4a33a28cc24e3a762dba4948

I found the fix myself after piecing together the hints provided across
the many different patch series.  This panel driver assigns
slice_count=1 based on downstream's qcom,mdss-dsc-slice-per-pkt = <1>,
but as per the many slice_count-related fixes the latter DT parameter is
a QCOM-specific hardware feature, whereas slice_count is simply the
number of slices per line.

Since a line is a scanline, and that panel has a width of hdisplay=1440
and a slice_width of 720, the number of slices spanning a line is simply
slice_count=hdisplay/slice_width=2.  This makes the panel work again
atop the four-or-so-series without a revert of this patch.

Is it a big ask to request a single, coherent series fixing all uses of
slice_count - and implementing support for slice-per-pkt - instead of
having the patches spread across multiple series?  That makes it much
easier to cover ground here and review this series, as slice_count seems
to be used everywhere where downstream used slice_per_pkt - even I
mistakenly used it after assuming it was the same based on the original
patches.

- Marijn
Marijn Suijten May 8, 2023, 9:46 p.m. UTC | #7 
On 2023-04-12 16:25:20, Jessica Zhang wrote:
> hdisplay for compressed images should be calculated as bytes_per_slice *
> slice_count. Thus, use MSM DSC helper to calculate hdisplay for
> dsi_timing_setup instead of directly using mode->hdisplay.

This doesn't really matter in the common case of of bpp=8, as the number
of horizontal pixels is equal to the number of horizontal slices times
the width of one horizontal slice.

> Changes in v3:
> - Split from previous patch
> - Initialized hdisplay as uncompressed pclk per line at the beginning of
>   dsi_timing_setup as to not break dual DSI calculations
> 
> Changes in v4:
> - Moved pclk_per_intf calculations to DSC hdisplay adjustments
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  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 508577c596ff..ae966d4e349d 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 /= 3;
> +		hdisplay = msm_dsc_get_pclk_per_intf(msm_host->dsc) / 3;

This function sounds like it returns bytes_per_line instead, not the
number of pixels in case bpp!=8.  Should we rename it?

- Marijn

>  		h_total += hdisplay;
>  		ha_end = ha_start + hdisplay;
>  	}
> 
> -- 
> 2.40.0
>
Jessica Zhang May 9, 2023, 12:39 a.m. UTC | #8 
On 5/8/2023 2:46 PM, Marijn Suijten wrote:
> On 2023-04-12 16:25:20, Jessica Zhang wrote:
>> hdisplay for compressed images should be calculated as bytes_per_slice *
>> slice_count. Thus, use MSM DSC helper to calculate hdisplay for
>> dsi_timing_setup instead of directly using mode->hdisplay.
> 
> This doesn't really matter in the common case of of bpp=8, as the number
> of horizontal pixels is equal to the number of horizontal slices times
> the width of one horizontal slice.
> 
>> Changes in v3:
>> - Split from previous patch
>> - Initialized hdisplay as uncompressed pclk per line at the beginning of
>>    dsi_timing_setup as to not break dual DSI calculations
>>
>> Changes in v4:
>> - Moved pclk_per_intf calculations to DSC hdisplay adjustments
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   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 508577c596ff..ae966d4e349d 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 /= 3;
>> +		hdisplay = msm_dsc_get_pclk_per_intf(msm_host->dsc) / 3;
> 
> This function sounds like it returns bytes_per_line instead, not the
> number of pixels in case bpp!=8.  Should we rename it?

Hi Marijn,

Sounds good.

Thanks,

Jessica Zhang

> 
> - Marijn
> 
>>   		h_total += hdisplay;
>>   		ha_end = ha_start + hdisplay;
>>   	}
>>
>> -- 
>> 2.40.0
>>
Jessica Zhang May 9, 2023, 12:51 a.m. UTC | #9 
On 5/7/2023 11:34 AM, Marijn Suijten wrote:
> On 2023-05-07 17:27:33, Marijn Suijten wrote:
>> On 2023-05-04 15:05:15, Abhinav Kumar wrote:
>>>
>>>
>>> On 5/4/2023 2:56 PM, Marijn Suijten wrote:
>>>> On 2023-04-12 16:25:20, Jessica Zhang wrote:
>>>>> hdisplay for compressed images should be calculated as bytes_per_slice *
>>>>> slice_count. Thus, use MSM DSC helper to calculate hdisplay for
>>>>> dsi_timing_setup instead of directly using mode->hdisplay.
>>>>>
>>>>> Changes in v3:
>>>>> - Split from previous patch
>>>>> - Initialized hdisplay as uncompressed pclk per line at the beginning of
>>>>>     dsi_timing_setup as to not break dual DSI calculations
>>>>>
>>>>> Changes in v4:
>>>>> - Moved pclk_per_intf calculations to DSC hdisplay adjustments
>>>>>
>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>>    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 508577c596ff..ae966d4e349d 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 /= 3;
>>>>> +		hdisplay = msm_dsc_get_pclk_per_intf(msm_host->dsc) / 3;
>>>>
>>>> This patch is unfortunately regressing the Sony Xperia XZ3 (sdm845,
>>>> single DSI), which will only show garbage when it is applied.
>>>>
>>>> Are you sure this is correct, and the helper is returning the right
>>>> values?  I'll see if I can help review and validate those later, and
>>>> debug if necessary.
>>>>
>>>> - Marijn
>>>
>>> To help us debug these kind of issues, can you pls point us to your
>>> panel driver?
>>
>> https://github.com/SoMainline/linux/commit/b154ea72e6c2ca0d4a33a28cc24e3a762dba4948
> 
> I found the fix myself after piecing together the hints provided across
> the many different patch series.  This panel driver assigns
> slice_count=1 based on downstream's qcom,mdss-dsc-slice-per-pkt = <1>,
> but as per the many slice_count-related fixes the latter DT parameter is
> a QCOM-specific hardware feature, whereas slice_count is simply the
> number of slices per line.
> 
> Since a line is a scanline, and that panel has a width of hdisplay=1440
> and a slice_width of 720, the number of slices spanning a line is simply
> slice_count=hdisplay/slice_width=2.  This makes the panel work again
> atop the four-or-so-series without a revert of this patch.
> 
> Is it a big ask to request a single, coherent series fixing all uses of
> slice_count - and implementing support for slice-per-pkt - instead of
> having the patches spread across multiple series?  That makes it much
> easier to cover ground here and review this series, as slice_count seems
> to be used everywhere where downstream used slice_per_pkt - even I
> mistakenly used it after assuming it was the same based on the original
> patches.

Hi Marijn,

Just want to document the changes we agreed on regarding the slice_count 
fixes:

I will move "drm/msm/dsi: Fix calculation for pkt_per_line" to the "Add 
DSC v1.2 Support for DSI" series so that all the 
slice_count/slice_per_pkt fixes are consolidated.

In addition I will also add a patch in "Add DSC v1.2 Support for DSI" to 
remove the incorrect `dsc->slice_count = 1` line in dsi_update_dsc_timing().

Thanks,

Jessica Zhang

> 
> - Marijn
Marijn Suijten May 9, 2023, 6:53 a.m. UTC | #10 
On 2023-05-08 17:51:08, Jessica Zhang wrote:
> 
> 
> On 5/7/2023 11:34 AM, Marijn Suijten wrote:
> > On 2023-05-07 17:27:33, Marijn Suijten wrote:
> >> On 2023-05-04 15:05:15, Abhinav Kumar wrote:
> >>>
> >>>
> >>> On 5/4/2023 2:56 PM, Marijn Suijten wrote:
> >>>> On 2023-04-12 16:25:20, Jessica Zhang wrote:
> >>>>> hdisplay for compressed images should be calculated as bytes_per_slice *
> >>>>> slice_count. Thus, use MSM DSC helper to calculate hdisplay for
> >>>>> dsi_timing_setup instead of directly using mode->hdisplay.
> >>>>>
> >>>>> Changes in v3:
> >>>>> - Split from previous patch
> >>>>> - Initialized hdisplay as uncompressed pclk per line at the beginning of
> >>>>>     dsi_timing_setup as to not break dual DSI calculations
> >>>>>
> >>>>> Changes in v4:
> >>>>> - Moved pclk_per_intf calculations to DSC hdisplay adjustments
> >>>>>
> >>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> >>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>>> ---
> >>>>>    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 508577c596ff..ae966d4e349d 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 /= 3;
> >>>>> +		hdisplay = msm_dsc_get_pclk_per_intf(msm_host->dsc) / 3;
> >>>>
> >>>> This patch is unfortunately regressing the Sony Xperia XZ3 (sdm845,
> >>>> single DSI), which will only show garbage when it is applied.
> >>>>
> >>>> Are you sure this is correct, and the helper is returning the right
> >>>> values?  I'll see if I can help review and validate those later, and
> >>>> debug if necessary.
> >>>>
> >>>> - Marijn
> >>>
> >>> To help us debug these kind of issues, can you pls point us to your
> >>> panel driver?
> >>
> >> https://github.com/SoMainline/linux/commit/b154ea72e6c2ca0d4a33a28cc24e3a762dba4948
> > 
> > I found the fix myself after piecing together the hints provided across
> > the many different patch series.  This panel driver assigns
> > slice_count=1 based on downstream's qcom,mdss-dsc-slice-per-pkt = <1>,
> > but as per the many slice_count-related fixes the latter DT parameter is
> > a QCOM-specific hardware feature, whereas slice_count is simply the
> > number of slices per line.
> > 
> > Since a line is a scanline, and that panel has a width of hdisplay=1440
> > and a slice_width of 720, the number of slices spanning a line is simply
> > slice_count=hdisplay/slice_width=2.  This makes the panel work again
> > atop the four-or-so-series without a revert of this patch.
> > 
> > Is it a big ask to request a single, coherent series fixing all uses of
> > slice_count - and implementing support for slice-per-pkt - instead of
> > having the patches spread across multiple series?  That makes it much
> > easier to cover ground here and review this series, as slice_count seems
> > to be used everywhere where downstream used slice_per_pkt - even I
> > mistakenly used it after assuming it was the same based on the original
> > patches.
> 
> Hi Marijn,
> 
> Just want to document the changes we agreed on regarding the slice_count 
> fixes:
> 
> I will move "drm/msm/dsi: Fix calculation for pkt_per_line" to the "Add 
> DSC v1.2 Support for DSI" series so that all the 
> slice_count/slice_per_pkt fixes are consolidated.
> 
> In addition I will also add a patch in "Add DSC v1.2 Support for DSI" to 
> remove the incorrect `dsc->slice_count = 1` line in dsi_update_dsc_timing().

That sounds good, but be sure to check two things:

- Can we squash some of the patches?
- Can we make the wording and title consistent?

That goes a long way in understanding that the patches in question are
addressing the same "wrong use of dsc->slice_count because it was
thought to be equal to slice_per_pkt" issue.

- Marijn
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 508577c596ff..ae966d4e349d 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 /= 3;
+		hdisplay = msm_dsc_get_pclk_per_intf(msm_host->dsc) / 3;
 		h_total += hdisplay;
 		ha_end = ha_start + hdisplay;
 	}