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

[Paulo Zanoni]

2014-11-03 8:29 GMT-02:00 Daniel Vetter <daniel at ffwll.ch>:
> 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.

Why?

> Can't we add a new
> OUTPUT_DDI type, which together with the ddi personality stuff dtrt?

Or we can rename INTEL_OUTPUT_UNKNOWN to INTEL_OUTPUT_DDI_DISCONNECTED
so it smells less hacky to you?

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

Why?

>
>> With that: Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>
> r-b even without that, just to get things going?

No.

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni