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

[James A. MacInnes]

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.

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

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