Re: [PATCH 14/50] drm/i915/dp: Factor out align_max_sink_dsc_input_bpp()

[Imre Deak <imre.deak@intel.com>]

On Mon, Dec 15, 2025 at 02:02:07PM +0200, Luca Coelho wrote:
> On Mon, 2025-12-15 at 13:53 +0200, Imre Deak wrote:
> > On Mon, Dec 15, 2025 at 09:46:24AM +0200, Luca Coelho wrote:
> > > On Thu, 2025-11-27 at 19:49 +0200, Imre Deak wrote:
> > > > Factor out align_max_sink_dsc_input_bpp(), also used later for computing
> > > > the maximum DSC input BPP limit.
> > > > 
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_dp.c | 28 ++++++++++++++++---------
> > > >  1 file changed, 18 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > index 000fccc39a292..dcb9bc11e677b 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > @@ -1893,12 +1893,27 @@ int intel_dp_dsc_max_src_input_bpc(struct intel_display *display)
> > > >  	return intel_dp_dsc_min_src_input_bpc();
> > > >  }
> > > >  
> > > > +static int align_max_sink_dsc_input_bpp(const struct intel_connector *connector,
> > > > +					int max_pipe_bpp)
> > > > +{
> > > > +	u8 dsc_bpc[3];
> > > 
> > > I think it's safer to use the '= {}' we had before, because that zeroes
> > > the array, so in case of any stack leaks, you won't leak aleatory parts
> > > of the memory.  In this case it's only 3 bytes, so hardly anything
> > > important could leak, but anyway.
> > 
> > As for any other variable I don't see any reason for initializing it, if
> > it will be initialized before its first use. It will be initialized
> > before its first use by drm_dp_dsc_sink_supported_input_bpcs().
> 
> Fair enough.  Security here is probably not so important, and as I
> said, it's only 3 bytes, but in wifi we once had the activity of pre-
> initializing all arrays like this for security reasons.  Your call.

I don't see how it is more secure. I think any valid reason to zero out
variables on the stack for security reasons would need to be a guideline
explained and mandated in the whole kernel ubiquitously and should not
be considered as an opt-in practice. I'm not aware of such a guideline.

> > > Also, since this is 3 bytes long, it's theoretically better to have it
> > > at the end of the stack declarations.
> > 
> > The compiler is free to reorder the allocation order on the stack and
> > is expected to that for optimal alignment.
> 
> Of course the compiler will do this sort of things, but it's just
> better practice IMHO to keeps organized in some way.  If you had said
> that it was in alphabetical order (it isn't), then it would probably
> satisfy my OCD. lol

The ordering rule I follow is the readability of declarations, which is
better if it's in decreasing line length order.

> In any case, these were just nitpicks, so it's up to you.
> 
> Reviewed-by: Luca Coelho <luciano.coelho@intel.com>
> 
> --
> Cheers,
> Luca.
> 
> 
> 
> > > > +	int num_bpc;
> > > > +	int i;
> > > > +
> > > > +	num_bpc = drm_dp_dsc_sink_supported_input_bpcs(connector->dp.dsc_dpcd,
> > > > +						       dsc_bpc);
> > > > +	for (i = 0; i < num_bpc; i++) {
> > > > +		if (dsc_bpc[i] * 3 <= max_pipe_bpp)
> > > > +			return dsc_bpc[i] * 3;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  int intel_dp_dsc_compute_max_bpp(const struct intel_connector *connector,
> > > >  				 u8 max_req_bpc)
> > > >  {
> > > >  	struct intel_display *display = to_intel_display(connector);
> > > > -	int i, num_bpc;
> > > > -	u8 dsc_bpc[3] = {};
> > > >  	int dsc_max_bpc;
> > > >  
> > > >  	dsc_max_bpc = intel_dp_dsc_max_src_input_bpc(display);
> > > > @@ -1908,14 +1923,7 @@ int intel_dp_dsc_compute_max_bpp(const struct intel_connector *connector,
> > > >  
> > > >  	dsc_max_bpc = min(dsc_max_bpc, max_req_bpc);
> > > >  
> > > > -	num_bpc = drm_dp_dsc_sink_supported_input_bpcs(connector->dp.dsc_dpcd,
> > > > -						       dsc_bpc);
> > > > -	for (i = 0; i < num_bpc; i++) {
> > > > -		if (dsc_max_bpc >= dsc_bpc[i])
> > > > -			return dsc_bpc[i] * 3;
> > > > -	}
> > > > -
> > > > -	return 0;
> > > > +	return align_max_sink_dsc_input_bpp(connector, dsc_max_bpc * 3);
> > > >  }
> > > >  
> > > >  static int intel_dp_source_dsc_version_minor(struct intel_display *display)