diff mbox series

[v3,5/5] drm/msm/dsi: Remove incorrect references to slice_count

Message ID 20230405-add-dsc-support-v3-5-6e1d35a206b3@quicinc.com (mailing list archive)
State Superseded
Headers show
Series Add DSC v1.2 Support for DSI | expand

Commit Message

Jessica Zhang May 19, 2023, 9:17 p.m. UTC 
Currently, slice_count is being used to calculate word count and
pkt_per_line. In downstream, these values are calculated using slice per
packet, which is not the same as slice_count.

Slice count represents the number of soft slices per interface, and its
value will not always match that of slice per packet. For example, it is
possible to have cases where there are multiple soft slices per interface
but the panel specifies only one slice per packet.

Thus, use the default value of one slice per packet and remove slice_count
from the aforementioned calculations.

Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to compute word count")
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

Comments

Marijn Suijten May 19, 2023, 9:24 p.m. UTC | #1 
On 2023-05-19 14:17:30, Jessica Zhang wrote:
> Currently, slice_count is being used to calculate word count and
> pkt_per_line. In downstream, these values are calculated using slice per
> packet, which is not the same as slice_count.
> 
> Slice count represents the number of soft slices per interface, and its
> value will not always match that of slice per packet. For example, it is
> possible to have cases where there are multiple soft slices per interface
> but the panel specifies only one slice per packet.
> 
> Thus, use the default value of one slice per packet and remove slice_count
> from the aforementioned calculations.
> 
> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to compute word count")
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index d04f8bbd707d..8c8858ee59ec 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -866,18 +866,15 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>  	 */
>  	slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay);
>  
> -	/*
> -	 * If slice_count is greater than slice_per_intf
> -	 * then default to 1. This can happen during partial
> -	 * update.
> -	 */
> -	if (dsc->slice_count > slice_per_intf)
> -		dsc->slice_count = 1;
> -
>  	total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
>  
>  	eol_byte_num = total_bytes_per_intf % 3;
> -	pkt_per_line = slice_per_intf / dsc->slice_count;
> +
> +	/*
> +	 * Default to 1 slice_per_pkt, so pkt_per_line will be equal to
> +	 * slice per intf.
> +	 */
> +	pkt_per_line = slice_per_intf;

Same as the comment below, perhaps it is a good idea to clarify in the
comment here that the original value wa *multiplied by* slice_per_pkt?
Otherwise this default of 1 is "invisible".

Alternatively we could have a `const slice_per_pkt = 1;` at the top, and
have the comment (the elaborate one from below) there?

- Marijn

>  
>  	if (is_cmd_mode) /* packet data type */
>  		reg = DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
> @@ -1001,7 +998,14 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>  		if (!msm_host->dsc)
>  			wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
>  		else
> -			wc = msm_host->dsc->slice_chunk_size * msm_host->dsc->slice_count + 1;
> +			/*
> +			 * When DSC is enabled, WC = slice_chunk_size * slice_per_packet + 1.
> +			 * Currently, the driver only supports default value of slice_per_packet = 1
> +			 *
> +			 * TODO: Expand mipi_dsi_device struct to hold slice_per_packet info
> +			 *       and adjust DSC math to account for slice_per_packet.
> +			 */
> +			wc = msm_host->dsc->slice_chunk_size + 1;
>  
>  		dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>  			DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
> 
> -- 
> 2.40.1
>
Jessica Zhang May 19, 2023, 11:02 p.m. UTC | #2 
On 5/19/2023 2:24 PM, Marijn Suijten wrote:
> On 2023-05-19 14:17:30, Jessica Zhang wrote:
>> Currently, slice_count is being used to calculate word count and
>> pkt_per_line. In downstream, these values are calculated using slice per
>> packet, which is not the same as slice_count.
>>
>> Slice count represents the number of soft slices per interface, and its
>> value will not always match that of slice per packet. For example, it is
>> possible to have cases where there are multiple soft slices per interface
>> but the panel specifies only one slice per packet.
>>
>> Thus, use the default value of one slice per packet and remove slice_count
>> from the aforementioned calculations.
>>
>> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
>> Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to compute word count")
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 24 ++++++++++++++----------
>>   1 file changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index d04f8bbd707d..8c8858ee59ec 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -866,18 +866,15 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>>   	 */
>>   	slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay);
>>   
>> -	/*
>> -	 * If slice_count is greater than slice_per_intf
>> -	 * then default to 1. This can happen during partial
>> -	 * update.
>> -	 */
>> -	if (dsc->slice_count > slice_per_intf)
>> -		dsc->slice_count = 1;
>> -
>>   	total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
>>   
>>   	eol_byte_num = total_bytes_per_intf % 3;
>> -	pkt_per_line = slice_per_intf / dsc->slice_count;
>> +
>> +	/*
>> +	 * Default to 1 slice_per_pkt, so pkt_per_line will be equal to
>> +	 * slice per intf.
>> +	 */
>> +	pkt_per_line = slice_per_intf;
> 
> Same as the comment below, perhaps it is a good idea to clarify in the
> comment here that the original value wa *multiplied by* slice_per_pkt?
> Otherwise this default of 1 is "invisible".

Hi Marijn,

Acked.

> 
> Alternatively we could have a `const slice_per_pkt = 1;` at the top, and
> have the comment (the elaborate one from below) there?

Since the default slice_per_pkt = 1 and there's only 2 places where I 
have to add a clarifying comment, I think having a separate 
slice_per_pkt variable is a bit redundant.

Plus, if support for multiple slice_per_pkt values is added in the 
future, we'd also have to drop this variable anyways.

Thanks,

Jessica Zhang

> 
> - Marijn
> 
>>   
>>   	if (is_cmd_mode) /* packet data type */
>>   		reg = DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
>> @@ -1001,7 +998,14 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>   		if (!msm_host->dsc)
>>   			wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
>>   		else
>> -			wc = msm_host->dsc->slice_chunk_size * msm_host->dsc->slice_count + 1;
>> +			/*
>> +			 * When DSC is enabled, WC = slice_chunk_size * slice_per_packet + 1.
>> +			 * Currently, the driver only supports default value of slice_per_packet = 1
>> +			 *
>> +			 * TODO: Expand mipi_dsi_device struct to hold slice_per_packet info
>> +			 *       and adjust DSC math to account for slice_per_packet.
>> +			 */
>> +			wc = msm_host->dsc->slice_chunk_size + 1;
>>   
>>   		dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>>   			DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
>>
>> -- 
>> 2.40.1
>>
Dmitry Baryshkov May 21, 2023, 12:32 a.m. UTC | #3 
On 20/05/2023 00:17, Jessica Zhang wrote:
> Currently, slice_count is being used to calculate word count and
> pkt_per_line. In downstream, these values are calculated using slice per
> packet, which is not the same as slice_count.

I'd say the reference to downstream is not correct. We have seen cases 
where the vendor kernel contained errors. So it should be something like 
"Instead these values should be calculated using ...."

> 
> Slice count represents the number of soft slices per interface, and its
> value will not always match that of slice per packet. For example, it is
> possible to have cases where there are multiple soft slices per interface
> but the panel specifies only one slice per packet.
> 
> Thus, use the default value of one slice per packet and remove slice_count
> from the aforementioned calculations.
> 
> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to compute word count")
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 24 ++++++++++++++----------
>   1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index d04f8bbd707d..8c8858ee59ec 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -866,18 +866,15 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>   	 */
>   	slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay);
>   
> -	/*
> -	 * If slice_count is greater than slice_per_intf
> -	 * then default to 1. This can happen during partial
> -	 * update.
> -	 */
> -	if (dsc->slice_count > slice_per_intf)
> -		dsc->slice_count = 1;
> -
>   	total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
>   
>   	eol_byte_num = total_bytes_per_intf % 3;
> -	pkt_per_line = slice_per_intf / dsc->slice_count;
> +
> +	/*
> +	 * Default to 1 slice_per_pkt, so pkt_per_line will be equal to
> +	 * slice per intf.
> +	 */
> +	pkt_per_line = slice_per_intf;
>   
>   	if (is_cmd_mode) /* packet data type */
>   		reg = DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
> @@ -1001,7 +998,14 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>   		if (!msm_host->dsc)
>   			wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
>   		else
> -			wc = msm_host->dsc->slice_chunk_size * msm_host->dsc->slice_count + 1;
> +			/*
> +			 * When DSC is enabled, WC = slice_chunk_size * slice_per_packet + 1.
> +			 * Currently, the driver only supports default value of slice_per_packet = 1
> +			 *
> +			 * TODO: Expand mipi_dsi_device struct to hold slice_per_packet info
> +			 *       and adjust DSC math to account for slice_per_packet.
> +			 */
> +			wc = msm_host->dsc->slice_chunk_size + 1;
>   
>   		dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>   			DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
>
Jessica Zhang May 22, 2023, 6:38 p.m. UTC | #4 
On 5/20/2023 5:32 PM, Dmitry Baryshkov wrote:
> On 20/05/2023 00:17, Jessica Zhang wrote:
>> Currently, slice_count is being used to calculate word count and
>> pkt_per_line. In downstream, these values are calculated using slice per
>> packet, which is not the same as slice_count.
> 
> I'd say the reference to downstream is not correct. We have seen cases 
> where the vendor kernel contained errors. So it should be something like 
> "Instead these values should be calculated using ...."

Hi Dmitry,

Acked.

Thanks,

Jessica Zhang

> 
>>
>> Slice count represents the number of soft slices per interface, and its
>> value will not always match that of slice per packet. For example, it is
>> possible to have cases where there are multiple soft slices per interface
>> but the panel specifies only one slice per packet.
>>
>> Thus, use the default value of one slice per packet and remove 
>> slice_count
>> from the aforementioned calculations.
>>
>> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
>> Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to 
>> compute word count")
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 24 ++++++++++++++----------
>>   1 file changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index d04f8bbd707d..8c8858ee59ec 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -866,18 +866,15 @@ static void dsi_update_dsc_timing(struct 
>> msm_dsi_host *msm_host, bool is_cmd_mod
>>        */
>>       slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay);
>> -    /*
>> -     * If slice_count is greater than slice_per_intf
>> -     * then default to 1. This can happen during partial
>> -     * update.
>> -     */
>> -    if (dsc->slice_count > slice_per_intf)
>> -        dsc->slice_count = 1;
>> -
>>       total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
>>       eol_byte_num = total_bytes_per_intf % 3;
>> -    pkt_per_line = slice_per_intf / dsc->slice_count;
>> +
>> +    /*
>> +     * Default to 1 slice_per_pkt, so pkt_per_line will be equal to
>> +     * slice per intf.
>> +     */
>> +    pkt_per_line = slice_per_intf;
>>       if (is_cmd_mode) /* packet data type */
>>           reg = 
>> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
>> @@ -1001,7 +998,14 @@ static void dsi_timing_setup(struct msm_dsi_host 
>> *msm_host, bool is_bonded_dsi)
>>           if (!msm_host->dsc)
>>               wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
>>           else
>> -            wc = msm_host->dsc->slice_chunk_size * 
>> msm_host->dsc->slice_count + 1;
>> +            /*
>> +             * When DSC is enabled, WC = slice_chunk_size * 
>> slice_per_packet + 1.
>> +             * Currently, the driver only supports default value of 
>> slice_per_packet = 1
>> +             *
>> +             * TODO: Expand mipi_dsi_device struct to hold 
>> slice_per_packet info
>> +             *       and adjust DSC math to account for 
>> slice_per_packet.
>> +             */
>> +            wc = msm_host->dsc->slice_chunk_size + 1;
>>           dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>>               DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
>>
> 
> -- 
> With best wishes
> Dmitry
>
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 d04f8bbd707d..8c8858ee59ec 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -866,18 +866,15 @@  static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 	 */
 	slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay);
 
-	/*
-	 * If slice_count is greater than slice_per_intf
-	 * then default to 1. This can happen during partial
-	 * update.
-	 */
-	if (dsc->slice_count > slice_per_intf)
-		dsc->slice_count = 1;
-
 	total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
 
 	eol_byte_num = total_bytes_per_intf % 3;
-	pkt_per_line = slice_per_intf / dsc->slice_count;
+
+	/*
+	 * Default to 1 slice_per_pkt, so pkt_per_line will be equal to
+	 * slice per intf.
+	 */
+	pkt_per_line = slice_per_intf;
 
 	if (is_cmd_mode) /* packet data type */
 		reg = DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
@@ -1001,7 +998,14 @@  static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 		if (!msm_host->dsc)
 			wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
 		else
-			wc = msm_host->dsc->slice_chunk_size * msm_host->dsc->slice_count + 1;
+			/*
+			 * When DSC is enabled, WC = slice_chunk_size * slice_per_packet + 1.
+			 * Currently, the driver only supports default value of slice_per_packet = 1
+			 *
+			 * TODO: Expand mipi_dsi_device struct to hold slice_per_packet info
+			 *       and adjust DSC math to account for slice_per_packet.
+			 */
+			wc = msm_host->dsc->slice_chunk_size + 1;
 
 		dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
 			DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |