Re: [PATCH] drm/i915/ddi: Guard reg_val against a INVALID_TRANSCODER

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 04 Sep 2025, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
> Currently we check if the encoder is INVALID or -1 and throw a
> WARN_ON but we still end up writing the temp value which will
> overflow and corrupt the whole programmed value.
>
> Fixes: 6671c367a9bea ("drm/i915/tgl: Select master transcoder for MST stream")
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 4e4ea3a0ff83..0a58c07a8240 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -596,9 +596,9 @@ intel_ddi_transcoder_func_reg_val_get(struct intel_encoder *encoder,
>  			enum transcoder master;
>  
>  			master = crtc_state->mst_master_transcoder;
> -			drm_WARN_ON(display->drm,
> -				    master == INVALID_TRANSCODER);
> -			temp |= TRANS_DDI_MST_TRANSPORT_SELECT(master);
> +			if (!drm_WARN_ON(display->drm,
> +					 master == INVALID_TRANSCODER))
> +				temp |= TRANS_DDI_MST_TRANSPORT_SELECT(master);

I really *really* hate hiding happy day scenario paths within if
statements with stuff that should never happen.

Frankly I'd rather see

        if (drm_WARN_ON(display->drm, master == INVALID_TRANSCODER))
		master = TRANSCODER_A; /* bogus */

because it doesn't matter, we're hosed anyway, and this shouldn't happen
in the first place.

The code should be easy to read for the happy day scenario, and it
should be quick to mentally ignore checks like this. The !drm_WARN_ON()
is not it.


BR,
Jani.


>  		}
>  	} else {
>  		temp |= TRANS_DDI_MODE_SELECT_DP_SST;

-- 
Jani Nikula, Intel