Re: [PATCH v3 3/4] drm/i915: Clean up DDI DDC/AUX CH sanitation

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

On Fri, Oct 21, 2016 at 12:00:56AM +0200, Maarten Maathuis wrote:
> I meant DON't suspect
> 
> On Thu, Oct 20, 2016 at 11:53 PM, Maarten Maathuis <madman2003@gmail.com>
> wrote:
> 
> > Also tested v3 on top of 4.8.3 (mainline git is a mess right now for
> > booting).
> >
> > I did encounter a seemingly unrelated message during boot (including a
> > WARN_ON):
> > [drm:skylake_pfit_enable [i915]] *ERROR* Requesting pfit without getting a
> > scaler first
> >
> > I suspect any causal relation with these patches.

Yeah shouldn't be related.

I've pushed the patches to dinq. Thanks for testing and reviews.

> >
> > On Mon, Oct 17, 2016 at 8:07 PM, <ville.syrjala@linux.intel.com> wrote:
> >
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> Now that we use the AUX and GMBUS assignment from VBT for all ports,
> >> let's clean up the sanitization of the port information a bit.
> >> Previosuly we only did this for port E, and only complained about a
> >> non-standard assignment for the other ports. But as we know that
> >> non-standard assignments are a fact of life, let's expand the
> >> sanitization to all the ports.
> >>
> >> v2: Include a commit message, fix up the comments a bit
> >> v3: Don't clobber other ports if the current port has no alternate aux
> >> ch/ddc pin
> >>
> >> Cc: stable@vger.kernel.org
> >> Cc: Maarten Maathuis <madman2003@gmail.com>
> >> Tested-by: Maarten Maathuis <madman2003@gmail.com>
> >> References: https://bugs.freedesktop.org/show_bug.cgi?id=97877
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Link: http://patchwork.freedesktop.org/patch/msgid/1476208368-5710
> >> -4-git-send-email-ville.syrjala@linux.intel.com
> >> Reviewed-by: Jim Bride <jim.bride@linux.intel.com> (v2)
> >> ---
> >>  drivers/gpu/drm/i915/intel_bios.c | 122 ++++++++++++++++++++++++------
> >> --------
> >>  1 file changed, 77 insertions(+), 45 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_bios.c
> >> b/drivers/gpu/drm/i915/intel_bios.c
> >> index 83667e8cdd6b..a8ff8c099685 100644
> >> --- a/drivers/gpu/drm/i915/intel_bios.c
> >> +++ b/drivers/gpu/drm/i915/intel_bios.c
> >> @@ -1035,6 +1035,77 @@ static u8 translate_iboost(u8 val)
> >>         return mapping[val];
> >>  }
> >>
> >> +static void sanitize_ddc_pin(struct drm_i915_private *dev_priv,
> >> +                            enum port port)
> >> +{
> >> +       const struct ddi_vbt_port_info *info =
> >> +               &dev_priv->vbt.ddi_port_info[port];
> >> +       enum port p;
> >> +
> >> +       if (!info->alternate_ddc_pin)
> >> +               return;
> >> +
> >> +       for_each_port_masked(p, (1 << port) - 1) {
> >> +               struct ddi_vbt_port_info *i =
> >> &dev_priv->vbt.ddi_port_info[p];
> >> +
> >> +               if (info->alternate_ddc_pin != i->alternate_ddc_pin)
> >> +                       continue;
> >> +
> >> +               DRM_DEBUG_KMS("port %c trying to use the same DDC pin
> >> (0x%x) as port %c, "
> >> +                             "disabling port %c DVI/HDMI support\n",
> >> +                             port_name(p), i->alternate_ddc_pin,
> >> +                             port_name(port), port_name(p));
> >> +
> >> +               /*
> >> +                * If we have multiple ports supposedly sharing the
> >> +                * pin, then dvi/hdmi couldn't exist on the shared
> >> +                * port. Otherwise they share the same ddc bin and
> >> +                * system couldn't communicate with them separately.
> >> +                *
> >> +                * Due to parsing the ports in alphabetical order,
> >> +                * a higher port will always clobber a lower one.
> >> +                */
> >> +               i->supports_dvi = false;
> >> +               i->supports_hdmi = false;
> >> +               i->alternate_ddc_pin = 0;
> >> +       }
> >> +}
> >> +
> >> +static void sanitize_aux_ch(struct drm_i915_private *dev_priv,
> >> +                           enum port port)
> >> +{
> >> +       const struct ddi_vbt_port_info *info =
> >> +               &dev_priv->vbt.ddi_port_info[port];
> >> +       enum port p;
> >> +
> >> +       if (!info->alternate_aux_channel)
> >> +               return;
> >> +
> >> +       for_each_port_masked(p, (1 << port) - 1) {
> >> +               struct ddi_vbt_port_info *i =
> >> &dev_priv->vbt.ddi_port_info[p];
> >> +
> >> +               if (info->alternate_aux_channel !=
> >> i->alternate_aux_channel)
> >> +                       continue;
> >> +
> >> +               DRM_DEBUG_KMS("port %c trying to use the same AUX CH
> >> (0x%x) as port %c, "
> >> +                             "disabling port %c DP support\n",
> >> +                             port_name(p), i->alternate_aux_channel,
> >> +                             port_name(port), port_name(p));
> >> +
> >> +               /*
> >> +                * If we have multiple ports supposedlt sharing the
> >> +                * aux channel, then DP couldn't exist on the shared
> >> +                * port. Otherwise they share the same aux channel
> >> +                * and system couldn't communicate with them separately.
> >> +                *
> >> +                * Due to parsing the ports in alphabetical order,
> >> +                * a higher port will always clobber a lower one.
> >> +                */
> >> +               i->supports_dp = false;
> >> +               i->alternate_aux_channel = 0;
> >> +       }
> >> +}
> >> +
> >>  static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port
> >> port,
> >>                            const struct bdb_header *bdb)
> >>  {
> >> @@ -1109,54 +1180,15 @@ static void parse_ddi_port(struct
> >> drm_i915_private *dev_priv, enum port port,
> >>                 DRM_DEBUG_KMS("Port %c is internal DP\n",
> >> port_name(port));
> >>
> >>         if (is_dvi) {
> >> -               if (port == PORT_E) {
> >> -                       info->alternate_ddc_pin = ddc_pin;
> >> -                       /* if DDIE share ddc pin with other port, then
> >> -                        * dvi/hdmi couldn't exist on the shared port.
> >> -                        * Otherwise they share the same ddc bin and
> >> system
> >> -                        * couldn't communicate with them seperately. */
> >> -                       if (ddc_pin == DDC_PIN_B) {
> >> -                               dev_priv->vbt.ddi_port_info[PORT_B].supports_dvi
> >> = 0;
> >> -                               dev_priv->vbt.ddi_port_info[PORT_B].supports_hdmi
> >> = 0;
> >> -                       } else if (ddc_pin == DDC_PIN_C) {
> >> -                               dev_priv->vbt.ddi_port_info[PORT_C].supports_dvi
> >> = 0;
> >> -                               dev_priv->vbt.ddi_port_info[PORT_C].supports_hdmi
> >> = 0;
> >> -                       } else if (ddc_pin == DDC_PIN_D) {
> >> -                               dev_priv->vbt.ddi_port_info[PORT_D].supports_dvi
> >> = 0;
> >> -                               dev_priv->vbt.ddi_port_info[PORT_D].supports_hdmi
> >> = 0;
> >> -                       }
> >> -               } else if (ddc_pin == DDC_PIN_B && port != PORT_B)
> >> -                       DRM_DEBUG_KMS("Unexpected DDC pin for port B\n");
> >> -               else if (ddc_pin == DDC_PIN_C && port != PORT_C)
> >> -                       DRM_DEBUG_KMS("Unexpected DDC pin for port C\n");
> >> -               else if (ddc_pin == DDC_PIN_D && port != PORT_D)
> >> -                       DRM_DEBUG_KMS("Unexpected DDC pin for port D\n");
> >> +               info->alternate_ddc_pin = ddc_pin;
> >> +
> >> +               sanitize_ddc_pin(dev_priv, port);
> >>         }
> >>
> >>         if (is_dp) {
> >> -               if (port == PORT_E) {
> >> -                       info->alternate_aux_channel = aux_channel;
> >> -                       /* if DDIE share aux channel with other port, then
> >> -                        * DP couldn't exist on the shared port. Otherwise
> >> -                        * they share the same aux channel and system
> >> -                        * couldn't communicate with them seperately. */
> >> -                       if (aux_channel == DP_AUX_A)
> >> -                               dev_priv->vbt.ddi_port_info[PORT_A].supports_dp
> >> = 0;
> >> -                       else if (aux_channel == DP_AUX_B)
> >> -                               dev_priv->vbt.ddi_port_info[PORT_B].supports_dp
> >> = 0;
> >> -                       else if (aux_channel == DP_AUX_C)
> >> -                               dev_priv->vbt.ddi_port_info[PORT_C].supports_dp
> >> = 0;
> >> -                       else if (aux_channel == DP_AUX_D)
> >> -                               dev_priv->vbt.ddi_port_info[PORT_D].supports_dp
> >> = 0;
> >> -               }
> >> -               else if (aux_channel == DP_AUX_A && port != PORT_A)
> >> -                       DRM_DEBUG_KMS("Unexpected AUX channel for port
> >> A\n");
> >> -               else if (aux_channel == DP_AUX_B && port != PORT_B)
> >> -                       DRM_DEBUG_KMS("Unexpected AUX channel for port
> >> B\n");
> >> -               else if (aux_channel == DP_AUX_C && port != PORT_C)
> >> -                       DRM_DEBUG_KMS("Unexpected AUX channel for port
> >> C\n");
> >> -               else if (aux_channel == DP_AUX_D && port != PORT_D)
> >> -                       DRM_DEBUG_KMS("Unexpected AUX channel for port
> >> D\n");
> >> +               info->alternate_aux_channel = aux_channel;
> >> +
> >> +               sanitize_aux_ch(dev_priv, port);
> >>         }
> >>
> >>         if (bdb->version >= 158) {
> >> --
> >> 2.7.4
> >>
> >>
> >
> >
> > --
> > Far away from the primal instinct, the song seems to fade away, the river
> > get wider between your thoughts and the things we do and say.
> >
> 
> 
> 
> -- 
> Far away from the primal instinct, the song seems to fade away, the river
> get wider between your thoughts and the things we do and say.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx