| Message ID | 20250212034225.2565069-2-james.a.macinnes@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm/dp: Fix Type-C Timing | expand |
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 >
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 > >
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
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 },
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(-)