[PATCH] drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector

[PATCH] drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector

[James Ausmus]

Make intel_dp_add_mst_connector handle error returns from the drm_ calls.
Add intel_connector_destroy to support cleanup on the error path.

Signed-off-by: James Ausmus <james.ausmus@intel.com>
---

Note that this patch is compile/boot tested only on non-MST, as I
currently don't have MST-enabled HW.

 drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
 drivers/gpu/drm/i915/intel_dp_mst.c  | 24 ++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 3 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8599e425abb1..ab261c063032 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6146,6 +6146,16 @@ struct intel_connector *intel_connector_alloc(void)
 	return connector;
 }
 
+/*
+ * Free the bits allocated by intel_connector_alloc.
+ * Type-specific connector cleanup should be done prior to this.
+ */
+void intel_connector_destroy(struct intel_connector *connector)
+{
+	kfree(to_intel_digital_connector_state(connector->base.state));
+	kfree(connector);
+}
+
 /* Simple connector->get_hw_state implementation for encoders that support only
  * one connector and no cloning and hence the encoder state determines the state
  * of the connector. */
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 8e3aad0ea60b..80b3d665e988 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -451,14 +451,18 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct intel_connector *intel_connector;
 	struct drm_connector *connector;
-	int i;
+	int i, ret;
 
 	intel_connector = intel_connector_alloc();
 	if (!intel_connector)
 		return NULL;
 
 	connector = &intel_connector->base;
-	drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs, DRM_MODE_CONNECTOR_DisplayPort);
+	ret = drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs,
+				 DRM_MODE_CONNECTOR_DisplayPort);
+	if (ret)
+		goto out_init;
+
 	drm_connector_helper_add(connector, &intel_dp_mst_connector_helper_funcs);
 
 	intel_connector->get_hw_state = intel_dp_mst_get_hw_state;
@@ -466,14 +470,26 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	intel_connector->port = port;
 
 	for (i = PIPE_A; i <= PIPE_C; i++) {
-		drm_mode_connector_attach_encoder(&intel_connector->base,
+		ret = drm_mode_connector_attach_encoder(&intel_connector->base,
 						  &intel_dp->mst_encoders[i]->base.base);
+		if (ret)
+			goto out_conn;
 	}
 
 	drm_object_attach_property(&connector->base, dev->mode_config.path_property, 0);
 	drm_object_attach_property(&connector->base, dev->mode_config.tile_property, 0);
 
-	drm_mode_connector_set_path_property(connector, pathprop);
+	ret = drm_mode_connector_set_path_property(connector, pathprop);
+
+out_conn:
+	if (ret)
+		drm_connector_cleanup(connector);
+out_init:
+	if (ret) {
+		intel_connector_destroy(intel_connector);
+		connector = NULL;
+	}
+
 	return connector;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 307807672896..d13c5c6b46e9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1358,6 +1358,7 @@ void intel_pps_unlock_regs_wa(struct drm_i915_private *dev_priv);
 void intel_encoder_destroy(struct drm_encoder *encoder);
 int intel_connector_init(struct intel_connector *);
 struct intel_connector *intel_connector_alloc(void);
+void intel_connector_destroy(struct intel_connector *connector);
 bool intel_connector_get_hw_state(struct intel_connector *connector);
 void intel_connector_attach_encoder(struct intel_connector *connector,
 				    struct intel_encoder *encoder);
-- 
2.14.1

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

✓ Fi.CI.BAT: success for drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector

[Patchwork]

== Series Details ==

Series: drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector
URL   : https://patchwork.freedesktop.org/series/30384/
State : success

== Summary ==

Series 30384v1 drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector
https://patchwork.freedesktop.org/api/1.0/series/30384/revisions/1/mbox/

Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                fail       -> PASS       (fi-snb-2600) fdo#100215

fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:451s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:455s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:377s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:534s
fi-bwr-2160      total:289  pass:184  dwarn:0   dfail:0   fail:0   skip:105 time:267s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:503s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:504s
fi-byt-n2820     total:289  pass:250  dwarn:1   dfail:0   fail:0   skip:38  time:492s
fi-cfl-s         total:289  pass:223  dwarn:34  dfail:0   fail:0   skip:32  time:546s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:449s
fi-glk-2a        total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:595s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:432s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:407s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:436s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:483s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:466s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:484s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:576s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:588s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:549s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:461s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:527s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:502s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:462s
fi-skl-x1585l    total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:472s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:573s
fi-snb-2600      total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:429s

12bd8f17c04c174e1e4b98338147870997306558 drm-tip: 2017y-09m-14d-19h-32m-57s UTC integration manifest
70dd52ab1af2 drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5711/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector

[Patchwork]

== Series Details ==

Series: drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector
URL   : https://patchwork.freedesktop.org/series/30384/
State : success

== Summary ==

Test kms_setmode:
        Subgroup basic:
                pass       -> FAIL       (shard-hsw) fdo#99912

fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912

shard-hsw        total:2266 pass:1217 dwarn:0   dfail:0   fail:12  skip:1037 time:9267s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5711/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector

[Ville Syrjälä]

On Thu, Sep 14, 2017 at 12:59:35PM -0700, James Ausmus wrote:
> Make intel_dp_add_mst_connector handle error returns from the drm_ calls.
> Add intel_connector_destroy to support cleanup on the error path.

That name makes one think that it could be plugged into the connector
.destroy() hook. So I'd rename it to intel_connector_free() or something
like that.

Also the this MST thing just looks like the tip of the iceberg.
I think pretty much every connector has the same problem. But I
guess MST is slightly more interesting since it can happen at
runtime.

> 
> Signed-off-by: James Ausmus <james.ausmus@intel.com>
> ---
> 
> Note that this patch is compile/boot tested only on non-MST, as I
> currently don't have MST-enabled HW.
> 
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
>  drivers/gpu/drm/i915/intel_dp_mst.c  | 24 ++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  3 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8599e425abb1..ab261c063032 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6146,6 +6146,16 @@ struct intel_connector *intel_connector_alloc(void)
>  	return connector;
>  }
>  
> +/*
> + * Free the bits allocated by intel_connector_alloc.
> + * Type-specific connector cleanup should be done prior to this.
> + */
> +void intel_connector_destroy(struct intel_connector *connector)
> +{
> +	kfree(to_intel_digital_connector_state(connector->base.state));
> +	kfree(connector);
> +}
> +
>  /* Simple connector->get_hw_state implementation for encoders that support only
>   * one connector and no cloning and hence the encoder state determines the state
>   * of the connector. */
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 8e3aad0ea60b..80b3d665e988 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -451,14 +451,18 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>  	struct intel_connector *intel_connector;
>  	struct drm_connector *connector;
> -	int i;
> +	int i, ret;
>  
>  	intel_connector = intel_connector_alloc();
>  	if (!intel_connector)
>  		return NULL;
>  
>  	connector = &intel_connector->base;
> -	drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs, DRM_MODE_CONNECTOR_DisplayPort);
> +	ret = drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs,
> +				 DRM_MODE_CONNECTOR_DisplayPort);
> +	if (ret)
> +		goto out_init;
> +
>  	drm_connector_helper_add(connector, &intel_dp_mst_connector_helper_funcs);
>  
>  	intel_connector->get_hw_state = intel_dp_mst_get_hw_state;
> @@ -466,14 +470,26 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>  	intel_connector->port = port;
>  
>  	for (i = PIPE_A; i <= PIPE_C; i++) {
> -		drm_mode_connector_attach_encoder(&intel_connector->base,
> +		ret = drm_mode_connector_attach_encoder(&intel_connector->base,
>  						  &intel_dp->mst_encoders[i]->base.base);
> +		if (ret)
> +			goto out_conn;
>  	}
>  
>  	drm_object_attach_property(&connector->base, dev->mode_config.path_property, 0);
>  	drm_object_attach_property(&connector->base, dev->mode_config.tile_property, 0);
>  
> -	drm_mode_connector_set_path_property(connector, pathprop);
> +	ret = drm_mode_connector_set_path_property(connector, pathprop);
> +

'return connector' here so you can skip the 'if (ret)' things below?

> +out_conn:
> +	if (ret)
> +		drm_connector_cleanup(connector);
> +out_init:
> +	if (ret) {
> +		intel_connector_destroy(intel_connector);
> +		connector = NULL;
> +	}
> +
>  	return connector;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 307807672896..d13c5c6b46e9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1358,6 +1358,7 @@ void intel_pps_unlock_regs_wa(struct drm_i915_private *dev_priv);
>  void intel_encoder_destroy(struct drm_encoder *encoder);
>  int intel_connector_init(struct intel_connector *);
>  struct intel_connector *intel_connector_alloc(void);
> +void intel_connector_destroy(struct intel_connector *connector);
>  bool intel_connector_get_hw_state(struct intel_connector *connector);
>  void intel_connector_attach_encoder(struct intel_connector *connector,
>  				    struct intel_encoder *encoder);
> -- 
> 2.14.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector

[Ausmus, James]

On Fri, Sep 15, 2017 at 3:18 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Thu, Sep 14, 2017 at 12:59:35PM -0700, James Ausmus wrote:
>> Make intel_dp_add_mst_connector handle error returns from the drm_ calls.
>> Add intel_connector_destroy to support cleanup on the error path.
>
> That name makes one think that it could be plugged into the connector
> .destroy() hook. So I'd rename it to intel_connector_free() or something
> like that.

Sounds good.

>
> Also the this MST thing just looks like the tip of the iceberg.
> I think pretty much every connector has the same problem. But I
> guess MST is slightly more interesting since it can happen at
> runtime.

Yeah, I'll look at fixing up the other connectors too, as I can.

>
>>
>> Signed-off-by: James Ausmus <james.ausmus@intel.com>
>> ---
>>
>> Note that this patch is compile/boot tested only on non-MST, as I
>> currently don't have MST-enabled HW.
>>
>>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
>>  drivers/gpu/drm/i915/intel_dp_mst.c  | 24 ++++++++++++++++++++----
>>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>  3 files changed, 31 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 8599e425abb1..ab261c063032 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6146,6 +6146,16 @@ struct intel_connector *intel_connector_alloc(void)
>>       return connector;
>>  }
>>
>> +/*
>> + * Free the bits allocated by intel_connector_alloc.
>> + * Type-specific connector cleanup should be done prior to this.
>> + */
>> +void intel_connector_destroy(struct intel_connector *connector)
>> +{
>> +     kfree(to_intel_digital_connector_state(connector->base.state));
>> +     kfree(connector);
>> +}
>> +
>>  /* Simple connector->get_hw_state implementation for encoders that support only
>>   * one connector and no cloning and hence the encoder state determines the state
>>   * of the connector. */
>> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
>> index 8e3aad0ea60b..80b3d665e988 100644
>> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
>> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
>> @@ -451,14 +451,18 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>>       struct drm_device *dev = intel_dig_port->base.base.dev;
>>       struct intel_connector *intel_connector;
>>       struct drm_connector *connector;
>> -     int i;
>> +     int i, ret;
>>
>>       intel_connector = intel_connector_alloc();
>>       if (!intel_connector)
>>               return NULL;
>>
>>       connector = &intel_connector->base;
>> -     drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs, DRM_MODE_CONNECTOR_DisplayPort);
>> +     ret = drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs,
>> +                              DRM_MODE_CONNECTOR_DisplayPort);
>> +     if (ret)
>> +             goto out_init;
>> +
>>       drm_connector_helper_add(connector, &intel_dp_mst_connector_helper_funcs);
>>
>>       intel_connector->get_hw_state = intel_dp_mst_get_hw_state;
>> @@ -466,14 +470,26 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>>       intel_connector->port = port;
>>
>>       for (i = PIPE_A; i <= PIPE_C; i++) {
>> -             drm_mode_connector_attach_encoder(&intel_connector->base,
>> +             ret = drm_mode_connector_attach_encoder(&intel_connector->base,
>>                                                 &intel_dp->mst_encoders[i]->base.base);
>> +             if (ret)
>> +                     goto out_conn;
>>       }
>>
>>       drm_object_attach_property(&connector->base, dev->mode_config.path_property, 0);
>>       drm_object_attach_property(&connector->base, dev->mode_config.tile_property, 0);
>>
>> -     drm_mode_connector_set_path_property(connector, pathprop);
>> +     ret = drm_mode_connector_set_path_property(connector, pathprop);
>> +
>
> 'return connector' here so you can skip the 'if (ret)' things below?

Will do.


Thanks for the review!

-James

>
>> +out_conn:
>> +     if (ret)
>> +             drm_connector_cleanup(connector);
>> +out_init:
>> +     if (ret) {
>> +             intel_connector_destroy(intel_connector);
>> +             connector = NULL;
>> +     }
>> +
>>       return connector;
>>  }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 307807672896..d13c5c6b46e9 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1358,6 +1358,7 @@ void intel_pps_unlock_regs_wa(struct drm_i915_private *dev_priv);
>>  void intel_encoder_destroy(struct drm_encoder *encoder);
>>  int intel_connector_init(struct intel_connector *);
>>  struct intel_connector *intel_connector_alloc(void);
>> +void intel_connector_destroy(struct intel_connector *connector);
>>  bool intel_connector_get_hw_state(struct intel_connector *connector);
>>  void intel_connector_attach_encoder(struct intel_connector *connector,
>>                                   struct intel_encoder *encoder);
>> --
>> 2.14.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC



-- 


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

[PATCH v2] drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector

[James Ausmus]

Make intel_dp_add_mst_connector handle error returns from the drm_ calls.
Add intel_connector_free to support cleanup on the error path.

v2: Rename new function to avoid confusion, and simplify error
paths (Ville)

Signed-off-by: James Ausmus <james.ausmus@intel.com>
---

Note that this patch is compile/boot tested only on non-MST, as I
currently don't have MST-enabled HW.

 drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
 drivers/gpu/drm/i915/intel_dp_mst.c  | 23 +++++++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8599e425abb1..e3b1a6ead4fb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6146,6 +6146,16 @@ struct intel_connector *intel_connector_alloc(void)
 	return connector;
 }
 
+/*
+ * Free the bits allocated by intel_connector_alloc.
+ * Type-specific connector cleanup should be done prior to this.
+ */
+void intel_connector_free(struct intel_connector *connector)
+{
+	kfree(to_intel_digital_connector_state(connector->base.state));
+	kfree(connector);
+}
+
 /* Simple connector->get_hw_state implementation for encoders that support only
  * one connector and no cloning and hence the encoder state determines the state
  * of the connector. */
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 8e3aad0ea60b..6e4447cbbe0e 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -451,14 +451,18 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct intel_connector *intel_connector;
 	struct drm_connector *connector;
-	int i;
+	int i, ret;
 
 	intel_connector = intel_connector_alloc();
 	if (!intel_connector)
 		return NULL;
 
 	connector = &intel_connector->base;
-	drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs, DRM_MODE_CONNECTOR_DisplayPort);
+	ret = drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs,
+				 DRM_MODE_CONNECTOR_DisplayPort);
+	if (ret)
+		goto out_init;
+
 	drm_connector_helper_add(connector, &intel_dp_mst_connector_helper_funcs);
 
 	intel_connector->get_hw_state = intel_dp_mst_get_hw_state;
@@ -466,14 +470,25 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	intel_connector->port = port;
 
 	for (i = PIPE_A; i <= PIPE_C; i++) {
-		drm_mode_connector_attach_encoder(&intel_connector->base,
+		ret = drm_mode_connector_attach_encoder(&intel_connector->base,
 						  &intel_dp->mst_encoders[i]->base.base);
+		if (ret)
+			goto out_conn;
 	}
 
 	drm_object_attach_property(&connector->base, dev->mode_config.path_property, 0);
 	drm_object_attach_property(&connector->base, dev->mode_config.tile_property, 0);
 
-	drm_mode_connector_set_path_property(connector, pathprop);
+	ret = drm_mode_connector_set_path_property(connector, pathprop);
+	if (ret == 0)
+		return connector;
+
+out_conn:
+	drm_connector_cleanup(connector);
+out_init:
+	intel_connector_free(intel_connector);
+	connector = NULL;
+
 	return connector;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 307807672896..2a5cee4302f7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1358,6 +1358,7 @@ void intel_pps_unlock_regs_wa(struct drm_i915_private *dev_priv);
 void intel_encoder_destroy(struct drm_encoder *encoder);
 int intel_connector_init(struct intel_connector *);
 struct intel_connector *intel_connector_alloc(void);
+void intel_connector_free(struct intel_connector *connector);
 bool intel_connector_get_hw_state(struct intel_connector *connector);
 void intel_connector_attach_encoder(struct intel_connector *connector,
 				    struct intel_encoder *encoder);
-- 
2.14.1

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

Re: [PATCH v2] drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector

[Ville Syrjälä]

On Fri, Sep 15, 2017 at 10:49:09AM -0700, James Ausmus wrote:
> Make intel_dp_add_mst_connector handle error returns from the drm_ calls.
> Add intel_connector_free to support cleanup on the error path.
> 
> v2: Rename new function to avoid confusion, and simplify error
> paths (Ville)
> 
> Signed-off-by: James Ausmus <james.ausmus@intel.com>
> ---
> 
> Note that this patch is compile/boot tested only on non-MST, as I
> currently don't have MST-enabled HW.
> 
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
>  drivers/gpu/drm/i915/intel_dp_mst.c  | 23 +++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  3 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8599e425abb1..e3b1a6ead4fb 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6146,6 +6146,16 @@ struct intel_connector *intel_connector_alloc(void)
>  	return connector;
>  }
>  
> +/*
> + * Free the bits allocated by intel_connector_alloc.
> + * Type-specific connector cleanup should be done prior to this.
> + */
> +void intel_connector_free(struct intel_connector *connector)
> +{
> +	kfree(to_intel_digital_connector_state(connector->base.state));
> +	kfree(connector);
> +}
> +
>  /* Simple connector->get_hw_state implementation for encoders that support only
>   * one connector and no cloning and hence the encoder state determines the state
>   * of the connector. */
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 8e3aad0ea60b..6e4447cbbe0e 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -451,14 +451,18 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>  	struct intel_connector *intel_connector;
>  	struct drm_connector *connector;
> -	int i;
> +	int i, ret;
>  
>  	intel_connector = intel_connector_alloc();
>  	if (!intel_connector)
>  		return NULL;
>  
>  	connector = &intel_connector->base;
> -	drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs, DRM_MODE_CONNECTOR_DisplayPort);
> +	ret = drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs,
> +				 DRM_MODE_CONNECTOR_DisplayPort);
> +	if (ret)
> +		goto out_init;
> +
>  	drm_connector_helper_add(connector, &intel_dp_mst_connector_helper_funcs);
>  
>  	intel_connector->get_hw_state = intel_dp_mst_get_hw_state;
> @@ -466,14 +470,25 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>  	intel_connector->port = port;
>  
>  	for (i = PIPE_A; i <= PIPE_C; i++) {

BTW a followup patch to switch these mst_encoders[] loops over
to for_each_pipe() might be nice.

> -		drm_mode_connector_attach_encoder(&intel_connector->base,
> +		ret = drm_mode_connector_attach_encoder(&intel_connector->base,
>  						  &intel_dp->mst_encoders[i]->base.base);

Please reindent appropriately.

> +		if (ret)
> +			goto out_conn;
>  	}
>  
>  	drm_object_attach_property(&connector->base, dev->mode_config.path_property, 0);
>  	drm_object_attach_property(&connector->base, dev->mode_config.tile_property, 0);
>  
> -	drm_mode_connector_set_path_property(connector, pathprop);
> +	ret = drm_mode_connector_set_path_property(connector, pathprop);
> +	if (ret == 0)
> +		return connector;

if (ret)
	goto ...;

return connector;

is more idiomatic.

> +
> +out_conn:
> +	drm_connector_cleanup(connector);
> +out_init:
> +	intel_connector_free(intel_connector);

Hmm. I might call these out_cleanup and out_free. But I guess we have no
consistent style when it comes to goto labes. So feel free to ignore me.

> +	connector = NULL;
> +
>  	return connector;

Just return NULL.

>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 307807672896..2a5cee4302f7 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1358,6 +1358,7 @@ void intel_pps_unlock_regs_wa(struct drm_i915_private *dev_priv);
>  void intel_encoder_destroy(struct drm_encoder *encoder);
>  int intel_connector_init(struct intel_connector *);
>  struct intel_connector *intel_connector_alloc(void);
> +void intel_connector_free(struct intel_connector *connector);
>  bool intel_connector_get_hw_state(struct intel_connector *connector);
>  void intel_connector_attach_encoder(struct intel_connector *connector,
>  				    struct intel_encoder *encoder);
> -- 
> 2.14.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v2] drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector

[Ausmus, James]

On Fri, Sep 15, 2017 at 11:09 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Fri, Sep 15, 2017 at 10:49:09AM -0700, James Ausmus wrote:
>> Make intel_dp_add_mst_connector handle error returns from the drm_ calls.
>> Add intel_connector_free to support cleanup on the error path.
>>
>> v2: Rename new function to avoid confusion, and simplify error
>> paths (Ville)
>>
>> Signed-off-by: James Ausmus <james.ausmus@intel.com>
>> ---
>>
>> Note that this patch is compile/boot tested only on non-MST, as I
>> currently don't have MST-enabled HW.
>>
>>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
>>  drivers/gpu/drm/i915/intel_dp_mst.c  | 23 +++++++++++++++++++----
>>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>  3 files changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 8599e425abb1..e3b1a6ead4fb 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6146,6 +6146,16 @@ struct intel_connector *intel_connector_alloc(void)
>>       return connector;
>>  }
>>
>> +/*
>> + * Free the bits allocated by intel_connector_alloc.
>> + * Type-specific connector cleanup should be done prior to this.
>> + */
>> +void intel_connector_free(struct intel_connector *connector)
>> +{
>> +     kfree(to_intel_digital_connector_state(connector->base.state));
>> +     kfree(connector);
>> +}
>> +
>>  /* Simple connector->get_hw_state implementation for encoders that support only
>>   * one connector and no cloning and hence the encoder state determines the state
>>   * of the connector. */
>> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
>> index 8e3aad0ea60b..6e4447cbbe0e 100644
>> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
>> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
>> @@ -451,14 +451,18 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>>       struct drm_device *dev = intel_dig_port->base.base.dev;
>>       struct intel_connector *intel_connector;
>>       struct drm_connector *connector;
>> -     int i;
>> +     int i, ret;
>>
>>       intel_connector = intel_connector_alloc();
>>       if (!intel_connector)
>>               return NULL;
>>
>>       connector = &intel_connector->base;
>> -     drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs, DRM_MODE_CONNECTOR_DisplayPort);
>> +     ret = drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs,
>> +                              DRM_MODE_CONNECTOR_DisplayPort);
>> +     if (ret)
>> +             goto out_init;
>> +
>>       drm_connector_helper_add(connector, &intel_dp_mst_connector_helper_funcs);
>>
>>       intel_connector->get_hw_state = intel_dp_mst_get_hw_state;
>> @@ -466,14 +470,25 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>>       intel_connector->port = port;
>>
>>       for (i = PIPE_A; i <= PIPE_C; i++) {
>
> BTW a followup patch to switch these mst_encoders[] loops over
> to for_each_pipe() might be nice.

I'll queue that one up as well :)

>
>> -             drm_mode_connector_attach_encoder(&intel_connector->base,
>> +             ret = drm_mode_connector_attach_encoder(&intel_connector->base,
>>                                                 &intel_dp->mst_encoders[i]->base.base);
>
> Please reindent appropriately.

Done

>
>> +             if (ret)
>> +                     goto out_conn;
>>       }
>>
>>       drm_object_attach_property(&connector->base, dev->mode_config.path_property, 0);
>>       drm_object_attach_property(&connector->base, dev->mode_config.tile_property, 0);
>>
>> -     drm_mode_connector_set_path_property(connector, pathprop);
>> +     ret = drm_mode_connector_set_path_property(connector, pathprop);
>> +     if (ret == 0)
>> +             return connector;
>
> if (ret)
>         goto ...;
>
> return connector;
>
> is more idiomatic.

Done

>
>> +
>> +out_conn:
>> +     drm_connector_cleanup(connector);
>> +out_init:
>> +     intel_connector_free(intel_connector);
>
> Hmm. I might call these out_cleanup and out_free. But I guess we have no
> consistent style when it comes to goto labes. So feel free to ignore me.

Sounds good to me - done :)

>
>> +     connector = NULL;
>> +
>>       return connector;
>
> Just return NULL.

Done

Thanks for the review!

>
>>  }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 307807672896..2a5cee4302f7 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1358,6 +1358,7 @@ void intel_pps_unlock_regs_wa(struct drm_i915_private *dev_priv);
>>  void intel_encoder_destroy(struct drm_encoder *encoder);
>>  int intel_connector_init(struct intel_connector *);
>>  struct intel_connector *intel_connector_alloc(void);
>> +void intel_connector_free(struct intel_connector *connector);
>>  bool intel_connector_get_hw_state(struct intel_connector *connector);
>>  void intel_connector_attach_encoder(struct intel_connector *connector,
>>                                   struct intel_encoder *encoder);
>> --
>> 2.14.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC



-- 


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

✓ Fi.CI.BAT: success for drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector (rev2)
URL   : https://patchwork.freedesktop.org/series/30384/
State : success

== Summary ==

Series 30384v2 drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector
https://patchwork.freedesktop.org/api/1.0/series/30384/revisions/2/mbox/

Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                fail       -> PASS       (fi-snb-2600) fdo#100215 +1

fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:446s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:453s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:381s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:525s
fi-bwr-2160      total:289  pass:184  dwarn:0   dfail:0   fail:0   skip:105 time:268s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:508s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:504s
fi-byt-n2820     total:289  pass:250  dwarn:1   dfail:0   fail:0   skip:38  time:487s
fi-cfl-s         total:289  pass:223  dwarn:34  dfail:0   fail:0   skip:32  time:546s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:423s
fi-glk-2a        total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:590s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:428s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:408s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:432s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:481s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:463s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:487s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:579s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:588s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:550s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:452s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:518s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:496s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:461s
fi-skl-x1585l    total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:477s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:565s
fi-snb-2600      total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:420s

9adc9e93d6243c82bcefd175c2d11770802de194 drm-tip: 2017y-09m-15d-11h-44m-46s UTC integration manifest
3687f332bb98 drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5716/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3] drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector

[James Ausmus]

Make intel_dp_add_mst_connector handle error returns from the drm_ calls.
Add intel_connector_free to support cleanup on the error path.

v2: Rename new function to avoid confusion, and simplify error
paths (Ville)

v3: Indentation fixup, style fixes (Ville)

Signed-off-by: James Ausmus <james.ausmus@intel.com>
---

Note that this patch is compile/boot tested only on non-MST, as I
currently don't have MST-enabled HW.

 drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
 drivers/gpu/drm/i915/intel_dp_mst.c  | 27 ++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 3 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8599e425abb1..e3b1a6ead4fb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6146,6 +6146,16 @@ struct intel_connector *intel_connector_alloc(void)
 	return connector;
 }
 
+/*
+ * Free the bits allocated by intel_connector_alloc.
+ * Type-specific connector cleanup should be done prior to this.
+ */
+void intel_connector_free(struct intel_connector *connector)
+{
+	kfree(to_intel_digital_connector_state(connector->base.state));
+	kfree(connector);
+}
+
 /* Simple connector->get_hw_state implementation for encoders that support only
  * one connector and no cloning and hence the encoder state determines the state
  * of the connector. */
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 8e3aad0ea60b..45bf3703bc5e 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -451,14 +451,18 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct intel_connector *intel_connector;
 	struct drm_connector *connector;
-	int i;
+	int i, ret;
 
 	intel_connector = intel_connector_alloc();
 	if (!intel_connector)
 		return NULL;
 
 	connector = &intel_connector->base;
-	drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs, DRM_MODE_CONNECTOR_DisplayPort);
+	ret = drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs,
+				 DRM_MODE_CONNECTOR_DisplayPort);
+	if (ret)
+		goto out_free;
+
 	drm_connector_helper_add(connector, &intel_dp_mst_connector_helper_funcs);
 
 	intel_connector->get_hw_state = intel_dp_mst_get_hw_state;
@@ -466,15 +470,28 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	intel_connector->port = port;
 
 	for (i = PIPE_A; i <= PIPE_C; i++) {
-		drm_mode_connector_attach_encoder(&intel_connector->base,
-						  &intel_dp->mst_encoders[i]->base.base);
+		struct drm_encoder *enc = &intel_dp->mst_encoders[i]->base.base;
+
+		ret = drm_mode_connector_attach_encoder(&intel_connector->base,
+							enc);
+		if (ret)
+			goto out_cleanup;
 	}
 
 	drm_object_attach_property(&connector->base, dev->mode_config.path_property, 0);
 	drm_object_attach_property(&connector->base, dev->mode_config.tile_property, 0);
 
