| Message ID | 20250212034225.2565069-3-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: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 >
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 > >
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 > > > >
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 > > > > > >
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
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(-)