On Wed, 12 Feb 2025 18:15:51 +0100
Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > > > ---
> > > > 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
> > > >
> >
