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

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

[James A. MacInnes]

SDM845 DisplayPort output inop on DP Monitor and tears on HDMI.

Fixed
 - SDM845 does not support wide mode.
 - SDM845 does not need porch shift.

Verified functionality on SDM845.

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

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

[James A. MacInnes]

SDM845 DPU hardware is rev 4.0.0 per hardware document.

Incorrect setting caused inop displayport.
Corrected by separating SDM845 to 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..2cbdbf85a85c 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, .wide_bus_supported = false },
+	{}
+};
+
 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 1/2] drm/msm/dp: Disable wide bus support for SDM845

[Marijn Suijten]

On 2025-02-11 19:42:24, James A. MacInnes wrote:
> SDM845 DPU hardware is rev 4.0.0 per hardware document.

Just checking: version 4.0.0 is not named in the code that you're changing: are
you mentioning this because the patch you're fixing here [1] says that widebus
is "recommended" on 5.x.x which includes sc7180, yet didn't account for that
sc7180_dp_descs also being used in the SDM845 compatible which is 4.0.0?  That
is something worth mentioning in the patch description.

[1]: https://lore.kernel.org/linux-arm-msm/20240730195012.2595980-1-quic_abhinavk@quicinc.com/

> 
> Incorrect setting caused inop displayport.

Inop doesn't seem to be a common abbreviation, there's enough space to spell
out "inoperative".  And spend some more words on _why_ this is an "incorrect
setting" in the first place  (based on the suggestion above)?

I am trying to remember the details from the original widebus series: we
discussed that the INTF_CFG2_DATABUS_WIDEN flag was available starting with DPU
4.0.0 (IIRC, cannot find the source), yet the DSI host only supports it from
6G v2.5 onwards (SC7280 and up?) [2].  Seems a similar limitation applies to
DP hosts.

[2]: https://lore.kernel.org/linux-arm-msm/20230822-add-widebus-support-v4-4-9dc86083d6ea@quicinc.com/

> Corrected by separating SDM845 to own descriptor.

its own*

> 
> Fixes: c7c412202623 ("drm/msm/dp: enable widebus on all relevant chipsets")
> 

No need for empty lines between trailing tags.

> 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..2cbdbf85a85c 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, .wide_bus_supported = false },

We can probably drop the assignment, it'll be false/0 by default.

- Marijn

> +	{}
> +};
> +
>  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 1/2] drm/msm/dp: Disable wide bus support for SDM845

[James A. MacInnes]

On Wed, 12 Feb 2025 11:03:39 +0100
Marijn Suijten <marijn.suijten@somainline.org> wrote:

> On 2025-02-11 19:42:24, James A. MacInnes wrote:
> > SDM845 DPU hardware is rev 4.0.0 per hardware document.  
> 
> Just checking: version 4.0.0 is not named in the code that you're
> changing: are you mentioning this because the patch you're fixing
> here [1] says that widebus is "recommended" on 5.x.x which includes
> sc7180, yet didn't account for that sc7180_dp_descs also being used
> in the SDM845 compatible which is 4.0.0?  That is something worth
> mentioning in the patch description.
> 
> [1]:
> https://lore.kernel.org/linux-arm-msm/20240730195012.2595980-1-quic_abhinavk@quicinc.com/

That is correct. All the 'modern' Qualcomm dpus got wide-bus turned on
and as the SDM845 has not had a working Type-C port it was never tested.

Happy to add that to my commit message. This is my second submission to
the kernel forum.

> 
> > 
> > Incorrect setting caused inop displayport.  
> 
> Inop doesn't seem to be a common abbreviation, there's enough space
> to spell out "inoperative".  And spend some more words on _why_ this
> is an "incorrect setting" in the first place  (based on the
> suggestion above)?
> 

Happy to spend many more words. Just looking to meet the requirements
for the boards and try to not ruffle any feathers. It seems to easy to
be far too verbose.

> I am trying to remember the details from the original widebus series:
> we discussed that the INTF_CFG2_DATABUS_WIDEN flag was available
> starting with DPU 4.0.0 (IIRC, cannot find the source), yet the DSI
> host only supports it from 6G v2.5 onwards (SC7280 and up?) [2].
> Seems a similar limitation applies to DP hosts.
> 
> [2]:
> https://lore.kernel.org/linux-arm-msm/20230822-add-widebus-support-v4-4-9dc86083d6ea@quicinc.com/
> 

That would reflect the testing I have performed. With the wide_bus
system enabled, The MIPI display functions fine, but the Altmode
DisplayPort (type-c to DP) does not turn on a standard monitor and the
type-c to HDMI connection has either a system that does not sync
(horrific flashing) or just a single solid line. At other resolutions I
was getting vblank errors from deeper into the system.

> > Corrected by separating SDM845 to own descriptor.  
> 
> its own*
> 
> > 
> > Fixes: c7c412202623 ("drm/msm/dp: enable widebus on all relevant
> > chipsets") 
> 
> No need for empty lines between trailing tags.
> 
> > 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..2cbdbf85a85c 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,
> > .wide_bus_supported = false },  
> 
> We can probably drop the assignment, it'll be false/0 by default.
> 
> - Marijn
> 

Thank you, I will get that cleaned up. 

> > +	{}
> > +};
> > +
> >  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 1/2] drm/msm/dp: Disable wide bus support for SDM845

[Marijn Suijten]

On 2025-02-12 08:16:41, James A. MacInnes wrote:
> On Wed, 12 Feb 2025 11:03:39 +0100
> Marijn Suijten <marijn.suijten@somainline.org> wrote:
> 
> > On 2025-02-11 19:42:24, James A. MacInnes wrote:
> > > SDM845 DPU hardware is rev 4.0.0 per hardware document.  
> > 
> > Just checking: version 4.0.0 is not named in the code that you're
> > changing: are you mentioning this because the patch you're fixing
> > here [1] says that widebus is "recommended" on 5.x.x which includes
> > sc7180, yet didn't account for that sc7180_dp_descs also being used
> > in the SDM845 compatible which is 4.0.0?  That is something worth
> > mentioning in the patch description.
> > 
> > [1]:
> > https://lore.kernel.org/linux-arm-msm/20240730195012.2595980-1-quic_abhinavk@quicinc.com/
> 
> That is correct. All the 'modern' Qualcomm dpus got wide-bus turned on
> and as the SDM845 has not had a working Type-C port it was never tested.
> 
> Happy to add that to my commit message. This is my second submission to
> the kernel forum.

Thanks, would be great to include all of this as context in some form!

> > > Incorrect setting caused inop displayport.  
> > 
> > Inop doesn't seem to be a common abbreviation, there's enough space
> > to spell out "inoperative".  And spend some more words on _why_ this
> > is an "incorrect setting" in the first place  (based on the
> > suggestion above)?
> > 
> 
> Happy to spend many more words. Just looking to meet the requirements
> for the boards and try to not ruffle any feathers. It seems to easy to
> be far too verbose.

I'd say: the more the merrier.  Especially DRM/MSM is *flooded* with
poorly-worded series, if they include a description (beyond paraphrasing the
title...) at all :(.  Having all this context available makes it possible
to understand the debugging and thought process behind a change.  Links to
downstream are a great way to showcase how things are dealt with there, as it's
generally the only source of "documentation" for these drivers (to hobbyist
contributors like myself).

> 
> > I am trying to remember the details from the original widebus series:
> > we discussed that the INTF_CFG2_DATABUS_WIDEN flag was available
> > starting with DPU 4.0.0 (IIRC, cannot find the source), yet the DSI
> > host only supports it from 6G v2.5 onwards (SC7280 and up?) [2].
> > Seems a similar limitation applies to DP hosts.
> > 
> > [2]:
> > https://lore.kernel.org/linux-arm-msm/20230822-add-widebus-support-v4-4-9dc86083d6ea@quicinc.com/
> > 
> 
> That would reflect the testing I have performed. With the wide_bus
> system enabled, The MIPI display functions fine, but the Altmode
> DisplayPort (type-c to DP) does not turn on a standard monitor and the
> type-c to HDMI connection has either a system that does not sync
> (horrific flashing) or just a single solid line. At other resolutions I
> was getting vblank errors from deeper into the system.

Technically the MIPI DSI display (data connection between DPU INTF and DSI host
AFAIR) shouldn't be using wide_bus, AFAIK it has DSI host 6G v2.4 which doesn't
support it according to the code.

Thanks for taking all these suggestions!

- Marijn

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

[James A. MacInnes]

Type-C DisplayPort inop due to incorrect settings.

SDM845 (DPU 4.0) lacks wide_bus support; porch shift removed.

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 | 8 ++++----
 1 file changed, 4 insertions(+), 4 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..3e0fef0955ce 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,17 @@ static void drm_mode_to_intf_timing_params(
 		timing->vsync_polarity = 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/EDP, Shift timings to align it to bottom right */
-	if (phys_enc->hw_intf->cap->type == INTF_DP) {
+	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 2/2] drm/msm/disp: Correct porch timing for SDM845

[Marijn Suijten]

On 2025-02-11 19:42:25, James A. MacInnes wrote:
> Type-C DisplayPort inop due to incorrect settings.
> 
> SDM845 (DPU 4.0) lacks wide_bus support; porch shift removed.

Same comment on "inop", elaborating the meaning of "incorrect settings" and
describing relevance to DPU 4.0 from patch 1/2.

> 
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")

This commit came long before wide bus support, are you sure this is the right
Fixes tag?

> 

Drop empty line between tags.

> Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 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..3e0fef0955ce 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,17 @@ static void drm_mode_to_intf_timing_params(
>  		timing->vsync_polarity = 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/EDP, Shift timings to align it to bottom right */
> -	if (phys_enc->hw_intf->cap->type == INTF_DP) {
> +	if (phys_enc->hw_intf->cap->type == INTF_DP && timing->wide_bus_en) {

This code existed long before widebus: are you sure this is correct?

Note that an identical `if` condtion exists right below, under the "for DP,
divide the horizonal parameters by 2 when widebus is enabled" comment.  If
this "Shift timings to align it to bottom right" should really only happen when
widebus is enabled, move the code into that instead.

- Marijn

>  		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 2/2] drm/msm/disp: Correct porch timing for SDM845

[James A. MacInnes]

On Wed, 12 Feb 2025 11:13:24 +0100
Marijn Suijten <marijn.suijten@somainline.org> wrote:

> On 2025-02-11 19:42:25, James A. MacInnes wrote:
> > Type-C DisplayPort inop due to incorrect settings.
> > 
> > SDM845 (DPU 4.0) lacks wide_bus support; porch shift removed.
> 
> Same comment on "inop", elaborating the meaning of "incorrect
> settings" and describing relevance to DPU 4.0 from patch 1/2.
> 

Again, happy to use more words.

> > 
> > Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> 
> This commit came long before wide bus support, are you sure this is
> the right Fixes tag?
> 

Yes, I went back to the Android 4.9 driver (that was working) and found
that the porch shift was not there. After experimenting with removing
the porch shift code, I had fully working video. As the SDM845 is the
only chip that doesn't use wide_bus, the pair are not related, but each
one contributes to no/poor video output.

> > 
> 
> Drop empty line between tags.
> 
> > Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 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..3e0fef0955ce 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,17 @@ static void drm_mode_to_intf_timing_params(
> > timing->vsync_polarity = 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/EDP, Shift timings to align it to bottom right */
> > -	if (phys_enc->hw_intf->cap->type == INTF_DP) {
> > +	if (phys_enc->hw_intf->cap->type == INTF_DP &&
> > timing->wide_bus_en) {
> 
> This code existed long before widebus: are you sure this is correct?
> 
> Note that an identical `if` condtion exists right below, under the
> "for DP, divide the horizonal parameters by 2 when widebus is
> enabled" comment.  If this "Shift timings to align it to bottom
> right" should really only happen when widebus is enabled, move the
> code into that instead.
> 
> - Marijn
> 

Happy to condense it. I left it in two sections for clear review at
this point. As stated above, I reused the wide_bus parameter as the
SDM845 appears to be the only affected chip.

> >  		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 2/2] drm/msm/disp: Correct porch timing for SDM845

[Marijn Suijten]

On 2025-02-12 08:23:03, James A. MacInnes wrote:
> On Wed, 12 Feb 2025 11:13:24 +0100
> Marijn Suijten <marijn.suijten@somainline.org> wrote:
> 
> > On 2025-02-11 19:42:25, James A. MacInnes wrote:
> > > Type-C DisplayPort inop due to incorrect settings.
> > > 
> > > SDM845 (DPU 4.0) lacks wide_bus support; porch shift removed.
> > 
> > Same comment on "inop", elaborating the meaning of "incorrect
> > settings" and describing relevance to DPU 4.0 from patch 1/2.
> > 
> 
> Again, happy to use more words.
> 
> > > 
> > > Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> > 
> > This commit came long before wide bus support, are you sure this is
> > the right Fixes tag?
> > 
> 
> Yes, I went back to the Android 4.9 driver (that was working) and found
> that the porch shift was not there. After experimenting with removing
> the porch shift code, I had fully working video. As the SDM845 is the
> only chip that doesn't use wide_bus, the pair are not related, but each
> one contributes to no/poor video output.

Ack: such information is exactly critical to have in the patch description.
Looking forward to seeing it in v2 :).  It's not something I have been able to
deduce from "SDM845 lacks wide_bus support; porch shift removed".

> > > 
> > 
> > Drop empty line between tags.
> > 
> > > Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
> > > ---
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 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..3e0fef0955ce 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,17 @@ static void drm_mode_to_intf_timing_params(
> > > timing->vsync_polarity = 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/EDP, Shift timings to align it to bottom right */
> > > -	if (phys_enc->hw_intf->cap->type == INTF_DP) {
> > > +	if (phys_enc->hw_intf->cap->type == INTF_DP &&
> > > timing->wide_bus_en) {
> > 
> > This code existed long before widebus: are you sure this is correct?
> > 
> > Note that an identical `if` condtion exists right below, under the
> > "for DP, divide the horizonal parameters by 2 when widebus is
> > enabled" comment.  If this "Shift timings to align it to bottom
> > right" should really only happen when widebus is enabled, move the
> > code into that instead.
> > 
> > - Marijn
> > 
> 
> Happy to condense it. I left it in two sections for clear review at
> this point. As stated above, I reused the wide_bus parameter as the
> SDM845 appears to be the only affected chip.

If you plan on reusing the wide_bus_en feature to "detect" SDM845, such a thing
should be very clearly described in both commit and comment description.  Though
I'm certain such behaviour is buggy, this'll be set to false on other SoCs if
the output format is yuv420 for example.

Without looking at the code too much, you should be able to get access to the
current DPU version through some of these structures which I'd recommend.

At the same time we should analyze _when_ downstream added this exception
for other SoCs, perhaps there's a hint or clearer conditional in one of their
patches or descriptions or code comments?

- Marijn

> > >  		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 2/2] drm/msm/disp: Correct porch timing for SDM845

[James A. MacInnes]

On Wed, 12 Feb 2025 18:15:51 +0100
Marijn Suijten <marijn.suijten@somainline.org> wrote:

> On 2025-02-12 08:23:03, James A. MacInnes wrote:
> > On Wed, 12 Feb 2025 11:13:24 +0100
> > Marijn Suijten <marijn.suijten@somainline.org> wrote:
> >   
> > > On 2025-02-11 19:42:25, James A. MacInnes wrote:  
> > > > Type-C DisplayPort inop due to incorrect settings.
> > > > 
> > > > SDM845 (DPU 4.0) lacks wide_bus support; porch shift removed.  
> > > 
> > > Same comment on "inop", elaborating the meaning of "incorrect
> > > settings" and describing relevance to DPU 4.0 from patch 1/2.
> > >   
> > 
> > Again, happy to use more words.
> >   
> > > > 
> > > > Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver
> > > > support")  
> > > 
> > > This commit came long before wide bus support, are you sure this
> > > is the right Fixes tag?
> > >   
> > 
> > Yes, I went back to the Android 4.9 driver (that was working) and
> > found that the porch shift was not there. After experimenting with
> > removing the porch shift code, I had fully working video. As the
> > SDM845 is the only chip that doesn't use wide_bus, the pair are not
> > related, but each one contributes to no/poor video output.  
> 
> Ack: such information is exactly critical to have in the patch
> description. Looking forward to seeing it in v2 :).  It's not
> something I have been able to deduce from "SDM845 lacks wide_bus
> support; porch shift removed".
> 
> > > >   
> > > 
> > > Drop empty line between tags.
> > >   
> > > > Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
> > > > ---
> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 8
> > > > ++++---- 1 file changed, 4 insertions(+), 4 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..3e0fef0955ce 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,17 @@ static void drm_mode_to_intf_timing_params(
> > > > timing->vsync_polarity = 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/EDP, Shift timings to align it to bottom
> > > > right */
> > > > -	if (phys_enc->hw_intf->cap->type == INTF_DP) {
> > > > +	if (phys_enc->hw_intf->cap->type == INTF_DP &&
> > > > timing->wide_bus_en) {  
> > > 
> > > This code existed long before widebus: are you sure this is
> > > correct?
> > > 
> > > Note that an identical `if` condtion exists right below, under the
> > > "for DP, divide the horizonal parameters by 2 when widebus is
> > > enabled" comment.  If this "Shift timings to align it to bottom
> > > right" should really only happen when widebus is enabled, move the
> > > code into that instead.
> > > 
> > > - Marijn
> > >   
> > 
> > Happy to condense it. I left it in two sections for clear review at
> > this point. As stated above, I reused the wide_bus parameter as the
> > SDM845 appears to be the only affected chip.  
> 
> If you plan on reusing the wide_bus_en feature to "detect" SDM845,
> such a thing should be very clearly described in both commit and
> comment description.  Though I'm certain such behaviour is buggy,
> this'll be set to false on other SoCs if the output format is yuv420
> for example.
> 
> Without looking at the code too much, you should be able to get
> access to the current DPU version through some of these structures
> which I'd recommend.
> 
> At the same time we should analyze _when_ downstream added this
> exception for other SoCs, perhaps there's a hint or clearer
> conditional in one of their patches or descriptions or code comments?
> 
> - Marijn
> 

I will perform my due diligence for this fix. From what I could see in
the file history, this was an arbitrary change that probably worked
fine on all the 5.x.x hardware, but lacking a working type-c port, it
was never tested on the SDM845.

I can also see if this part of the driver has access to the catalog
description or elements within. I would greatly prefer to not create
some new variable that fixes this one bug!

Quick summary: The preference would be to have a specific declared item
that references the SoC instead of re-using the wide_bus_supported
element? 

- James

> > > >  		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 0/2] drm/msm/dp: Fix Type-C Timing

[Dmitry Baryshkov]

On Tue, Feb 11, 2025 at 07:42:23PM -0800, James A. MacInnes wrote:
> SDM845 DisplayPort output inop on DP Monitor and tears on HDMI.
> 
> Fixed
>  - SDM845 does not support wide mode.
>  - SDM845 does not need porch shift.
> 
> Verified functionality on SDM845.

Please use ./scripts/get_maintainer.pl to get the To / Cc lists. Your
messages missed several mailing lists and maybe some of maintainers.

> 
> 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
> 

-- 
With best wishes
Dmitry

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

[James A. MacInnes]

On Wed, 12 Feb 2025 13:20:01 +0200
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On Tue, Feb 11, 2025 at 07:42:23PM -0800, James A. MacInnes wrote:
> > SDM845 DisplayPort output inop on DP Monitor and tears on HDMI.
> > 
> > Fixed
> >  - SDM845 does not support wide mode.
> >  - SDM845 does not need porch shift.
> > 
> > Verified functionality on SDM845.  
> 
> Please use ./scripts/get_maintainer.pl to get the To / Cc lists. Your
> messages missed several mailing lists and maybe some of maintainers.
> 

Will do. The list from get_maintainers.pl was very long and I was
attempting to not spam everyone. On revision I will include everyone.

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

[Marijn Suijten]

On 2025-02-12 08:34:24, James A. MacInnes wrote:
> On Wed, 12 Feb 2025 13:20:01 +0200
> Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> 
> > On Tue, Feb 11, 2025 at 07:42:23PM -0800, James A. MacInnes wrote:
> > > SDM845 DisplayPort output inop on DP Monitor and tears on HDMI.
> > > 
> > > Fixed
> > >  - SDM845 does not support wide mode.
> > >  - SDM845 does not need porch shift.
> > > 
> > > Verified functionality on SDM845.  
> > 
> > Please use ./scripts/get_maintainer.pl to get the To / Cc lists. Your
> > messages missed several mailing lists and maybe some of maintainers.
> > 
> 
> Will do. The list from get_maintainers.pl was very long and I was
> attempting to not spam everyone. On revision I will include everyone.

May I suggest using b4 for patch handling?  It'll help with tracking revisions,
cover letters, To/Cc, running checkpatch andsoforth:

https://b4.docs.kernel.org/en/latest/

(Another patch of you seemed to have V2 tacked on to the end of the cover letter
title; it's instead supposed to be part of the PREFIX like [PATCH v2 1/2] for
every message.  This tool will automatically handle that for you :D)

- Marijn