-	drm_mode_connector_set_path_property(connector, pathprop);
+	ret = drm_mode_connector_set_path_property(connector, pathprop);
+	if (ret)
+		goto out_cleanup;
+
 	return connector;
+
+out_cleanup:
+	drm_connector_cleanup(connector);
+out_free:
+	intel_connector_free(intel_connector);
+	return NULL;
 }
 
 static void intel_dp_register_mst_connector(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 307807672896..2a5cee4302f7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1358,6 +1358,7 @@ void intel_pps_unlock_regs_wa(struct drm_i915_private *dev_priv);
 void intel_encoder_destroy(struct drm_encoder *encoder);
 int intel_connector_init(struct intel_connector *);
 struct intel_connector *intel_connector_alloc(void);
+void intel_connector_free(struct intel_connector *connector);
 bool intel_connector_get_hw_state(struct intel_connector *connector);
 void intel_connector_attach_encoder(struct intel_connector *connector,
 				    struct intel_encoder *encoder);
-- 
2.14.1

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

✓ Fi.CI.IGT: success for drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector (rev2)
URL   : https://patchwork.freedesktop.org/series/30384/
State : success

== Summary ==

Test perf:
        Subgroup polling:
                pass       -> FAIL       (shard-hsw) fdo#102252
Test kms_setmode:
        Subgroup basic:
                fail       -> PASS       (shard-hsw) fdo#99912

fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912

shard-hsw        total:2313 pass:1245 dwarn:0   dfail:0   fail:13  skip:1055 time:9353s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5716/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector (rev3)
URL   : https://patchwork.freedesktop.org/series/30384/
State : success

== Summary ==

Series 30384v3 drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector
https://patchwork.freedesktop.org/api/1.0/series/30384/revisions/3/mbox/

Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                fail       -> PASS       (fi-snb-2600) fdo#100215
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                skip       -> PASS       (fi-skl-x1585l) fdo#101781

fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fdo#101781 https://bugs.freedesktop.org/show_bug.cgi?id=101781

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:449s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:454s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:381s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:545s
fi-bwr-2160      total:289  pass:184  dwarn:0   dfail:0   fail:0   skip:105 time:270s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:510s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:509s
fi-byt-n2820     total:289  pass:250  dwarn:1   dfail:0   fail:0   skip:38  time:505s
fi-cfl-s         total:289  pass:223  dwarn:34  dfail:0   fail:0   skip:32  time:560s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:424s
fi-glk-2a        total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:600s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:415s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:439s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:495s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:470s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:491s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:584s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:591s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:557s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:467s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:497s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:466s
fi-skl-x1585l    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:507s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:576s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:1   skip:39  time:427s
fi-skl-6700k failed to connect after reboot

9adc9e93d6243c82bcefd175c2d11770802de194 drm-tip: 2017y-09m-15d-11h-44m-46s UTC integration manifest
1534456d40f6 drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5717/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector (rev3)
URL   : https://patchwork.freedesktop.org/series/30384/
State : success

== Summary ==

Test perf:
        Subgroup blocking:
                fail       -> PASS       (shard-hsw) fdo#102252

fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252

shard-hsw        total:2313 pass:1246 dwarn:0   dfail:0   fail:12  skip:1055 time:9393s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5717/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 1/2] drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector

[James Ausmus]

Make intel_dp_add_mst_connector handle error returns from the drm_ calls.
Add intel_connector_free to support cleanup on the error path.

v2: Rename new function to avoid confusion, and simplify error
paths (Ville)

v3: Indentation fixup, style fixes (Ville)

v4: Clarify usage of intel_connector_free, and fix usage of
intel_connector_free

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: James Ausmus <james.ausmus@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
 drivers/gpu/drm/i915/intel_dp_mst.c  | 27 ++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 3 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c4b224a3a0ee..725014bf6f0f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6146,6 +6146,19 @@ struct intel_connector *intel_connector_alloc(void)
 	return connector;
 }
 
+/*
+ * Free the bits allocated by intel_connector_alloc.
+ * This should only be used after intel_connector_alloc has returned
+ * successfully, and before drm_connector_init returns successfully.
+ * Otherwise the destroy callbacks for the connector and the state should
+ * take care of proper cleanup/free
+ */
+void intel_connector_free(struct intel_connector *connector)
+{
+	kfree(to_intel_digital_connector_state(connector->base.state));
+	kfree(connector);
+}
+
 /* Simple connector->get_hw_state implementation for encoders that support only
  * one connector and no cloning and hence the encoder state determines the state
  * of the connector. */
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 9a396f483f8b..8ceffad3e665 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -450,14 +450,20 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct intel_connector *intel_connector;
 	struct drm_connector *connector;
-	int i;
+	int i, ret;
 
 	intel_connector = intel_connector_alloc();
 	if (!intel_connector)
 		return NULL;
 
 	connector = &intel_connector->base;
-	drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs, DRM_MODE_CONNECTOR_DisplayPort);
+	ret = drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs,
+				 DRM_MODE_CONNECTOR_DisplayPort);
+	if (ret) {
+		intel_connector_free(intel_connector);
+		return NULL;
+	}
+
 	drm_connector_helper_add(connector, &intel_dp_mst_connector_helper_funcs);
 
 	intel_connector->get_hw_state = intel_dp_mst_get_hw_state;
@@ -465,15 +471,26 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	intel_connector->port = port;
 
 	for (i = PIPE_A; i <= PIPE_C; i++) {
-		drm_mode_connector_attach_encoder(&intel_connector->base,
-						  &intel_dp->mst_encoders[i]->base.base);
+		struct drm_encoder *enc = &intel_dp->mst_encoders[i]->base.base;
+
+		ret = drm_mode_connector_attach_encoder(&intel_connector->base,
+							enc);
+		if (ret)
+			goto err;
 	}
 
 	drm_object_attach_property(&connector->base, dev->mode_config.path_property, 0);
 	drm_object_attach_property(&connector->base, dev->mode_config.tile_property, 0);
 
-	drm_mode_connector_set_path_property(connector, pathprop);
+	ret = drm_mode_connector_set_path_property(connector, pathprop);
+	if (ret)
+		goto err;
+
 	return connector;
