[PATCH v2 0/2] drm/msm/dp: Fix Type-C Timing

[PATCH v2 0/2] drm/msm/dp: Fix Type-C Timing

[James A. MacInnes]

SDM845 Type-C DisplayPort output inactive on DP Monitor and tears on HDMI.

During testing and research found that the dp and dpu drivers more
closely match later incarnations of the Android driver.
Compared against the 4.9 Android and found the porch timing and
wide bus elements were disabled.
Tested by commenting out code and the graphical glitches on
HDMI improved.
Fully turning off wide_bus resolved the single vertical line and
vblank errors when using non-native resolutions.
Removing the porch adjustment fixed HDMI and DisplayPort operated
correctly (for the first time) for all monitor supported resolutions.

Changes v1:
- Patch 1/2: Separated the descriptor from the sc7180 and turned off
             wide_bus support.
- Patch 2/2: Removed porch timing adjustment.

Changes v2:
- Patch 1/2: Removed unneeded assignment.
             Increased verbosity of commit message.
- Patch 2/2: Added comments to explain use of wide_bus_en.
             Increased verbosity of commit message.

Verified functionality on SDM845 using Lantronix SOM.
Tested with Type-C to DisplayPort and Dell Monitor.
Tested with Type-C hub with HDMI to Samsung 4k TV.

James A. MacInnes (2):
  drm/msm/dp: Disable wide bus support for SDM845
  drm/msm/disp: Correct porch timing for SDM845

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 8 ++++----
 drivers/gpu/drm/msm/dp/dp_display.c                  | 7 ++++++-
 2 files changed, 10 insertions(+), 5 deletions(-)

--
2.43.0

---
James A. MacInnes (2):
      drm/msm/dp: Disable wide bus support for SDM845
      drm/msm/disp: Correct porch timing for SDM845

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 14 +++++++++-----
 drivers/gpu/drm/msm/dp/dp_display.c                  |  7 ++++++-
 2 files changed, 15 insertions(+), 6 deletions(-)
---
base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04
change-id: 20250212-sdm845_dp-6ed993977a53

Best regards,
-- 
James A. MacInnes <james.a.macinnes@gmail.com>

[PATCH v2 1/2] drm/msm/dp: Disable wide bus support for SDM845

[James A. MacInnes]

SDM845 DPU hardware is rev 4.0.0 per hardware documents.
Original patch to enable wide_bus operation did not take into account
the SDM845 and it got carried over by accident.

- Incorrect setting caused inoperable DisplayPort.
- Corrected by separating SDM845 into its own descriptor.

Fixes: c7c412202623 ("drm/msm/dp: enable widebus on all relevant chipsets")
Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index aff51bb973eb..e30cccd63910 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -126,6 +126,11 @@ static const struct msm_dp_desc msm_dp_desc_sa8775p[] = {
 	{}
 };
 
