Re: [PATCH v3 26/26] drm/i915/writeback: Modify state verify function

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

On Wed, Mar 25, 2026 at 04:37:44PM +0530, Suraj Kandpal wrote:
> Modify the state verify functions to take into account the fact
> that writeback does not need all the timings for it to be set.
> Moreover there is no need for dpll state nor do we need to set
> any sort of flags for it.
> 
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 57 ++++++++++++--------
>  1 file changed, 35 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index e47b4e667fec..59b6c61890bc 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -5140,6 +5140,8 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  	struct drm_printer p;
>  	u32 exclude_infoframes = 0;
>  	bool ret = true;
> +	bool is_writeback =
> +		intel_crtc_has_type(current_config, INTEL_OUTPUT_WRITEBACK);
>  
>  	if (fastset)
>  		p = drm_dbg_printer(display->drm, DRM_UT_KMS, NULL);
> @@ -5245,20 +5247,25 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  } while (0)
>  
>  #define PIPE_CONF_CHECK_TIMINGS(name) do {     \
> -	PIPE_CONF_CHECK_I(name.crtc_hdisplay); \
> -	PIPE_CONF_CHECK_I(name.crtc_htotal); \
> -	PIPE_CONF_CHECK_I(name.crtc_hblank_start); \
> -	PIPE_CONF_CHECK_I(name.crtc_hblank_end); \
> -	PIPE_CONF_CHECK_I(name.crtc_hsync_start); \
> -	PIPE_CONF_CHECK_I(name.crtc_hsync_end); \
> -	PIPE_CONF_CHECK_I(name.crtc_vdisplay); \
> -	if (!fastset || !allow_vblank_delay_fastset(current_config)) \
> -		PIPE_CONF_CHECK_I(name.crtc_vblank_start); \
> -	PIPE_CONF_CHECK_I(name.crtc_vsync_start); \
> -	PIPE_CONF_CHECK_I(name.crtc_vsync_end); \
> -	if (!fastset || !pipe_config->update_lrr) { \
> -		PIPE_CONF_CHECK_I(name.crtc_vtotal); \
> -		PIPE_CONF_CHECK_I(name.crtc_vblank_end); \
> +	if (is_writeback) { \
> +		PIPE_CONF_CHECK_I(name.crtc_hdisplay); \
> +		PIPE_CONF_CHECK_I(name.crtc_vdisplay); \
> +	} else { \
> +		PIPE_CONF_CHECK_I(name.crtc_hdisplay); \
> +		PIPE_CONF_CHECK_I(name.crtc_htotal); \
> +		PIPE_CONF_CHECK_I(name.crtc_hblank_start); \
> +		PIPE_CONF_CHECK_I(name.crtc_hblank_end); \
> +		PIPE_CONF_CHECK_I(name.crtc_hsync_start); \
> +		PIPE_CONF_CHECK_I(name.crtc_hsync_end); \
> +		PIPE_CONF_CHECK_I(name.crtc_vdisplay); \
> +		if (!fastset || !allow_vblank_delay_fastset(current_config)) \
> +			PIPE_CONF_CHECK_I(name.crtc_vblank_start); \
> +		PIPE_CONF_CHECK_I(name.crtc_vsync_start); \
> +		PIPE_CONF_CHECK_I(name.crtc_vsync_end); \
> +		if (!fastset || !pipe_config->update_lrr) { \
> +			PIPE_CONF_CHECK_I(name.crtc_vtotal); \
> +			PIPE_CONF_CHECK_I(name.crtc_vblank_end); \
> +		} \

This is ugly. I think a much better option is to make sure the
writeback stuff actually does proper compute/readout for all
of this.

>  	} \
>  } while (0)
>  
> @@ -5387,10 +5394,11 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  
>  	PIPE_CONF_CHECK_I(pixel_multiplier);
>  
> -	PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags,
> -			      DRM_MODE_FLAG_INTERLACE);
> +	if (!is_writeback)
> +		PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags,
> +				      DRM_MODE_FLAG_INTERLACE);
>  
> -	if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS)) {
> +	if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS) && !is_writeback) {
>  		PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags,
>  				      DRM_MODE_FLAG_PHSYNC);
>  		PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags,
> @@ -5441,6 +5449,8 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  		PIPE_CONF_CHECK_BOOL(hw.casf_params.casf_enable);
>  		PIPE_CONF_CHECK_I(hw.casf_params.win_size);
>  		PIPE_CONF_CHECK_I(hw.casf_params.strength);
> +		if (!is_writeback)
> +			PIPE_CONF_CHECK_I(pixel_rate);
>  
>  		PIPE_CONF_CHECK_X(gamma_mode);
>  		if (display->platform.cherryview)
> @@ -5463,24 +5473,27 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  
>  	PIPE_CONF_CHECK_BOOL(double_wide);
>  
> -	if (display->dpll.mgr)
> +	if (display->dpll.mgr && !is_writeback)
>  		PIPE_CONF_CHECK_P(intel_dpll);
>  
>  	/* FIXME convert everything over the dpll_mgr */
> -	if (display->dpll.mgr || HAS_GMCH(display))
> +	if ((display->dpll.mgr || HAS_GMCH(display)) && !is_writeback)
>  		PIPE_CONF_CHECK_PLL(dpll_hw_state);
>  
>  	PIPE_CONF_CHECK_X(dsi_pll.ctrl);
>  	PIPE_CONF_CHECK_X(dsi_pll.div);
>  
> -	if (display->platform.g4x || DISPLAY_VER(display) >= 5)
> +	if ((display->platform.g4x || DISPLAY_VER(display) >= 5) &&
> +	    !is_writeback)
>  		PIPE_CONF_CHECK_I(pipe_bpp);
>  
> -	if (!fastset || !pipe_config->update_m_n) {
> +	if ((!fastset || !pipe_config->update_m_n) && !is_writeback) {
>  		PIPE_CONF_CHECK_I(hw.pipe_mode.crtc_clock);
>  		PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_clock);
>  	}
> -	PIPE_CONF_CHECK_I(port_clock);
> +
> +	if (!is_writeback)
> +		PIPE_CONF_CHECK_I(port_clock);
>  
>  	PIPE_CONF_CHECK_I(min_voltage_level);
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel