Re: [PATCH] drm/i915/display: Check some INVALID_TRANSCODER cases

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Apr 09, 2026 at 09:00:02PM +0000, Jonathan Cavitt wrote:
> There are some cases in intel_ddi.c, such as in intel_ddi_is_audio_enabled
> and intel_ddi_compute_config_late, where we attempt to perform a BIT
> shift using a passed transcoder enum value.  This value may be -1,
> INVALID_TRANSCODER, which can result in undefined behavior if this
> occurs.
> 
> In the former case, we can simply return false if this is the transcoder
> passed (as audio is not enabled on an invalid transcoder).
> 
> In the latter case, the likely expected behavior is to set the
> crtc_state->sync_mode_slaves_mask to zero, so just do that directly and
> avoid a risky bit shift.
> 
> The likelihood of either case occurring during normal execution is
> unknown and possibly very low.  Regardless, this covers a static analyis
> issue.
> 
> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 178074316a2c..7abeb60779e5 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3875,7 +3875,8 @@ static void intel_ddi_set_idle_link_train(struct intel_dp *intel_dp,
>  static bool intel_ddi_is_audio_enabled(struct intel_display *display,
>  				       enum transcoder cpu_transcoder)
>  {
> -	if (cpu_transcoder == TRANSCODER_EDP)
> +	if (cpu_transcoder == TRANSCODER_EDP ||
> +	    cpu_transcoder == INVALID_TRANSCODER)
>  		return false;
>  
>  	if (!intel_display_power_is_enabled(display, POWER_DOMAIN_AUDIO_MMIO))
> @@ -4637,8 +4638,13 @@ static int intel_ddi_compute_config_late(struct intel_encoder *encoder,
>  
>  	if (crtc_state->master_transcoder == crtc_state->cpu_transcoder) {
>  		crtc_state->master_transcoder = INVALID_TRANSCODER;
> -		crtc_state->sync_mode_slaves_mask =
> -			port_sync_transcoders & ~BIT(crtc_state->cpu_transcoder);
> +		if (crtc_state->cpu_transcoder == INVALID_TRANSCODER ||
> +		    !port_sync_transcoders)
> +			crtc_state->sync_mode_slaves_mask = 0;
> +		else
> +
> +			crtc_state->sync_mode_slaves_mask =
> +				port_sync_transcoders & ~BIT(crtc_state->cpu_transcoder);

Making the code convoluted just to satisfy some dumb static
analysis tool is not great.

If we really need to care about this crap then I think it'd be better
to introduce some kind of intel_transcoder_mask() function that
explicitly deals with INVALID_TRANSCODER.

>  	}
>  
>  	return 0;
> -- 
> 2.43.0

-- 
Ville Syrjälä
Intel