+static const struct msm_dp_desc msm_dp_desc_sdm845[] = {
+	{ .io_start = 0x0ae90000, .id = MSM_DP_CONTROLLER_0 },
+	{}
+};
+
 static const struct msm_dp_desc msm_dp_desc_sc7180[] = {
 	{ .io_start = 0x0ae90000, .id = MSM_DP_CONTROLLER_0, .wide_bus_supported = true },
 	{}
@@ -178,7 +183,7 @@ static const struct of_device_id msm_dp_dt_match[] = {
 	{ .compatible = "qcom,sc8180x-edp", .data = &msm_dp_desc_sc8180x },
 	{ .compatible = "qcom,sc8280xp-dp", .data = &msm_dp_desc_sc8280xp },
 	{ .compatible = "qcom,sc8280xp-edp", .data = &msm_dp_desc_sc8280xp },
-	{ .compatible = "qcom,sdm845-dp", .data = &msm_dp_desc_sc7180 },
+	{ .compatible = "qcom,sdm845-dp", .data = &msm_dp_desc_sdm845 },
 	{ .compatible = "qcom,sm8350-dp", .data = &msm_dp_desc_sc7180 },
 	{ .compatible = "qcom,sm8650-dp", .data = &msm_dp_desc_sm8650 },
 	{ .compatible = "qcom,x1e80100-dp", .data = &msm_dp_desc_x1e80100 },

-- 
2.43.0

[PATCH v2 2/2] drm/msm/disp: Correct porch timing for SDM845

[James A. MacInnes]

Type-C DisplayPort inoperable due to incorrect porch settings.
- Re-used wide_bus_en as flag to prevent porch shifting

Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index abd6600046cb..a21addc4794f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -94,17 +94,21 @@ static void drm_mode_to_intf_timing_params(
 		timing->vsync_polarity = 0;
 	}
 
-	/* for DP/EDP, Shift timings to align it to bottom right */
-	if (phys_enc->hw_intf->cap->type == INTF_DP) {
+	timing->wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
+	timing->compression_en = dpu_encoder_is_dsc_enabled(phys_enc->parent);
+
+	/*
+	 *  For DP/EDP, Shift timings to align it to bottom right.
+	 *  wide_bus_en is set for everything excluding SDM845 &
+	 *  porch changes cause DisplayPort failure and HDMI tearing.
+	 */
+	if (phys_enc->hw_intf->cap->type == INTF_DP && timing->wide_bus_en) {
 		timing->h_back_porch += timing->h_front_porch;
 		timing->h_front_porch = 0;
 		timing->v_back_porch += timing->v_front_porch;
 		timing->v_front_porch = 0;
 	}
 
-	timing->wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
-	timing->compression_en = dpu_encoder_is_dsc_enabled(phys_enc->parent);
-
 	/*
 	 * for DP, divide the horizonal parameters by 2 when
 	 * widebus is enabled

-- 
2.43.0

Re: [PATCH v2 1/2] drm/msm/dp: Disable wide bus support for SDM845

[Marijn Suijten]

On 2025-02-12 15:03:46, James A. MacInnes wrote:
> SDM845 DPU hardware is rev 4.0.0 per hardware documents.
> Original patch to enable wide_bus operation did not take into account
> the SDM845 and it got carried over by accident.
> 
> - Incorrect setting caused inoperable DisplayPort.
> - Corrected by separating SDM845 into its own descriptor.

If anything I'd have appreciated to see our conversation in v1 pasted here
verbatim which is of the right verbosity to explain this.  I can't do much with
a list of two items.

I don't have a clearer way of explaining what all I find confusing about this
description, so let me propose what I would have written if this was my patch
instead:

	When widebus was enabled for DisplayPort in commit c7c412202623 ("drm/msm/dp:
	enable widebus on all relevant chipsets") it was clarified that it is only
	supported on DPU 5.0.0 onwards which includes SC7180 on DPU revision 6.2.
	However, this patch missed that the description structure for SC7180 is also
	reused for SDM845 (because of identical io_start address) which is only DPU
	4.0.0, leading to a wrongly enbled widebus feature and corruption on that
	platform.

	Create a separate msm_dp_desc_sdm845 structure for this SoC compatible,
	with the wide_bus_supported flag turned off.

	Note that no other DisplayPort compatibles currently exist for SoCs older
	than DPU 4.0.0 besides SDM845.

Hope I'm not considered being too picky.  I first sketch **how** the original
patch created a problem, then explain how this patch is intending to fix it,
and finally describe that we went a step further and ensured no other SoCs
are suffering from a similar problem.

- Marijn

> 
> Fixes: c7c412202623 ("drm/msm/dp: enable widebus on all relevant chipsets")
> Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index aff51bb973eb..e30cccd63910 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -126,6 +126,11 @@ static const struct msm_dp_desc msm_dp_desc_sa8775p[] = {
>  	{}
>  };
>  
> +static const struct msm_dp_desc msm_dp_desc_sdm845[] = {
> +	{ .io_start = 0x0ae90000, .id = MSM_DP_CONTROLLER_0 },
> +	{}
> +};
> +
>  static const struct msm_dp_desc msm_dp_desc_sc7180[] = {
>  	{ .io_start = 0x0ae90000, .id = MSM_DP_CONTROLLER_0, .wide_bus_supported = true },
>  	{}
> @@ -178,7 +183,7 @@ static const struct of_device_id msm_dp_dt_match[] = {
>  	{ .compatible = "qcom,sc8180x-edp", .data = &msm_dp_desc_sc8180x },
>  	{ .compatible = "qcom,sc8280xp-dp", .data = &msm_dp_desc_sc8280xp },
>  	{ .compatible = "qcom,sc8280xp-edp", .data = &msm_dp_desc_sc8280xp },
> -	{ .compatible = "qcom,sdm845-dp", .data = &msm_dp_desc_sc7180 },
> +	{ .compatible = "qcom,sdm845-dp", .data = &msm_dp_desc_sdm845 },
>  	{ .compatible = "qcom,sm8350-dp", .data = &msm_dp_desc_sc7180 },
>  	{ .compatible = "qcom,sm8650-dp", .data = &msm_dp_desc_sm8650 },
>  	{ .compatible = "qcom,x1e80100-dp", .data = &msm_dp_desc_x1e80100 },
> 
> -- 
> 2.43.0
> 

Re: [PATCH v2 1/2] drm/msm/dp: Disable wide bus support for SDM845

[Dmitry Baryshkov]

On Thu, Feb 13, 2025 at 12:41:02AM +0100, Marijn Suijten wrote:
> On 2025-02-12 15:03:46, James A. MacInnes wrote:
> > SDM845 DPU hardware is rev 4.0.0 per hardware documents.
> > Original patch to enable wide_bus operation did not take into account
> > the SDM845 and it got carried over by accident.
> > 
> > - Incorrect setting caused inoperable DisplayPort.
> > - Corrected by separating SDM845 into its own descriptor.
> 
> If anything I'd have appreciated to see our conversation in v1 pasted here
> verbatim which is of the right verbosity to explain this.  I can't do much with
> a list of two items.
> 
> I don't have a clearer way of explaining what all I find confusing about this
> description, so let me propose what I would have written if this was my patch
> instead:
> 
> 	When widebus was enabled for DisplayPort in commit c7c412202623 ("drm/msm/dp:
> 	enable widebus on all relevant chipsets") it was clarified that it is only
> 	supported on DPU 5.0.0 onwards which includes SC7180 on DPU revision 6.2.
> 	However, this patch missed that the description structure for SC7180 is also
> 	reused for SDM845 (because of identical io_start address) which is only DPU
> 	4.0.0, leading to a wrongly enbled widebus feature and corruption on that
> 	platform.
> 
> 	Create a separate msm_dp_desc_sdm845 structure for this SoC compatible,
> 	with the wide_bus_supported flag turned off.
> 
> 	Note that no other DisplayPort compatibles currently exist for SoCs older
> 	than DPU 4.0.0 besides SDM845.

With more or less similar commit message:


Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


> 
> Hope I'm not considered being too picky.  I first sketch **how** the original
> patch created a problem, then explain how this patch is intending to fix it,
> and finally describe that we went a step further and ensured no other SoCs
> are suffering from a similar problem.
> 
> - Marijn
> 
> > 
> > Fixes: c7c412202623 ("drm/msm/dp: enable widebus on all relevant chipsets")
> > Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
> > ---
> >  drivers/gpu/drm/msm/dp/dp_display.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > index aff51bb973eb..e30cccd63910 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -126,6 +126,11 @@ static const struct msm_dp_desc msm_dp_desc_sa8775p[] = {
> >  	{}
> >  };
> >  
> > +static const struct msm_dp_desc msm_dp_desc_sdm845[] = {
> > +	{ .io_start = 0x0ae90000, .id = MSM_DP_CONTROLLER_0 },
> > +	{}
> > +};
> > +
> >  static const struct msm_dp_desc msm_dp_desc_sc7180[] = {
> >  	{ .io_start = 0x0ae90000, .id = MSM_DP_CONTROLLER_0, .wide_bus_supported = true },
> >  	{}
> > @@ -178,7 +183,7 @@ static const struct of_device_id msm_dp_dt_match[] = {
> >  	{ .compatible = "qcom,sc8180x-edp", .data = &msm_dp_desc_sc8180x },
> >  	{ .compatible = "qcom,sc8280xp-dp", .data = &msm_dp_desc_sc8280xp },
> >  	{ .compatible = "qcom,sc8280xp-edp", .data = &msm_dp_desc_sc8280xp },
> > -	{ .compatible = "qcom,sdm845-dp", .data = &msm_dp_desc_sc7180 },
> > +	{ .compatible = "qcom,sdm845-dp", .data = &msm_dp_desc_sdm845 },
> >  	{ .compatible = "qcom,sm8350-dp", .data = &msm_dp_desc_sc7180 },
> >  	{ .compatible = "qcom,sm8650-dp", .data = &msm_dp_desc_sm8650 },
> >  	{ .compatible = "qcom,x1e80100-dp", .data = &msm_dp_desc_x1e80100 },
> > 
> > -- 
> > 2.43.0
> > 

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/2] drm/msm/disp: Correct porch timing for SDM845

[Dmitry Baryshkov]

On Wed, Feb 12, 2025 at 03:03:47PM -0800, James A. MacInnes wrote:
> Type-C DisplayPort inoperable due to incorrect porch settings.
> - Re-used wide_bus_en as flag to prevent porch shifting

Unfortunately I don't know enough details to comment on this change.
Maybe Abhinav can check it. I can only notice that msm-4.14 disables
programmable_fetch_config for non-DSI calls. Would disabling that call
for DP interface fix your issue?

> 
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> index abd6600046cb..a21addc4794f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> @@ -94,17 +94,21 @@ static void drm_mode_to_intf_timing_params(
>  		timing->vsync_polarity = 0;
>  	}
>  
> -	/* for DP/EDP, Shift timings to align it to bottom right */
> -	if (phys_enc->hw_intf->cap->type == INTF_DP) {
> +	timing->wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
> +	timing->compression_en = dpu_encoder_is_dsc_enabled(phys_enc->parent);
> +
> +	/*
> +	 *  For DP/EDP, Shift timings to align it to bottom right.
> +	 *  wide_bus_en is set for everything excluding SDM845 &
> +	 *  porch changes cause DisplayPort failure and HDMI tearing.
> +	 */
> +	if (phys_enc->hw_intf->cap->type == INTF_DP && timing->wide_bus_en) {
>  		timing->h_back_porch += timing->h_front_porch;
>  		timing->h_front_porch = 0;
>  		timing->v_back_porch += timing->v_front_porch;
>  		timing->v_front_porch = 0;
>  	}
>  
> -	timing->wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
> -	timing->compression_en = dpu_encoder_is_dsc_enabled(phys_enc->parent);
> -
>  	/*
>  	 * for DP, divide the horizonal parameters by 2 when
>  	 * widebus is enabled
> 
> -- 
> 2.43.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 1/2] drm/msm/dp: Disable wide bus support for SDM845

[Abhinav Kumar]

On 2/12/2025 3:41 PM, Marijn Suijten wrote:
> On 2025-02-12 15:03:46, James A. MacInnes wrote:
>> SDM845 DPU hardware is rev 4.0.0 per hardware documents.
>> Original patch to enable wide_bus operation did not take into account
>> the SDM845 and it got carried over by accident.
>>
>> - Incorrect setting caused inoperable DisplayPort.
>> - Corrected by separating SDM845 into its own descriptor.
> 
> If anything I'd have appreciated to see our conversation in v1 pasted here
> verbatim which is of the right verbosity to explain this.  I can't do much with
> a list of two items.
> 
> I don't have a clearer way of explaining what all I find confusing about this
> description, so let me propose what I would have written if this was my patch
> instead:
> 
> 	When widebus was enabled for DisplayPort in commit c7c412202623 ("drm/msm/dp:
> 	enable widebus on all relevant chipsets") it was clarified that it is only
> 	supported on DPU 5.0.0 onwards which includes SC7180 on DPU revision 6.2.
> 	However, this patch missed that the description structure for SC7180 is also
> 	reused for SDM845 (because of identical io_start address) which is only DPU
> 	4.0.0, leading to a wrongly enbled widebus feature and corruption on that
> 	platform.
> 
> 	Create a separate msm_dp_desc_sdm845 structure for this SoC compatible,
> 	with the wide_bus_supported flag turned off.
> 
> 	Note that no other DisplayPort compatibles currently exist for SoCs older
> 	than DPU 4.0.0 besides SDM845.
> 

Yes, this is good description. Thanks Marijn!

> Hope I'm not considered being too picky.  I first sketch **how** the original
> patch created a problem, then explain how this patch is intending to fix it,
> and finally describe that we went a step further and ensured no other SoCs
> are suffering from a similar problem.
> 
> - Marijn
> 

Its indeed a bug introduced due to msm_dp_desc_sc7180 re-use. There is 
no widebus on this chipset.


With the commit text fixed like above,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>
>> Fixes: c7c412202623 ("drm/msm/dp: enable widebus on all relevant chipsets")
>> Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_display.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index aff51bb973eb..e30cccd63910 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -126,6 +126,11 @@ static const struct msm_dp_desc msm_dp_desc_sa8775p[] = {
>>   	{}
>>   };
>>   
>> +static const struct msm_dp_desc msm_dp_desc_sdm845[] = {
>> +	{ .io_start = 0x0ae90000, .id = MSM_DP_CONTROLLER_0 },
>> +	{}
>> +};
>> +
>>   static const struct msm_dp_desc msm_dp_desc_sc7180[] = {
>>   	{ .io_start = 0x0ae90000, .id = MSM_DP_CONTROLLER_0, .wide_bus_supported = true },
>>   	{}
>> @@ -178,7 +183,7 @@ static const struct of_device_id msm_dp_dt_match[] = {
>>   	{ .compatible = "qcom,sc8180x-edp", .data = &msm_dp_desc_sc8180x },
>>   	{ .compatible = "qcom,sc8280xp-dp", .data = &msm_dp_desc_sc8280xp },
>>   	{ .compatible = "qcom,sc8280xp-edp", .data = &msm_dp_desc_sc8280xp },
>> -	{ .compatible = "qcom,sdm845-dp", .data = &msm_dp_desc_sc7180 },
>> +	{ .compatible = "qcom,sdm845-dp", .data = &msm_dp_desc_sdm845 },
>>   	{ .compatible = "qcom,sm8350-dp", .data = &msm_dp_desc_sc7180 },
>>   	{ .compatible = "qcom,sm8650-dp", .data = &msm_dp_desc_sm8650 },
>>   	{ .compatible = "qcom,x1e80100-dp", .data = &msm_dp_desc_x1e80100 },
>>
>> -- 
>> 2.43.0
>>

Re: [PATCH v2 2/2] drm/msm/disp: Correct porch timing for SDM845

[Abhinav Kumar]

On 2/12/2025 4:04 PM, Dmitry Baryshkov wrote:
> On Wed, Feb 12, 2025 at 03:03:47PM -0800, James A. MacInnes wrote:
>> Type-C DisplayPort inoperable due to incorrect porch settings.
>> - Re-used wide_bus_en as flag to prevent porch shifting
> 
> Unfortunately I don't know enough details to comment on this change.
> Maybe Abhinav can check it. I can only notice that msm-4.14 disables
> programmable_fetch_config for non-DSI calls. Would disabling that call
> for DP interface fix your issue?
> 

Yes, this piece of timing adjustment matches what we have even without 
widebus.

I do agree about the programmable fetch that it is enabled only on DSI 
even on the latest kernels.

698 	if (phys_enc->hw_intf->cap->type == INTF_DSI)
699 		programmable_fetch_config(phys_enc, &timing_params);

We can try if that works.

>>
>> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
>> Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 14 +++++++++-----
>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> index abd6600046cb..a21addc4794f 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> @@ -94,17 +94,21 @@ static void drm_mode_to_intf_timing_params(
>>   		timing->vsync_polarity = 0;
>>   	}
>>   
>> -	/* for DP/EDP, Shift timings to align it to bottom right */
>> -	if (phys_enc->hw_intf->cap->type == INTF_DP) {
>> +	timing->wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
>> +	timing->compression_en = dpu_encoder_is_dsc_enabled(phys_enc->parent);
>> +
>> +	/*
>> +	 *  For DP/EDP, Shift timings to align it to bottom right.
>> +	 *  wide_bus_en is set for everything excluding SDM845 &
>> +	 *  porch changes cause DisplayPort failure and HDMI tearing.
>> +	 */
>> +	if (phys_enc->hw_intf->cap->type == INTF_DP && timing->wide_bus_en) {
>>   		timing->h_back_porch += timing->h_front_porch;
>>   		timing->h_front_porch = 0;
>>   		timing->v_back_porch += timing->v_front_porch;
>>   		timing->v_front_porch = 0;
>>   	}
>>   
>> -	timing->wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
>> -	timing->compression_en = dpu_encoder_is_dsc_enabled(phys_enc->parent);
>> -
>>   	/*
>>   	 * for DP, divide the horizonal parameters by 2 when
>>   	 * widebus is enabled
>>
>> -- 
>> 2.43.0
>>
> 

Re: [PATCH v2 1/2] drm/msm/dp: Disable wide bus support for SDM845

[James A. MacInnes]

On Thu, 13 Feb 2025 01:58:06 +0200
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On Thu, Feb 13, 2025 at 12:41:02AM +0100, Marijn Suijten wrote:
> > On 2025-02-12 15:03:46, James A. MacInnes wrote:  
> > > SDM845 DPU hardware is rev 4.0.0 per hardware documents.
> > > Original patch to enable wide_bus operation did not take into
> > > account the SDM845 and it got carried over by accident.
> > > 
> > > - Incorrect setting caused inoperable DisplayPort.
> > > - Corrected by separating SDM845 into its own descriptor.  
> > 
> > If anything I'd have appreciated to see our conversation in v1
> > pasted here verbatim which is of the right verbosity to explain
> > this.  I can't do much with a list of two items.
> > 
> > I don't have a clearer way of explaining what all I find confusing
> > about this description, so let me propose what I would have written
> > if this was my patch instead:
> > 
> > 	When widebus was enabled for DisplayPort in commit
> > c7c412202623 ("drm/msm/dp: enable widebus on all relevant
> > chipsets") it was clarified that it is only supported on DPU 5.0.0
> > onwards which includes SC7180 on DPU revision 6.2. However, this
> > patch missed that the description structure for SC7180 is also
> > reused for SDM845 (because of identical io_start address) which is
> > only DPU 4.0.0, leading to a wrongly enbled widebus feature and
> > corruption on that platform.
> > 
> > 	Create a separate msm_dp_desc_sdm845 structure for this SoC
> > compatible, with the wide_bus_supported flag turned off.
> > 
> > 	Note that no other DisplayPort compatibles currently exist
> > for SoCs older than DPU 4.0.0 besides SDM845.  
> 
> With more or less similar commit message:
> 
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> 
> > 
> > Hope I'm not considered being too picky.  I first sketch **how**
> > the original patch created a problem, then explain how this patch
> > is intending to fix it, and finally describe that we went a step
> > further and ensured no other SoCs are suffering from a similar
> > problem.
> > 
> > - Marijn
> >
Not too picky at all. I will use your text. I apologize as I had changed
the cover instead of the patch. I will do my best to balance too many
words and not enough.

Would it be appropriate to split this patch and the other into separate
submissions?

Thank you again.

 - James

> > > 
> > > Fixes: c7c412202623 ("drm/msm/dp: enable widebus on all relevant
> > > chipsets") Signed-off-by: James A. MacInnes
> > > <james.a.macinnes@gmail.com> ---
> > >  drivers/gpu/drm/msm/dp/dp_display.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > > b/drivers/gpu/drm/msm/dp/dp_display.c index
> > > aff51bb973eb..e30cccd63910 100644 ---
> > > a/drivers/gpu/drm/msm/dp/dp_display.c +++
> > > b/drivers/gpu/drm/msm/dp/dp_display.c @@ -126,6 +126,11 @@ static
> > > const struct msm_dp_desc msm_dp_desc_sa8775p[] = { {}
> > >  };
> > >  
> > > +static const struct msm_dp_desc msm_dp_desc_sdm845[] = {
> > > +	{ .io_start = 0x0ae90000, .id = MSM_DP_CONTROLLER_0 },
> > > +	{}
> > > +};
> > > +
> > >  static const struct msm_dp_desc msm_dp_desc_sc7180[] = {
> > >  	{ .io_start = 0x0ae90000, .id = MSM_DP_CONTROLLER_0,
> > > .wide_bus_supported = true }, {}
> > > @@ -178,7 +183,7 @@ static const struct of_device_id
> > > msm_dp_dt_match[] = { { .compatible = "qcom,sc8180x-edp", .data =
> > > &msm_dp_desc_sc8180x }, { .compatible = "qcom,sc8280xp-dp", .data
> > > = &msm_dp_desc_sc8280xp }, { .compatible = "qcom,sc8280xp-edp",
> > > .data = &msm_dp_desc_sc8280xp },
> > > -	{ .compatible = "qcom,sdm845-dp", .data =
> > > &msm_dp_desc_sc7180 },
> > > +	{ .compatible = "qcom,sdm845-dp", .data =
> > > &msm_dp_desc_sdm845 }, { .compatible = "qcom,sm8350-dp", .data =
> > > &msm_dp_desc_sc7180 }, { .compatible = "qcom,sm8650-dp", .data =
> > > &msm_dp_desc_sm8650 }, { .compatible = "qcom,x1e80100-dp", .data
> > > = &msm_dp_desc_x1e80100 },
> > > 
> > > -- 
> > > 2.43.0
> > >   
> 

Re: [PATCH v2 2/2] drm/msm/disp: Correct porch timing for SDM845

[Dmitry Baryshkov]

On Wed, Feb 12, 2025 at 03:03:47PM -0800, James A. MacInnes wrote:
> Type-C DisplayPort inoperable due to incorrect porch settings.
> - Re-used wide_bus_en as flag to prevent porch shifting
> 
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 

I was able to run DP tests on SDM845. These changes are required on that
platform, disabling programmable fetch is not enough.

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/2] drm/msm/disp: Correct porch timing for SDM845

[Dmitry Baryshkov]

On Wed, Feb 12, 2025 at 03:03:47PM -0800, James A. MacInnes wrote:
> Type-C DisplayPort inoperable due to incorrect porch settings.
> - Re-used wide_bus_en as flag to prevent porch shifting
> 
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

-- 
With best wishes
Dmitry