+
+err:
+	drm_connector_cleanup(connector);
+	return NULL;
 }
 
 static void intel_dp_register_mst_connector(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0cab667fff57..b549a0b45e57 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1360,6 +1360,7 @@ void intel_pps_unlock_regs_wa(struct drm_i915_private *dev_priv);
 void intel_encoder_destroy(struct drm_encoder *encoder);
 int intel_connector_init(struct intel_connector *);
 struct intel_connector *intel_connector_alloc(void);
+void intel_connector_free(struct intel_connector *connector);
 bool intel_connector_get_hw_state(struct intel_connector *connector);
 void intel_connector_attach_encoder(struct intel_connector *connector,
 				    struct intel_encoder *encoder);
-- 
2.14.1

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

[PATCH 2/2] drm/i915: Use for_each_pipe in intel_dp_mst.c

[James Ausmus]

Use the helper instead of manually looping through pipes.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: James Ausmus <james.ausmus@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 8ceffad3e665..adb105bf98e2 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -448,9 +448,11 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_connector *intel_connector;
 	struct drm_connector *connector;
-	int i, ret;
+	enum pipe p;
+	int ret;
 
 	intel_connector = intel_connector_alloc();
 	if (!intel_connector)
@@ -470,8 +472,8 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	intel_connector->mst_port = intel_dp;
 	intel_connector->port = port;
 
-	for (i = PIPE_A; i <= PIPE_C; i++) {
-		struct drm_encoder *enc = &intel_dp->mst_encoders[i]->base.base;
+	for_each_pipe(dev_priv, p) {
+		struct drm_encoder *enc = &intel_dp->mst_encoders[p]->base.base;
 
 		ret = drm_mode_connector_attach_encoder(&intel_connector->base,
 							enc);
@@ -580,11 +582,14 @@ intel_dp_create_fake_mst_encoder(struct intel_digital_port *intel_dig_port, enum
 static bool
 intel_dp_create_fake_mst_encoders(struct intel_digital_port *intel_dig_port)
 {
-	int i;
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_dp *intel_dp = &intel_dig_port->dp;
+	enum pipe p;
 
-	for (i = PIPE_A; i <= PIPE_C; i++)
-		intel_dp->mst_encoders[i] = intel_dp_create_fake_mst_encoder(intel_dig_port, i);
+	for_each_pipe(dev_priv, p)
+		intel_dp->mst_encoders[p] =
+			intel_dp_create_fake_mst_encoder(intel_dig_port, p);
 	return true;
 }
 
-- 
2.14.1

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

Re: [PATCH v4 1/2] drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector

[Ausmus, James]

On Wed, Sep 27, 2017 at 11:29 AM, James Ausmus <james.ausmus@intel.com> wrote:
> Make intel_dp_add_mst_connector handle error returns from the drm_ calls.
> Add intel_connector_free to support cleanup on the error path.
>
> v2: Rename new function to avoid confusion, and simplify error
> paths (Ville)
>
> v3: Indentation fixup, style fixes (Ville)
>
> v4: Clarify usage of intel_connector_free, and fix usage of
> intel_connector_free


Ville - any additional issues you see with the latest spin?

Thanks!

-James


>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: James Ausmus <james.ausmus@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
>  drivers/gpu/drm/i915/intel_dp_mst.c  | 27 ++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  3 files changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c4b224a3a0ee..725014bf6f0f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6146,6 +6146,19 @@ struct intel_connector *intel_connector_alloc(void)
>         return connector;
>  }
>
> +/*
> + * Free the bits allocated by intel_connector_alloc.
> + * This should only be used after intel_connector_alloc has returned
> + * successfully, and before drm_connector_init returns successfully.
> + * Otherwise the destroy callbacks for the connector and the state should
> + * take care of proper cleanup/free
> + */
> +void intel_connector_free(struct intel_connector *connector)
> +{
> +       kfree(to_intel_digital_connector_state(connector->base.state));
> +       kfree(connector);
> +}
> +
>  /* Simple connector->get_hw_state implementation for encoders that support only
>   * one connector and no cloning and hence the encoder state determines the state
>   * of the connector. */
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 9a396f483f8b..8ceffad3e665 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -450,14 +450,20 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>         struct drm_device *dev = intel_dig_port->base.base.dev;
>         struct intel_connector *intel_connector;
>         struct drm_connector *connector;
> -       int i;
> +       int i, ret;
>
>         intel_connector = intel_connector_alloc();
>         if (!intel_connector)
>                 return NULL;
>
>         connector = &intel_connector->base;
> -       drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs, DRM_MODE_CONNECTOR_DisplayPort);
> +       ret = drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs,
> +                                DRM_MODE_CONNECTOR_DisplayPort);
> +       if (ret) {
> +               intel_connector_free(intel_connector);
> +               return NULL;
> +       }
> +
>         drm_connector_helper_add(connector, &intel_dp_mst_connector_helper_funcs);
>
>         intel_connector->get_hw_state = intel_dp_mst_get_hw_state;
> @@ -465,15 +471,26 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>         intel_connector->port = port;
>
>         for (i = PIPE_A; i <= PIPE_C; i++) {
> -               drm_mode_connector_attach_encoder(&intel_connector->base,
> -                                                 &intel_dp->mst_encoders[i]->base.base);
> +               struct drm_encoder *enc = &intel_dp->mst_encoders[i]->base.base;
> +
> +               ret = drm_mode_connector_attach_encoder(&intel_connector->base,
> +                                                       enc);
> +               if (ret)
> +                       goto err;
>         }
>
>         drm_object_attach_property(&connector->base, dev->mode_config.path_property, 0);
>         drm_object_attach_property(&connector->base, dev->mode_config.tile_property, 0);
>
> -       drm_mode_connector_set_path_property(connector, pathprop);
> +       ret = drm_mode_connector_set_path_property(connector, pathprop);
> +       if (ret)
> +               goto err;
> +
>         return connector;
> +
> +err:
> +       drm_connector_cleanup(connector);
> +       return NULL;
>  }
>
>  static void intel_dp_register_mst_connector(struct drm_connector *connector)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0cab667fff57..b549a0b45e57 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1360,6 +1360,7 @@ void intel_pps_unlock_regs_wa(struct drm_i915_private *dev_priv);
>  void intel_encoder_destroy(struct drm_encoder *encoder);
>  int intel_connector_init(struct intel_connector *);
>  struct intel_connector *intel_connector_alloc(void);
> +void intel_connector_free(struct intel_connector *connector);
>  bool intel_connector_get_hw_state(struct intel_connector *connector);
>  void intel_connector_attach_encoder(struct intel_connector *connector,
>                                     struct intel_encoder *encoder);
> --
> 2.14.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 


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

[PATCH v5] drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector

[James Ausmus]

Make intel_dp_add_mst_connector handle error returns from the drm_ calls.
Add intel_connector_free to support cleanup on the error path.

v2: Rename new function to avoid confusion, and simplify error
paths (Ville)

v3: Indentation fixup, style fixes (Ville)

v4: Clarify usage of intel_connector_free, and fix usage of
intel_connector_free

v5: Rebase

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: James Ausmus <james.ausmus@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
 drivers/gpu/drm/i915/intel_dp_mst.c  | 27 +++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 3 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 185be5726b5e..796ead425f23 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6139,6 +6139,19 @@ struct intel_connector *intel_connector_alloc(void)
 	return connector;
 }
 
+/*
+ * Free the bits allocated by intel_connector_alloc.
+ * This should only be used after intel_connector_alloc has returned
+ * successfully, and before drm_connector_init returns successfully.
+ * Otherwise the destroy callbacks for the connector and the state should
+ * take care of proper cleanup/free
+ */
+void intel_connector_free(struct intel_connector *connector)
+{
+	kfree(to_intel_digital_connector_state(connector->base.state));
+	kfree(connector);
+}
+
 /* Simple connector->get_hw_state implementation for encoders that support only
  * one connector and no cloning and hence the encoder state determines the state
  * of the connector. */
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index f7c782576162..772521440a9f 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -458,13 +458,20 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	struct intel_connector *intel_connector;
 	struct drm_connector *connector;
 	enum pipe pipe;
+	int ret;
 
 	intel_connector = intel_connector_alloc();
 	if (!intel_connector)
 		return NULL;
 
 	connector = &intel_connector->base;
-	drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs, DRM_MODE_CONNECTOR_DisplayPort);
+	ret = drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs,
+				 DRM_MODE_CONNECTOR_DisplayPort);
+	if (ret) {
+		intel_connector_free(intel_connector);
+		return NULL;
+	}
+
 	drm_connector_helper_add(connector, &intel_dp_mst_connector_helper_funcs);
 
 	intel_connector->get_hw_state = intel_dp_mst_get_hw_state;
