Re: [v1 2/3] drm/msm/disp/dpu1: add dspps into reservation if there is a ctm request

[Dmitry Baryshkov]

On 01/02/2023 13:16, Marijn Suijten wrote:
> On 2023-01-30 07:21:31, Kalyan Thota wrote:
> > Add dspp blocks into the topology for reservation, if there is a ctm
> > request for that composition.
>
> DSPP
>
> > Changes in v1:
> > - Minor nits (Dmitry)
>
> This should go below the triple dashes, so that it /does not/ become
> part of the patch/commit that is applied to the tree (where review
> history is irrelevant as it can be searched for separately).

This is one of DRM peculiarities which we have to live with.

> > Signed-off-by: Kalyan Thota <quic_kalyant@xxxxxxxxxxx>
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 ++++++-------
> >   1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 9c6817b..3bd46b4 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
> >   static struct msm_display_topology dpu_encoder_get_topology(
> >   			struct dpu_encoder_virt *dpu_enc,
> >   			struct dpu_kms *dpu_kms,
> > -			struct drm_display_mode *mode)
> > +			struct drm_display_mode *mode,
> > +			struct drm_crtc_state *crtc_state)
> >   {
> >   	struct msm_display_topology topology = {0};
> >   	int i, intf_count = 0;
> > @@ -573,11 +574,9 @@ static struct msm_display_topology dpu_encoder_get_topology(
> >   	else
> >   		topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
> >   
> > -	if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
> > -		if (dpu_kms->catalog->dspp &&
> > -			(dpu_kms->catalog->dspp_count >= topology.num_lm))
> > -			topology.num_dspp = topology.num_lm;
> > -	}
> > +	if (dpu_kms->catalog->dspp &&
> > +	    crtc_state->ctm && (dpu_kms->catalog->dspp_count >= topology.num_lm))
>
> Multiline-if-clause is typically indented with two tabs, not a half tab
> (4 spaces).

I tend to disagree here. Lately I have mostly seen it being indented to 
the opening parenthesis, so that nested statements also indent nicely.

> Nit: swap the && here?  dspp and dspp_count are related, so check ctm
> first or last but not in the middle - makes reading easier.

I think we can ignore dpu_kms->catalog->dspp completely. checking 
dspp_count should be enough for the purpose of the check (and note, the 
check for dspp/dspp_count is misleading and should be omitted).

> > +		topology.num_dspp = topology.num_lm;
> >   
> >   	topology.num_enc = 0;
> >   	topology.num_intf = intf_count;
> > @@ -643,7 +642,7 @@ static int dpu_encoder_virt_atomic_check(
> >   		}
> >   	}
> >   
> > -	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
> > +	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state);
> >   
> >   	/* Reserve dynamic resources now. */
> >   	if (!ret) {
> > --
> > 2.7.4

--
With best wishes
Dmitry