Re: [PATCH 09/15] drm/i915/pfit: Populate crtc_state->pfit.dst on GMCH platforms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/i915/pfit: uninitialized hscale and vscale in i965_pfit_get_config()
--

commit 1baf5f8a446b17a6276479fa9824e29481b05caf
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>

drm/i915/pfit: Populate crtc_state->pfit.dst on GMCH platforms

This commit populates crtc_state->pfit.dst on GMCH platforms by computing
the destination window coordinates from the horizontal and vertical panel
fitter scaling factors during both config computation and hardware readout.

> diff --git a/drivers/gpu/drm/i915/display/intel_pfit.c b/drivers/gpu/drm/i915/display/intel_pfit.c
> index 5d519f45adbec..63e5a656a33b8 100644
> --- a/drivers/gpu/drm/i915/display/intel_pfit.c
> +++ b/drivers/gpu/drm/i915/display/intel_pfit.c
[ ... ]
> @@ -716,19 +799,39 @@ static void i965_pfit_get_config(struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_display *display = to_intel_display(crtc_state);
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	u32 hscale, vscale, control, ratios;
>  	enum pipe pipe;
> -	u32 tmp;
>  
> -	tmp = intel_de_read(display, PFIT_CONTROL(display));
> -	if (!(tmp & PFIT_ENABLE))
> +	control = intel_de_read(display, PFIT_CONTROL(display));
> +	if (!(control & PFIT_ENABLE))
>  		return;
>  
>  	/* Check whether the pfit is attached to our pipe. */
> -	pipe = REG_FIELD_GET(PFIT_PIPE_MASK, tmp);
> +	pipe = REG_FIELD_GET(PFIT_PIPE_MASK, control);
>  	if (pipe != crtc->pipe)
>  		return;
>  
> -	crtc_state->gmch_pfit.control = tmp;
> +	crtc_state->gmch_pfit.control = control;
> +
> +	ratios = intel_de_read(display, PFIT_PGM_RATIOS(display));
> +
> +	switch (control & PFIT_SCALING_MASK) {
> +	case PFIT_SCALING_LETTER:
> +		hscale = REG_FIELD_GET(PFIT_HORIZ_SCALE_MASK_965, ratios);
> +		vscale = hscale; /* PFIT_VERT_SCALE_MASK_965 contains garbage */
> +		break;
> +	case PFIT_SCALING_PILLAR:
> +		vscale = REG_FIELD_GET(PFIT_VERT_SCALE_MASK_965, ratios);
> +		hscale = vscale; /* PFIT_HORIZ_SCALE_MASK_965 contains garbage */
> +		break;
> +	case PFIT_SCALING_AUTO:
> +	case PFIT_SCALING_PROGRAMMED:
> +		hscale = REG_FIELD_GET(PFIT_HORIZ_SCALE_MASK_965, ratios);
> +		vscale = REG_FIELD_GET(PFIT_VERT_SCALE_MASK_965, ratios);
> +		break;
> +	}
> +
> +	i9xx_compute_pfit_dst(crtc_state, hscale, vscale);
>  }

[Severity: Medium]
Can hscale and vscale be used uninitialized in i965_pfit_get_config()?

In drivers/gpu/drm/i915/display/intel_pfit_regs.h, PFIT_SCALING_MASK is a
3-bit mask with 8 possible values, while only 4 constants (0..3) are
defined:

#define   PFIT_SCALING_MASK		REG_GENMASK(28, 26) /* 965+ */
#define   PFIT_SCALING_AUTO		REG_FIELD_PREP(PFIT_SCALING_MASK, 0)
#define   PFIT_SCALING_PROGRAMMED	REG_FIELD_PREP(PFIT_SCALING_MASK, 1)
#define   PFIT_SCALING_PILLAR		REG_FIELD_PREP(PFIT_SCALING_MASK, 2)
#define   PFIT_SCALING_LETTER		REG_FIELD_PREP(PFIT_SCALING_MASK, 3)

Since the switch in intel_pfit_get_config() -> i965_pfit_get_config() has no
default case and hscale and vscale are not initialized at declaration, could
a reserved value in PFIT_CONTROL pass uninitialized stack values into
i9xx_compute_pfit_dst() (and trigger compiler uninitialized-variable
warnings)?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135309.23578-1-ville.syrjala@linux.intel.com?part=9