diff mbox series

[v10,8/8] drm/msm/dsi: update hdisplay calculation for dsi_timing_setup

Message ID 20230329-rfc-msm-dsc-helper-v10-8-4cb21168c227@quicinc.com (mailing list archive)
State New, archived
Headers show
Series Introduce MSM-specific DSC helpers | expand

Commit Message

Jessica Zhang May 12, 2023, 9:32 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.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
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(-)

Comments

Marijn Suijten May 14, 2023, 9:29 p.m. UTC | #1 
On 2023-05-12 14:32:18, 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.

As mentioned in review on an earlier revision, is there any sort of
clarification you can provide here to explain the cases where
hdisplay!=bytes_per_line?  That goes a long way towards justifying this
change.  Thanks!

- Marijn

> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> 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 9eeda018774e..739f62643cc5 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_bytes_per_line(msm_host->dsc) / 3;
>  		h_total += hdisplay;
>  		ha_end = ha_start + hdisplay;
>  	}
> 
> -- 
> 2.40.1
>
Jessica Zhang May 16, 2023, 6:18 p.m. UTC | #2 
On 5/14/2023 2:29 PM, Marijn Suijten wrote:
> On 2023-05-12 14:32:18, 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.
> 
> As mentioned in review on an earlier revision, is there any sort of
> clarification you can provide here to explain the cases where
> hdisplay!=bytes_per_line?  That goes a long way towards justifying this
> change.  Thanks!

Hi Marijn,

Sorry for not responding to this in the earlier revision, I think I 
missed the original comment.

Please correct me if I'm wrong, but I'm guessing the question here is 
why we can't keep the hdisplay adjustment as `hdisplay /= 3` and have to 
go out of our way to recalculate hdisplay before doing the `/ 3`.

This is because the original adjustment only works for BPP = 8. By using 
the msm_dsc_get_bytes_per_line() here, we can generalize this adjustment 
to work for cases where BPP != 8.

Thanks,

Jessica Zhang


> 
> - Marijn
> 
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>> 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 9eeda018774e..739f62643cc5 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_bytes_per_line(msm_host->dsc) / 3;
>>   		h_total += hdisplay;
>>   		ha_end = ha_start + hdisplay;
>>   	}
>>
>> -- 
>> 2.40.1
>>
Marijn Suijten May 16, 2023, 10:45 p.m. UTC | #3 
On 2023-05-16 11:18:17, Jessica Zhang wrote:
> On 5/14/2023 2:29 PM, Marijn Suijten wrote:
> > On 2023-05-12 14:32:18, 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.
> > 
> > As mentioned in review on an earlier revision, is there any sort of
> > clarification you can provide here to explain the cases where
> > hdisplay!=bytes_per_line?  That goes a long way towards justifying this
> > change.  Thanks!
> 
> Hi Marijn,
> 
> Sorry for not responding to this in the earlier revision, I think I 
> missed the original comment.
> 
> Please correct me if I'm wrong, but I'm guessing the question here is 
> why we can't keep the hdisplay adjustment as `hdisplay /= 3` and have to 
> go out of our way to recalculate hdisplay before doing the `/ 3`.
> 
> This is because the original adjustment only works for BPP = 8. By using 
> the msm_dsc_get_bytes_per_line() here, we can generalize this adjustment 
> to work for cases where BPP != 8.

I am fully aware that the original computation only works for BPP=8 and
even mentioned so in v7 review [1].  The question / request is instead
to include such context in your commit message, rather than the
nondescriptive "should be calculated as" -> who says that and why?
Stating that the current approach was only working for BPP=8 (hence why
currently tested panels are working fine) but that this isn't a
long-term solution if we starts upporting other BPP is a proper
justification to make this change.

[1]: https://lore.kernel.org/linux-arm-msm/ju7647tlogo25fnhswgp7zn67syvsjy2ldjugvygh3z4rxtdrx@kb76evjvulgw/

> Thanks,

Thanks for looking into improving this!

- Marijn
Jessica Zhang May 16, 2023, 11:10 p.m. UTC | #4 
On 5/16/2023 3:45 PM, Marijn Suijten wrote:
> On 2023-05-16 11:18:17, Jessica Zhang wrote:
>> On 5/14/2023 2:29 PM, Marijn Suijten wrote:
>>> On 2023-05-12 14:32:18, 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.
>>>
>>> As mentioned in review on an earlier revision, is there any sort of
>>> clarification you can provide here to explain the cases where
>>> hdisplay!=bytes_per_line?  That goes a long way towards justifying this
>>> change.  Thanks!
>>
>> Hi Marijn,
>>
>> Sorry for not responding to this in the earlier revision, I think I
>> missed the original comment.
>>
>> Please correct me if I'm wrong, but I'm guessing the question here is
>> why we can't keep the hdisplay adjustment as `hdisplay /= 3` and have to
>> go out of our way to recalculate hdisplay before doing the `/ 3`.
>>
>> This is because the original adjustment only works for BPP = 8. By using
>> the msm_dsc_get_bytes_per_line() here, we can generalize this adjustment
>> to work for cases where BPP != 8.
> 
> I am fully aware that the original computation only works for BPP=8 and
> even mentioned so in v7 review [1].  The question / request is instead
> to include such context in your commit message, rather than the
> nondescriptive "should be calculated as" -> who says that and why?
> Stating that the current approach was only working for BPP=8 (hence why
> currently tested panels are working fine) but that this isn't a
> long-term solution if we starts upporting other BPP is a proper
> justification to make this change.

Sounds good, will add this to the commit message.

Thanks,

Jessica Zhang

> 
> [1]: https://lore.kernel.org/linux-arm-msm/ju7647tlogo25fnhswgp7zn67syvsjy2ldjugvygh3z4rxtdrx@kb76evjvulgw/
> 
>> Thanks,
> 
> Thanks for looking into improving this!
> 
> - 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 9eeda018774e..739f62643cc5 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_bytes_per_line(msm_host->dsc) / 3;
 		h_total += hdisplay;
 		ha_end = ha_start + hdisplay;
 	}