| Message ID | 20171013180144.15865-1-james.ausmus@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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);
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(-)