On Fri, 09 Sep 2022, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
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@xxxxxxxxx>
> ---
> 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
