Re: [Intel-gfx] [PATCH] drm/i915: Fix TV encoder clock computation

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

On Fri, 09 Sep 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The TV encoder has its own special clocking strategy,
> which means we can't just use intel_crtc_dotclock() to
> figure out what the resulting dotclock will be given
> the actual DPLL port_clock. Additionally the DPLL can't
> always generate exactly the frequency we initially asked
> for. This results in us computing a bogus dotclock/etc.,
> and it won't match the readout which is handled by the
> encoder itself properly. Naturally the state checker
> becomes unhappy with the mismatch.
>
> To do this sanely we'll need to move the DPLL computation
> into encoder->compute_config() so that all the derived
> state gets correctly computed based on the actual DPLL
> output frequency. Start doing that just for the TV encoder
> initally as intel_crtc_dotclock() should be able to handle
> other encoder types well enough. Though eventually this
> should be done for all encoder types rather than
> doing it from intel_crtc_compute_config().
>
> With this we actually do some of the DPLL state computation
> twice, but we can skip the second actual .find_dpll() search
> by flagging .clock_set=true after we've done it once. We also
> still need to avoid clobbering the correct
> adjusted_mode.crtc_clock set up by encoder->compute_config()
> when called a second time from intel_crtc_compute_config().
>
> Fixes: 665a7b04092c ("drm/i915: Feed the DPLL output freq back into crtc_state")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I'll take your word for it, and I don't think we're going to find anyone
to review the details here...

Acked-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_dpll.c |  8 ++++++--
>  drivers/gpu/drm/i915/display/intel_tv.c   | 23 +++++++++++++++++------
>  2 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll.c b/drivers/gpu/drm/i915/display/intel_dpll.c
> index 52f2fe1735da..b15ba78d64d6 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll.c
> @@ -1302,7 +1302,9 @@ static int g4x_crtc_compute_clock(struct intel_atomic_state *state,
>  			  &crtc_state->dpll);
>  
>  	crtc_state->port_clock = crtc_state->dpll.dot;
> -	crtc_state->hw.adjusted_mode.crtc_clock = intel_crtc_dotclock(crtc_state);
> +	/* FIXME this is a mess */
> +	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_TVOUT))
> +		crtc_state->hw.adjusted_mode.crtc_clock = intel_crtc_dotclock(crtc_state);
>  
>  	return 0;
>  }
> @@ -1374,7 +1376,9 @@ static int i9xx_crtc_compute_clock(struct intel_atomic_state *state,
>  			  &crtc_state->dpll);
>  
>  	crtc_state->port_clock = crtc_state->dpll.dot;
> -	crtc_state->hw.adjusted_mode.crtc_clock = intel_crtc_dotclock(crtc_state);
> +	/* FIXME this is a mess */
> +	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_TVOUT))
> +		crtc_state->hw.adjusted_mode.crtc_clock = intel_crtc_dotclock(crtc_state);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_tv.c b/drivers/gpu/drm/i915/display/intel_tv.c
> index 9379f3463344..dcf89d701f0f 100644
> --- a/drivers/gpu/drm/i915/display/intel_tv.c
> +++ b/drivers/gpu/drm/i915/display/intel_tv.c
> @@ -39,6 +39,7 @@
>  #include "intel_crtc.h"
>  #include "intel_de.h"
>  #include "intel_display_types.h"
> +#include "intel_dpll.h"
>  #include "intel_hotplug.h"
>  #include "intel_tv.h"
>  
> @@ -982,10 +983,10 @@ intel_tv_mode_vdisplay(const struct tv_mode *tv_mode)
>  
>  static void
>  intel_tv_mode_to_mode(struct drm_display_mode *mode,
> -		      const struct tv_mode *tv_mode)
> +		      const struct tv_mode *tv_mode,
> +		      int clock)
>  {
> -	mode->clock = tv_mode->clock /
> -		(tv_mode->oversample >> !tv_mode->progressive);
> +	mode->clock = clock / (tv_mode->oversample >> !tv_mode->progressive);
>  
>  	/*
>  	 * tv_mode horizontal timings:
> @@ -1143,7 +1144,7 @@ intel_tv_get_config(struct intel_encoder *encoder,
>  	xsize = tmp >> 16;
>  	ysize = tmp & 0xffff;
>  
> -	intel_tv_mode_to_mode(&mode, &tv_mode);
> +	intel_tv_mode_to_mode(&mode, &tv_mode, pipe_config->port_clock);
>  
>  	drm_dbg_kms(&dev_priv->drm, "TV mode: " DRM_MODE_FMT "\n",
>  		    DRM_MODE_ARG(&mode));
> @@ -1184,6 +1185,9 @@ intel_tv_compute_config(struct intel_encoder *encoder,
>  			struct intel_crtc_state *pipe_config,
>  			struct drm_connector_state *conn_state)
>  {
> +	struct intel_atomic_state *state =
> +		to_intel_atomic_state(pipe_config->uapi.state);
> +	struct intel_crtc *crtc = to_intel_crtc(pipe_config->uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_tv_connector_state *tv_conn_state =
>  		to_intel_tv_connector_state(conn_state);
> @@ -1192,6 +1196,7 @@ intel_tv_compute_config(struct intel_encoder *encoder,
>  		&pipe_config->hw.adjusted_mode;
>  	int hdisplay = adjusted_mode->crtc_hdisplay;
>  	int vdisplay = adjusted_mode->crtc_vdisplay;
> +	int ret;
>  
>  	if (!tv_mode)
>  		return -EINVAL;
> @@ -1206,7 +1211,13 @@ intel_tv_compute_config(struct intel_encoder *encoder,
>  
>  	pipe_config->port_clock = tv_mode->clock;
>  
> -	intel_tv_mode_to_mode(adjusted_mode, tv_mode);
> +	ret = intel_dpll_crtc_compute_clock(state, crtc);
> +	if (ret)
> +		return ret;
> +
> +	pipe_config->clock_set = true;
> +
> +	intel_tv_mode_to_mode(adjusted_mode, tv_mode, pipe_config->port_clock);
>  	drm_mode_set_crtcinfo(adjusted_mode, 0);
>  
>  	if (intel_tv_source_too_wide(dev_priv, hdisplay) ||
> @@ -1804,7 +1815,7 @@ intel_tv_get_modes(struct drm_connector *connector)
>  		 * about the actual timings of the mode. We
>  		 * do ignore the margins though.
>  		 */
> -		intel_tv_mode_to_mode(mode, tv_mode);
> +		intel_tv_mode_to_mode(mode, tv_mode, tv_mode->clock);
>  		if (count == 0) {
>  			drm_dbg_kms(&dev_priv->drm, "TV mode: " DRM_MODE_FMT "\n",
>  				    DRM_MODE_ARG(mode));

-- 
Jani Nikula, Intel Open Source Graphics Center