[Intel-gfx] [PATCH] drm/i915: Reject modeset when the same digital port is used more than once

[Daniel Vetter]

On Tue, Oct 28, 2014 at 11:20:37AM -0200, Paulo Zanoni wrote:
> 2014-05-19 11:19 GMT-03:00  <ville.syrjala at linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > On pre-HSW we have two encoders per digital port: one HDMI, one DP.
> > However they are the same physical port in hardware and we can't enable
> > both at the same time. Reject the modeset if the user attempts this.
> >
> > So far we've been saved by the fact that we never see both HDMI and DP
> > connectors as connected. But if the user decides to force a mode anyway,
> > all kinds of funny stuff might happen.
> >
> > Unfortunately we don't seem to have any way to inform userspace that
> > such configurations are invalid except by returning an error from
> > setcrtc. possible_clones only covers real cloning situations, and
> > looking at the connector names doesn't work either since we don't
> > always register both connectors for the same port. I suppose the
> > only way to fix that would be to expose only a single encoder per
> > digital port like we do on HSW+ but that would be a fairly large
> > undertaking for little gain.
> >
> > kms_setmode hits this since it forces modes on non-connected VGA and
> > HDMI connectors. Previosuly it just resulted in weirdness such as
> > failed link training. With this patch it will now get an error back
> > from the kernel and will die with an assert since it thinks that the
> > configuration should be fine.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 44 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index a432348..13add38 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9621,6 +9621,45 @@ static bool check_encoder_cloning(struct intel_crtc *crtc)
> >         return true;
> >  }
> >
> > +static bool check_digital_port_conflicts(struct drm_device *dev)
> > +{
> > +       struct intel_connector *connector;
> > +       unsigned int used_ports = 0;
> > +
> > +       /*
> > +        * Walk the connector list instead of the encoder
> > +        * list to detect the problem on ddi platforms
> > +        * where there's just one encoder per digital port.
> > +        */
> > +       list_for_each_entry(connector,
> > +                           &dev->mode_config.connector_list, base.head) {
> > +               struct intel_encoder *encoder = connector->new_encoder;
> > +
> > +               if (!encoder)
> > +                       continue;
> > +
> > +               WARN_ON(!encoder->new_crtc);
> > +
> > +               switch (encoder->type) {
> > +                       unsigned int port_mask;
> > +               case INTEL_OUTPUT_DISPLAYPORT:
> > +               case INTEL_OUTPUT_HDMI:
> > +               case INTEL_OUTPUT_EDP:
> 
> If we're also interested on DDI platforms, we need to check for
> INTEL_OUTPUT_UNKNOWN here too. I guess Daniel could add this while
> applying the patch...

Adding UNKNOWN here smells like a massive hack. Can't we add a new
OUTPUT_DDI type, which together with the ddi personality stuff dtrt?

I'm really don't think that sprinkling UNKOWN here is a good idea ...

> With that: Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>

r-b even without that, just to get things going?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch