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

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

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@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@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:
+			port_mask = 1 << enc_to_dig_port(&encoder->base)->port;
+
+			/* the same port mustn't appear more than once */
+			if (used_ports & port_mask)
+				return false;
+
+			used_ports |= port_mask;
+		default:
+			break;
+		}
+	}
+
+	return true;
+}
+
 static struct intel_crtc_config *
 intel_modeset_pipe_config(struct drm_crtc *crtc,
 			  struct drm_framebuffer *fb,
@@ -9637,6 +9676,11 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 		return ERR_PTR(-EINVAL);
 	}
 
+	if (!check_digital_port_conflicts(dev)) {
+		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
+		return ERR_PTR(-EINVAL);
+	}
+
 	pipe_config = kzalloc(sizeof(*pipe_config), GFP_KERNEL);
 	if (!pipe_config)
 		return ERR_PTR(-ENOMEM);
-- 
1.8.5.5

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

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

[Daniel Vetter]

On Mon, May 19, 2014 at 05:19:09PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@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@linux.intel.com>

Do we have a bugzilla for this somewhere?
-Daniel

> ---
>  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:
> +			port_mask = 1 << enc_to_dig_port(&encoder->base)->port;
> +
> +			/* the same port mustn't appear more than once */
> +			if (used_ports & port_mask)
> +				return false;
> +
> +			used_ports |= port_mask;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return true;
> +}
> +
>  static struct intel_crtc_config *
>  intel_modeset_pipe_config(struct drm_crtc *crtc,
>  			  struct drm_framebuffer *fb,
> @@ -9637,6 +9676,11 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> +	if (!check_digital_port_conflicts(dev)) {
> +		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>  	pipe_config = kzalloc(sizeof(*pipe_config), GFP_KERNEL);
>  	if (!pipe_config)
>  		return ERR_PTR(-ENOMEM);
> -- 
> 1.8.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Ville Syrjälä]

On Mon, May 19, 2014 at 04:29:57PM +0200, Daniel Vetter wrote:
> On Mon, May 19, 2014 at 05:19:09PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@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@linux.intel.com>
> 
> Do we have a bugzilla for this somewhere?

No idea. Can't spot anything right now that would match my IVB
kms_setmode symptoms (tons of link training failures when the same port
gets used for both DP and HDMI).

-- 
Ville Syrjälä
Intel OTC

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

[Paulo Zanoni]

2014-05-19 11:19 GMT-03:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@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@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...

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

> +                       port_mask = 1 << enc_to_dig_port(&encoder->base)->port;
> +
> +                       /* the same port mustn't appear more than once */
> +                       if (used_ports & port_mask)
> +                               return false;
> +
> +                       used_ports |= port_mask;
> +               default:
> +                       break;
> +               }
> +       }
> +
> +       return true;
> +}
> +
>  static struct intel_crtc_config *
>  intel_modeset_pipe_config(struct drm_crtc *crtc,
>                           struct drm_framebuffer *fb,
> @@ -9637,6 +9676,11 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>                 return ERR_PTR(-EINVAL);
>         }
>
> +       if (!check_digital_port_conflicts(dev)) {
> +               DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
> +               return ERR_PTR(-EINVAL);
> +       }
> +
>         pipe_config = kzalloc(sizeof(*pipe_config), GFP_KERNEL);
>         if (!pipe_config)
>                 return ERR_PTR(-ENOMEM);
> --
> 1.8.5.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@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@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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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@linux.intel.com>:
>> > From: Ville Syrjälä <ville.syrjala@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@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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Tue, Nov 04, 2014 at 10:50:55AM -0200, Paulo Zanoni wrote:
> 2014-11-03 8:29 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> >> 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?

Someone will reuse UNKOWN for something else than DDI.
> 
> > 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?

Yeah that works, too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@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.

v2: Deal with INTEL_OUTPUT_UNKNOWN (Paulo)

Cc: Paulo Zanoni <przanoni@gmail.com>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 47 ++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c424c36..3a088c0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10218,6 +10218,48 @@ 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_UNKNOWN:
+			if (WARN_ON(!HAS_DDI(dev)))
+				break;
+		case INTEL_OUTPUT_DISPLAYPORT:
+		case INTEL_OUTPUT_HDMI:
+		case INTEL_OUTPUT_EDP:
+			port_mask = 1 << enc_to_dig_port(&encoder->base)->port;
+
+			/* the same port mustn't appear more than once */
+			if (used_ports & port_mask)
+				return false;
+
+			used_ports |= port_mask;
+		default:
+			break;
+		}
+	}
+
+	return true;
+}
+
 static struct intel_crtc_config *
 intel_modeset_pipe_config(struct drm_crtc *crtc,
 			  struct drm_framebuffer *fb,
@@ -10234,6 +10276,11 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 		return ERR_PTR(-EINVAL);
 	}
 
+	if (!check_digital_port_conflicts(dev)) {
+		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
+		return ERR_PTR(-EINVAL);
+	}
+
 	pipe_config = kzalloc(sizeof(*pipe_config), GFP_KERNEL);
 	if (!pipe_config)
 		return ERR_PTR(-ENOMEM);
-- 
2.0.4

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

Re: [PATCH v2] drm/i915: Reject modeset when the same digital port is used more than once

[Chris Wilson]

On Tue, Dec 02, 2014 at 02:10:46PM +0200, ville.syrjala@linux.intel.com wrote:
> +	if (!check_digital_port_conflicts(dev)) {

Being picky:

if not check digital port for conflicts, report error.

It reads backwards. Perhaps 

if (conflicting_digital_port_config(dev)) return -EINVAL;

> +		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
> +		return ERR_PTR(-EINVAL);
> +	}

Regarding getting more information back to the user about the error
message, we could with have a connector/crtc property, a procfs file or
an ioctl to grab a string describing the last error. A LastError
property blob might be the most convenient. Though I am not sure how
outlandish this idea is.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Reject modeset when the same digital port is used more than once

[Ville Syrjälä]

On Tue, Dec 02, 2014 at 12:17:28PM +0000, Chris Wilson wrote:
> On Tue, Dec 02, 2014 at 02:10:46PM +0200, ville.syrjala@linux.intel.com wrote:
> > +	if (!check_digital_port_conflicts(dev)) {
> 
> Being picky:
> 
> if not check digital port for conflicts, report error.
> 
> It reads backwards. Perhaps 
> 
> if (conflicting_digital_port_config(dev)) return -EINVAL;

I just followed the pattern established by check_encoder_cloning(). Should
probably rename both if we go down that path.

> 
> > +		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
> > +		return ERR_PTR(-EINVAL);
> > +	}
> 
> Regarding getting more information back to the user about the error
> message, we could with have a connector/crtc property, a procfs file or
> an ioctl to grab a string describing the last error. A LastError
> property blob might be the most convenient. Though I am not sure how
> outlandish this idea is.

Yeah somehow passing a string to userland might be nice for the user. But
I don't think we should start matching such strings in tests, so that
wouldn't help kms_setmode figure out if it just tried something that was
supposed to work or not.

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

Re: [PATCH v2] drm/i915: Reject modeset when the same digital port is used more than once

[Daniel Vetter]

On Tue, Dec 02, 2014 at 12:17:28PM +0000, Chris Wilson wrote:
> On Tue, Dec 02, 2014 at 02:10:46PM +0200, ville.syrjala@linux.intel.com wrote:
> > +	if (!check_digital_port_conflicts(dev)) {
> 
> Being picky:
> 
> if not check digital port for conflicts, report error.
> 
> It reads backwards. Perhaps 
> 
> if (conflicting_digital_port_config(dev)) return -EINVAL;
> 
> > +		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
> > +		return ERR_PTR(-EINVAL);
> > +	}
> 
> Regarding getting more information back to the user about the error
> message, we could with have a connector/crtc property, a procfs file or
> an ioctl to grab a string describing the last error. A LastError
> property blob might be the most convenient. Though I am not sure how
> outlandish this idea is.

It's an unsolved problem since right now we don't tell anyone ever why
something doesn't work. The current solution is the check-only mode of
atomic where you can quickly go through a bunch of options and then pick
the most suitable using heuristics (either user picking what he likes or
in the code).

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Reject modeset when the same digital port is used more than once

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -1              364/364              363/364
ILK              +1                 365/366              366/366
SNB                                  450/450              450/450
IVB                                  498/498              498/498
BYT                                  289/289              289/289
HSW                 -1              564/564              563/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt_gem_partial_pwrite_pread_write      PASS(2, M25M7)      NO_RESULT(1, M7)
 ILK  igt_kms_flip_wf_vblank-ts-check      DMESG_WARN(2, M26)PASS(10, M26M37)      PASS(1, M37)
*HSW  igt_gem_flink_race_flink_close      PASS(2, M19)      DMESG_WARN(1, M19)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx