linux-kernel - Re: [PATCH 2/2] drm/msm/disp: Correct porch timing for SDM845

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ythh7pwdr4g6ih5phkhmsmkpghigfrxieka4lkcqivckvw77j3@jscdxwdloqus>
Date: Wed, 12 Feb 2025 18:15:51 +0100
From: Marijn Suijten <marijn.suijten@...ainline.org>
To: "James A. MacInnes" <james.a.macinnes@...il.com>
Cc: linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	robdclark@...il.com, quic_abhinavk@...cinc.com, dmitry.baryshkov@...aro.org, 
	sean@...rly.run, airlied@...il.com, simona@...ll.ch
Subject: Re: [PATCH 2/2] drm/msm/disp: Correct porch timing for SDM845

On 2025-02-12 08:23:03, James A. MacInnes wrote:
> On Wed, 12 Feb 2025 11:13:24 +0100
> Marijn Suijten <marijn.suijten@...ainline.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@...il.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
> > > 
>