Message-ID: <20250212115658.34c89705@jamesmacinnes-VirtualBox>
Date: Wed, 12 Feb 2025 11:56:58 -0800
From: "James A. MacInnes" <james.a.macinnes@...il.com>
To: Marijn Suijten <marijn.suijten@...ainline.org>
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 Wed, 12 Feb 2025 18:15:51 +0100
Marijn Suijten <marijn.suijten@...ainline.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@...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
> 

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

Powered by blists - more mailing lists