[Intel-gfx] [PATCH 4/4] drm/i915: move infoframe setting to after port enable

[Paulo Zanoni]

2014-04-02 14:08 GMT-03:00 Jesse Barnes <jbarnes at virtuousgeek.org>:
> Needs to happen after clock is running or it doesn't behave correctly.
>
> Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index fb9839b..a4ca63b6 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -669,8 +669,6 @@ static void intel_hdmi_mode_set(struct intel_encoder *encoder)
>
>         I915_WRITE(intel_hdmi->hdmi_reg, hdmi_val);
>         POSTING_READ(intel_hdmi->hdmi_reg);
> -
> -       intel_hdmi->set_infoframes(&encoder->base, adjusted_mode);
>  }
>
>  static bool intel_hdmi_get_hw_state(struct intel_encoder *encoder,
> @@ -738,9 +736,13 @@ static void intel_enable_hdmi(struct intel_encoder *encoder)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>         struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> +       struct drm_display_mode *adjusted_mode =
> +               &intel_crtc->config.adjusted_mode;
>         u32 temp;
>         u32 enable_bits = SDVO_ENABLE;
>
> +       intel_hdmi->set_infoframes(&encoder->base, adjusted_mode);
> +
>         if (intel_hdmi->has_audio)
>                 enable_bits |= SDVO_AUDIO_ENABLE;
>

Due to my past, I am not a person who should be reviewing these
patches because I have a tendency to fear that a single variable
rename will break everybody's machines at this point of the code :)

My problem with this patch is that now we do the same thing at 3
different points depending on the platform:
- For VLV, we will enable infoframes at encoder->pre_enable
- For ILK and friends, we will enable infoframes at encoder->enable
- For HSW+, we will still enable infoframes at encoder->modeset

I'd really like to have a standard behavior here :)

Also, for ILK/SNB/IVB, we run encoder->enable after the very end of
the modeset sequence, and I suspect the pipe is already running at
that point (since the we already called intel_enable_pipe, we already
ran intel_enable_planes, and we also called ironlake_pch_enable). For
that case, we probably need to implement all those "wait for the exact
VSYNC moment before touching register" restrictions that are described
on the spec. Or we find a way to also call set_infoframes at
pre_enable on these platforms. Did we test these patches on the ILK+
family?

I also remember a lot of bugs that would only be seen after
suspend/resume, so I suggest testing it too :)

The good thing is that moving register writes away from the mode_set
callbacks is good IMHO since at some point we'll want crtc_enable to
fully setup the HW, so runtime PM will be able to work without
requiring a crtc_mode_set call. But you need to patch HSW+ too for
that :P

This is not an ack, nack nor a reviewed-by :) If you are still
confident, feel free to go ahead.

Thanks,
Paulo

> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni