[Intel-gfx] [PATCH] drm/i915/sdvo: Shut up state checker with hdmi cards on gen3

[Chris Wilson]

Quoting Daniel Vetter (2017-07-26 18:52:06)
> On Wed, Jul 26, 2017 at 4:33 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> > Quoting Daniel Vetter (2017-07-25 14:48:41)
> >> The hdmi bits simply don't exist, so nerf them. I think audio doesn't
> >> work on gen3 at all, and for the limited color range we should
> >> probably use the colorimetry sdvo paramater instead of the bit in the
> >> port.
> >>
> >> But fixing sdvo isn't my goal, I just want to get the backtrace out of
> >> the way, and this takes care of that.
> >>
> >> Still, while at it fix the missing read-out of the gen4 audio bit,
> >> maybe that part even works ...
> >>
> >> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_sdvo.c | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> >> index bea8152ae859..0403e30dfabc 100644
> >> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> >> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> >> @@ -1113,6 +1113,7 @@ static bool intel_sdvo_compute_config(struct intel_encoder *encoder,
> >>                                       struct intel_crtc_state *pipe_config,
> >>                                       struct drm_connector_state *conn_state)
> >>  {
> >> +       struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >>         struct intel_sdvo *intel_sdvo = to_sdvo(encoder);
> >>         struct intel_sdvo_connector_state *intel_sdvo_state =
> >>                 to_intel_sdvo_connector_state(conn_state);
> >> @@ -1174,6 +1175,12 @@ static bool intel_sdvo_compute_config(struct intel_encoder *encoder,
> >>                         pipe_config->limited_color_range = true;
> >>         }
> >>
> >> +       /* gen3 doesn't do the hdmi bits in the SDVO register */
> >> +       if (INTEL_GEN(dev_priv) < 4) {
> >> +               pipe_config->limited_color_range = false;
> >> +               pipe_config->has_audio = false;
> >> +       }
> >
> > So historically, this would be
> >
> >         if (!intel_sdvo->is_hdmi) {
> >                 pipe_config->limited_color_range = false;
> >                 pipe_config->has_audio = false;
> >         }
> >
> > Currently we set
> >
> >         pipe_config->has_audio =
> >                 intel_sdvo_state->base.force_audio == HDMI_AUDIO_ON ||
> >                 (intel_sdvo_state->base.force_audio == HDMI_AUDIO_AUTO && intel_sdvo->has_hdmi_audio))
> >
> > and, with a slight paraphrase,
> >
> >         pipe_config->limited_color_range = pipe_config->has_hdmi_sink;
> >
> > So, both of these should only be set if we have hdmi features which gen3
> > will not. So I think the consistent fix is
> >
> >         switch (intel_sdvo_state->base.force_audio) {
> >         case HDMI_AUDIO_ON:
> >                 pipe_config->has_audio = intel_sdvo->is_hdmi;
> >                 break;
> >         case HDMI_AUDIO_AUTO:
> >                 pipe_config->has_audio = intel_sdvo->has_hdmi_audio;
> >                 break;
> >         case HDMI_AUDIO_OFF:
> >         case HDMI_AUDIO_OFF_DVI:
> >                 pipe_config->has_audio = false;
> >                 break;
> >         }
> >
> > Though I'm not sure what the actual subtlety of OFF_DVI is.
> 
> is_hdmi is set when the SDVO transcoder supports hdmi. And you can
> plug a hdmi sdvo chip into a gen3 box, which is why this blows up
> here. I guess I could try to limit that, that should take care of
> things complete. Together with making sure we don't set any of the
> hdmi state bits if the is_hdmi is false (which I hope we already do).

Hmm, I was assuming that the is_hdmi query returned false for gen3.
I think having this flow from HDMI state is nicer (and so fixing up the
query), and then something like

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index e58a47db9a9d..3883cc6834a0 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1349,8 +1349,10 @@ static void intel_sdvo_pre_enable(struct intel_encoder *intel_encoder,
        else
                sdvox |= SDVO_PIPE_SEL(crtc->pipe);
 
-       if (crtc_state->has_audio)
+       if (crtc_state->has_audio) {
+               WARN_ON_ONCE(INTEL_GEN(dev_priv) < 4);
                sdvox |= SDVO_AUDIO_ENABLE;
+       }
 
        if (INTEL_GEN(dev_priv) >= 4) {
                /* done in crtc_mode_set as the dpll_md reg must be written early */

to document that this bit didn't exist before.
-Chris