@@ -472,15 +479,27 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	intel_connector->port = port;
 
 	for_each_pipe(dev_priv, pipe) {
-		drm_mode_connector_attach_encoder(&intel_connector->base,
-						  &intel_dp->mst_encoders[pipe]->base.base);
+		struct drm_encoder *enc =
+			&intel_dp->mst_encoders[pipe]->base.base;
+
+		ret = drm_mode_connector_attach_encoder(&intel_connector->base,
+							enc);
+		if (ret)
+			goto err;
 	}
 
 	drm_object_attach_property(&connector->base, dev->mode_config.path_property, 0);
 	drm_object_attach_property(&connector->base, dev->mode_config.tile_property, 0);
 
-	drm_mode_connector_set_path_property(connector, pathprop);
+	ret = drm_mode_connector_set_path_property(connector, pathprop);
+	if (ret)
+		goto err;
+
 	return connector;
+
+err:
+	drm_connector_cleanup(connector);
+	return NULL;
 }
 
 static void intel_dp_register_mst_connector(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d61985f93d40..f863a0ca39d9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1363,6 +1363,7 @@ void intel_pps_unlock_regs_wa(struct drm_i915_private *dev_priv);
 void intel_encoder_destroy(struct drm_encoder *encoder);
 int intel_connector_init(struct intel_connector *);
 struct intel_connector *intel_connector_alloc(void);
+void intel_connector_free(struct intel_connector *connector);
 bool intel_connector_get_hw_state(struct intel_connector *connector);
 void intel_connector_attach_encoder(struct intel_connector *connector,
 				    struct intel_encoder *encoder);
-- 
2.14.1

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

Re: [PATCH v5] drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector

[Ville Syrjälä]

On Fri, Oct 13, 2017 at 11:01:44AM -0700, James Ausmus wrote:
> Make intel_dp_add_mst_connector handle error returns from the drm_ calls.
> Add intel_connector_free to support cleanup on the error path.
> 
> v2: Rename new function to avoid confusion, and simplify error
> paths (Ville)
> 
> v3: Indentation fixup, style fixes (Ville)
> 
> v4: Clarify usage of intel_connector_free, and fix usage of
> intel_connector_free
> 
> v5: Rebase
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: James Ausmus <james.ausmus@intel.com>

Pushed to dinq. Thanks for the patch.

One slight concern that came to my mind is whether we'll leave sufficient
breadcrumbs into the log to see that we failed to add a connector that
was really meant to be there. We might want to add some error prints
for that.

> ---
>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
>  drivers/gpu/drm/i915/intel_dp_mst.c  | 27 +++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  3 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 185be5726b5e..796ead425f23 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6139,6 +6139,19 @@ struct intel_connector *intel_connector_alloc(void)
>  	return connector;
>  }
>  
> +/*
> + * Free the bits allocated by intel_connector_alloc.
> + * This should only be used after intel_connector_alloc has returned
> + * successfully, and before drm_connector_init returns successfully.
> + * Otherwise the destroy callbacks for the connector and the state should
> + * take care of proper cleanup/free
> + */
> +void intel_connector_free(struct intel_connector *connector)
> +{
> +	kfree(to_intel_digital_connector_state(connector->base.state));
> +	kfree(connector);
> +}
> +
>  /* Simple connector->get_hw_state implementation for encoders that support only
>   * one connector and no cloning and hence the encoder state determines the state
>   * of the connector. */
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index f7c782576162..772521440a9f 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -458,13 +458,20 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>  	struct intel_connector *intel_connector;
>  	struct drm_connector *connector;
>  	enum pipe pipe;
> +	int ret;
>  
>  	intel_connector = intel_connector_alloc();
>  	if (!intel_connector)
>  		return NULL;
>  
>  	connector = &intel_connector->base;
> -	drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs, DRM_MODE_CONNECTOR_DisplayPort);
> +	ret = drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs,
> +				 DRM_MODE_CONNECTOR_DisplayPort);
> +	if (ret) {
> +		intel_connector_free(intel_connector);
> +		return NULL;
> +	}
> +
>  	drm_connector_helper_add(connector, &intel_dp_mst_connector_helper_funcs);
>  
>  	intel_connector->get_hw_state = intel_dp_mst_get_hw_state;
> @@ -472,15 +479,27 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>  	intel_connector->port = port;
>  
>  	for_each_pipe(dev_priv, pipe) {
> -		drm_mode_connector_attach_encoder(&intel_connector->base,
> -						  &intel_dp->mst_encoders[pipe]->base.base);
> +		struct drm_encoder *enc =
> +			&intel_dp->mst_encoders[pipe]->base.base;
> +
> +		ret = drm_mode_connector_attach_encoder(&intel_connector->base,
> +							enc);
> +		if (ret)
> +			goto err;
>  	}
>  
>  	drm_object_attach_property(&connector->base, dev->mode_config.path_property, 0);
>  	drm_object_attach_property(&connector->base, dev->mode_config.tile_property, 0);
>  
> -	drm_mode_connector_set_path_property(connector, pathprop);
> +	ret = drm_mode_connector_set_path_property(connector, pathprop);
> +	if (ret)
> +		goto err;
> +
>  	return connector;
> +
> +err:
> +	drm_connector_cleanup(connector);
> +	return NULL;
>  }
>  
>  static void intel_dp_register_mst_connector(struct drm_connector *connector)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d61985f93d40..f863a0ca39d9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1363,6 +1363,7 @@ void intel_pps_unlock_regs_wa(struct drm_i915_private *dev_priv);
>  void intel_encoder_destroy(struct drm_encoder *encoder);
>  int intel_connector_init(struct intel_connector *);
>  struct intel_connector *intel_connector_alloc(void);
> +void intel_connector_free(struct intel_connector *connector);
>  bool intel_connector_get_hw_state(struct intel_connector *connector);
>  void intel_connector_attach_encoder(struct intel_connector *connector,
>  				    struct intel_encoder *encoder);
> -- 
> 2.14.1

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

Re: [PATCH v5] drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector

[James Ausmus]

On Tue, Oct 17, 2017 at 06:38:18PM +0300, Ville Syrjälä wrote:
> On Fri, Oct 13, 2017 at 11:01:44AM -0700, James Ausmus wrote:
> > Make intel_dp_add_mst_connector handle error returns from the drm_ calls.
> > Add intel_connector_free to support cleanup on the error path.
> > 
> > v2: Rename new function to avoid confusion, and simplify error
> > paths (Ville)
> > 
> > v3: Indentation fixup, style fixes (Ville)
> > 
> > v4: Clarify usage of intel_connector_free, and fix usage of
> > intel_connector_free
> > 
> > v5: Rebase
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: James Ausmus <james.ausmus@intel.com>
> 
> Pushed to dinq. Thanks for the patch.
> 
> One slight concern that came to my mind is whether we'll leave sufficient
> breadcrumbs into the log to see that we failed to add a connector that
> was really meant to be there. We might want to add some error prints
> for that.

Hmm - looking at the drm_dp_mst_topology_mgr bits, it looks like if
add_connector returns null, the port in question is just dropped
silently. I'll put adding some error messages on failure to my TODO
list. :)

Thanks!

-James

> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
> >  drivers/gpu/drm/i915/intel_dp_mst.c  | 27 +++++++++++++++++++++++----
> >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >  3 files changed, 37 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 185be5726b5e..796ead425f23 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6139,6 +6139,19 @@ struct intel_connector *intel_connector_alloc(void)
> >  	return connector;
> >  }
> >  
> > +/*
> > + * Free the bits allocated by intel_connector_alloc.
> > + * This should only be used after intel_connector_alloc has returned
> > + * successfully, and before drm_connector_init returns successfully.
> > + * Otherwise the destroy callbacks for the connector and the state should
> > + * take care of proper cleanup/free
> > + */
> > +void intel_connector_free(struct intel_connector *connector)
> > +{
> > +	kfree(to_intel_digital_connector_state(connector->base.state));
> > +	kfree(connector);
> > +}
> > +
> >  /* Simple connector->get_hw_state implementation for encoders that support only
> >   * one connector and no cloning and hence the encoder state determines the state
> >   * of the connector. */
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index f7c782576162..772521440a9f 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -458,13 +458,20 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
> >  	struct intel_connector *intel_connector;
> >  	struct drm_connector *connector;
> >  	enum pipe pipe;
> > +	int ret;
> >  
> >  	intel_connector = intel_connector_alloc();
> >  	if (!intel_connector)
> >  		return NULL;
> >  
> >  	connector = &intel_connector->base;
> > -	drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs, DRM_MODE_CONNECTOR_DisplayPort);
> > +	ret = drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs,
> > +				 DRM_MODE_CONNECTOR_DisplayPort);
> > +	if (ret) {
> > +		intel_connector_free(intel_connector);
> > +		return NULL;
> > +	}
> > +
> >  	drm_connector_helper_add(connector, &intel_dp_mst_connector_helper_funcs);
> >  
> >  	intel_connector->get_hw_state = intel_dp_mst_get_hw_state;
> > @@ -472,15 +479,27 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
> >  	intel_connector->port = port;
> >  
> >  	for_each_pipe(dev_priv, pipe) {
> > -		drm_mode_connector_attach_encoder(&intel_connector->base,
> > -						  &intel_dp->mst_encoders[pipe]->base.base);
> > +		struct drm_encoder *enc =
> > +			&intel_dp->mst_encoders[pipe]->base.base;
> > +
> > +		ret = drm_mode_connector_attach_encoder(&intel_connector->base,
> > +							enc);
> > +		if (ret)
> > +			goto err;
> >  	}
> >  
> >  	drm_object_attach_property(&connector->base, dev->mode_config.path_property, 0);
> >  	drm_object_attach_property(&connector->base, dev->mode_config.tile_property, 0);
> >  
> > -	drm_mode_connector_set_path_property(connector, pathprop);
> > +	ret = drm_mode_connector_set_path_property(connector, pathprop);
> > +	if (ret)
> > +		goto err;
> > +
> >  	return connector;
> > +
> > +err:
> > +	drm_connector_cleanup(connector);
> > +	return NULL;
> >  }
> >  
> >  static void intel_dp_register_mst_connector(struct drm_connector *connector)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index d61985f93d40..f863a0ca39d9 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1363,6 +1363,7 @@ void intel_pps_unlock_regs_wa(struct drm_i915_private *dev_priv);
> >  void intel_encoder_destroy(struct drm_encoder *encoder);
> >  int intel_connector_init(struct intel_connector *);
> >  struct intel_connector *intel_connector_alloc(void);
> > +void intel_connector_free(struct intel_connector *connector);
> >  bool intel_connector_get_hw_state(struct intel_connector *connector);
> >  void intel_connector_attach_encoder(struct intel_connector *connector,
> >  				    struct intel_encoder *encoder);
> > -- 
> > 2.14.1
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector (rev6)

[Patchwork]

== Series Details ==

Series: drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector (rev6)
URL   : https://patchwork.freedesktop.org/series/30384/
State : success

== Summary ==

Series 30384v6 drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector
https://patchwork.freedesktop.org/api/1.0/series/30384/revisions/6/mbox/

Test chamelium:
        Subgroup dp-crc-fast:
                fail       -> PASS       (fi-kbl-7500u) fdo#102514
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                pass       -> FAIL       (fi-gdg-551) fdo#102618
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-byt-n2820) fdo#101705

fdo#102514 https://bugs.freedesktop.org/show_bug.cgi?id=102514
fdo#102618 https://bugs.freedesktop.org/show_bug.cgi?id=102618
fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:455s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:471s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:389s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:579s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:285s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:521s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:523s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:550s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:522s
fi-cfl-s         total:289  pass:253  dwarn:4   dfail:0   fail:0   skip:32  time:560s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:428s
fi-gdg-551       total:289  pass:177  dwarn:1   dfail:0   fail:2   skip:109 time:281s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:599s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:438s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:461s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:501s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:473s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:504s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:488s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:598s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:667s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:462s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:662s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:533s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:560s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:475s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:586s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:426s

005c15a2795854ab64b6ce63dcb099d2eea4a889 drm-tip: 2017y-10m-13d-15h-39m-54s UTC integration manifest
eb62b4a30426 drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6031/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector (rev6)

[Patchwork]

== Series Details ==

Series: drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector (rev6)
URL   : https://patchwork.freedesktop.org/series/30384/
State : failure

== Summary ==

Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (shard-hsw)

shard-hsw        total:2553 pass:1440 dwarn:1   dfail:0   fail:9   skip:1103 time:9649s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6031/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.IGT: failure for drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector (rev6)

[Ausmus, James]

On Fri, Oct 13, 2017 at 8:35 PM, Patchwork
<patchwork@emeril.freedesktop.org> wrote:
> == Series Details ==
>
> Series: drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector (rev6)
> URL   : https://patchwork.freedesktop.org/series/30384/
> State : failure
>
> == Summary ==
>
> Test kms_flip:
>         Subgroup basic-flip-vs-wf_vblank:
>                 pass       -> FAIL       (shard-hsw)
>
> shard-hsw        total:2553 pass:1440 dwarn:1   dfail:0   fail:9   skip:1103 time:9649s
>
> == Logs ==
>
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6031/shards.html

I'm not seeing how this failure could relate to the patch involved -
am I missing something, or was this a fluke?

-James


-- 


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

Re: ✗ Fi.CI.IGT: failure for drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector (rev6)

[Ville Syrjälä]

On Mon, Oct 16, 2017 at 04:04:40PM -0700, Ausmus, James wrote:
> On Fri, Oct 13, 2017 at 8:35 PM, Patchwork
> <patchwork@emeril.freedesktop.org> wrote:
> > == Series Details ==
> >
> > Series: drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector (rev6)
> > URL   : https://patchwork.freedesktop.org/series/30384/
> > State : failure
> >
> > == Summary ==
> >
> > Test kms_flip:
> >         Subgroup basic-flip-vs-wf_vblank:
> >                 pass       -> FAIL       (shard-hsw)
> >
> > shard-hsw        total:2553 pass:1440 dwarn:1   dfail:0   fail:9   skip:1103 time:9649s
> >
> > == Logs ==
> >
> > For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6031/shards.html
> 
> I'm not seeing how this failure could relate to the patch involved -
> am I missing something, or was this a fluke?

Yeah, we just missed the target by one frame for some reason. Doesn't
look like that particular test has been flaky before though. But then
again the history for shard runs isn't very long so it's hard to say
for sure how frequent this might be.

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