Re: [PATCH 3/4] drm/i915: Bail if plane/crtc init fails

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Oct 25, 2016 at 06:58:02PM +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> 
> Due to the plane->index not getting readjusted in drm_plane_cleanup(),
> we can't continue initialization of some plane/crtc init fails.
> Well, we sort of could I suppose if we left all initialized planes on
> the list, but that would expose those planes to userspace as well.
> 
> But for crtcs the situation is even worse since we assume that
> pipe==crtc index occasionally, so we can't really deal with a partially
> initialize set of crtcs.
> 
> So seems safest to just abort the entire thing if anything goes wrong.
> All the failure paths here are kmalloc()s anyway, so it seems unlikely
> we'd get very far if these start failing.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>

Some bikeshedding in this patch, but I like it. For patches 1-3:

Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      |   4 +-
>  drivers/gpu/drm/i915/i915_drv.h      |   2 +-
>  drivers/gpu/drm/i915/intel_display.c | 101 ++++++++++++++++++++++-------------
>  drivers/gpu/drm/i915/intel_drv.h     |   3 +-
>  drivers/gpu/drm/i915/intel_sprite.c  |   8 +--
>  5 files changed, 75 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index af3559d34328..0e393b5a988a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -592,7 +592,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  
>  	/* Important: The output setup functions called by modeset_init need
>  	 * working irqs for e.g. gmbus and dp aux transfers. */
> -	intel_modeset_init(dev);
> +	ret = intel_modeset_init(dev);
> +	if (ret)
> +		goto cleanup_irq;
>  
>  	intel_guc_init(dev);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7a621c74254e..a9308aeb2f1f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3700,7 +3700,7 @@ void intel_device_info_dump(struct drm_i915_private *dev_priv);
>  
>  /* modesetting */
>  extern void intel_modeset_init_hw(struct drm_device *dev);
> -extern void intel_modeset_init(struct drm_device *dev);
> +extern int intel_modeset_init(struct drm_device *dev);
>  extern void intel_modeset_gem_init(struct drm_device *dev);
>  extern void intel_modeset_cleanup(struct drm_device *dev);
>  extern int intel_connector_register(struct drm_connector *);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 244a0f59d8f7..d5a49d12dc88 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14975,9 +14975,6 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc,
>   */
>  void intel_plane_destroy(struct drm_plane *plane)
>  {
> -	if (!plane)
> -		return;
> -
>  	drm_plane_cleanup(plane);
>  	kfree(to_intel_plane(plane));
>  }
> @@ -14991,11 +14988,10 @@ const struct drm_plane_funcs intel_plane_funcs = {
>  	.atomic_set_property = intel_plane_atomic_set_property,
>  	.atomic_duplicate_state = intel_plane_duplicate_state,
>  	.atomic_destroy_state = intel_plane_destroy_state,
> -
>  };
>  
> -static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> -						    int pipe)
> +static struct intel_plane *
> +intel_primary_plane_create(struct drm_device *dev, enum pipe pipe)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *primary = NULL;
> @@ -15006,12 +15002,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  	int ret;
>  
>  	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> -	if (!primary)
> +	if (!primary) {
> +		ret = -ENOMEM;
>  		goto fail;
> +	}
>  
>  	state = intel_create_plane_state(&primary->base);
> -	if (!state)
> +	if (!state) {
> +		ret = -ENOMEM;
>  		goto fail;
> +	}
> +
>  	primary->base.state = &state->base;
>  
>  	primary->can_scale = false;
> @@ -15092,13 +15093,13 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  
>  	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
>  
> -	return &primary->base;
> +	return primary;
>  
>  fail:
>  	kfree(state);
>  	kfree(primary);
>  
> -	return NULL;
> +	return ERR_PTR(ret);
>  }
>  
>  static int
> @@ -15194,8 +15195,8 @@ intel_update_cursor_plane(struct drm_plane *plane,
>  	intel_crtc_update_cursor(crtc, state);
>  }
>  
> -static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> -						   int pipe)
> +static struct intel_plane *
> +intel_cursor_plane_create(struct drm_device *dev, enum pipe pipe)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *cursor = NULL;
> @@ -15203,12 +15204,17 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>  	int ret;
>  
>  	cursor = kzalloc(sizeof(*cursor), GFP_KERNEL);
> -	if (!cursor)
> +	if (!cursor) {
> +		ret = -ENOMEM;
>  		goto fail;
> +	}
>  
>  	state = intel_create_plane_state(&cursor->base);
> -	if (!state)
> +	if (!state) {
> +		ret = -ENOMEM;
>  		goto fail;
> +	}
> +
>  	cursor->base.state = &state->base;
>  
>  	cursor->can_scale = false;
> @@ -15240,13 +15246,13 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>  
>  	drm_plane_helper_add(&cursor->base, &intel_plane_helper_funcs);
>  
> -	return &cursor->base;
> +	return cursor;
>  
>  fail:
>  	kfree(state);
>  	kfree(cursor);
>  
> -	return NULL;
> +	return ERR_PTR(ret);
>  }
>  
>  static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc,
> @@ -15265,22 +15271,24 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr
>  	scaler_state->scaler_id = -1;
>  }
>  
> -static void intel_crtc_init(struct drm_device *dev, int pipe)
> +static int intel_crtc_init(struct drm_device *dev, enum pipe pipe)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc;
>  	struct intel_crtc_state *crtc_state = NULL;
> -	struct drm_plane *primary = NULL;
> -	struct drm_plane *cursor = NULL;
> +	struct intel_plane *primary = NULL;
> +	struct intel_plane *cursor = NULL;
>  	int sprite, ret;
>  
>  	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
> -	if (intel_crtc == NULL)
> -		return;
> +	if (!intel_crtc)
> +		return -ENOMEM;
>  
>  	crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
> -	if (!crtc_state)
> +	if (!crtc_state) {
> +		ret = -ENOMEM;
>  		goto fail;
> +	}
>  	intel_crtc->config = crtc_state;
>  	intel_crtc->base.state = &crtc_state->base;
>  	crtc_state->base.crtc = &intel_crtc->base;
> @@ -15296,22 +15304,30 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	}
>  
>  	primary = intel_primary_plane_create(dev, pipe);
> -	if (!primary)
> +	if (IS_ERR(primary)) {
> +		ret = PTR_ERR(primary);
>  		goto fail;
> +	}
>  
>  	for_each_sprite(dev_priv, pipe, sprite) {
> -		ret = intel_plane_init(dev, pipe, sprite);
> -		if (ret)
> -			DRM_DEBUG_KMS("pipe %c sprite %c init failed: %d\n",
> -				      pipe_name(pipe), sprite_name(pipe, sprite), ret);
> +		struct intel_plane *plane;
> +
> +		plane = intel_sprite_plane_create(dev, pipe, sprite);
> +		if (!plane) {
> +			ret = PTR_ERR(plane);
> +			goto fail;
> +		}
>  	}
>  
>  	cursor = intel_cursor_plane_create(dev, pipe);
> -	if (!cursor)
> +	if (!cursor) {
> +		ret = PTR_ERR(cursor);
>  		goto fail;
> +	}
>  
> -	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> -					cursor, &intel_crtc_funcs,
> +	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base,
> +					&primary->base, &cursor->base,
> +					&intel_crtc_funcs,
>  					"pipe %c", pipe_name(pipe));
>  	if (ret)
>  		goto fail;
> @@ -15343,13 +15359,18 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	intel_color_init(&intel_crtc->base);
>  
>  	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
> -	return;
> +
> +	return 0;
>  
>  fail:
> -	intel_plane_destroy(primary);
> -	intel_plane_destroy(cursor);
> +	/*
> +	 * drm_mode_config_cleanup() will free up any
> +	 * crtcs/planes already initialized.
> +	 */
>  	kfree(crtc_state);
>  	kfree(intel_crtc);
> +
> +	return ret;
>  }
>  
>  enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
> @@ -16426,7 +16447,7 @@ static void sanitize_watermarks(struct drm_device *dev)
>  	drm_modeset_acquire_fini(&ctx);
>  }
>  
> -void intel_modeset_init(struct drm_device *dev)
> +int intel_modeset_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> @@ -16450,7 +16471,7 @@ void intel_modeset_init(struct drm_device *dev)
>  	intel_init_pm(dev);
>  
>  	if (INTEL_INFO(dev)->num_pipes == 0)
> -		return;
> +		return 0;
>  
>  	/*
>  	 * There may be no VBT; and if the BIOS enabled SSC we can
> @@ -16499,7 +16520,13 @@ void intel_modeset_init(struct drm_device *dev)
>  		      INTEL_INFO(dev)->num_pipes > 1 ? "s" : "");
>  
>  	for_each_pipe(dev_priv, pipe) {
> -		intel_crtc_init(dev, pipe);
> +		int ret;
> +
> +		ret = intel_crtc_init(dev, pipe);
> +		if (ret) {
> +			drm_mode_config_cleanup(dev);
> +			return ret;
> +		}
>  	}
>  
>  	intel_update_czclk(dev_priv);
> @@ -16547,6 +16574,8 @@ void intel_modeset_init(struct drm_device *dev)
>  	 * since the watermark calculation done here will use pstate->fb.
>  	 */
>  	sanitize_watermarks(dev);
> +
> +	return 0;
>  }
>  
>  static void intel_enable_pipe_a(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 7dda79df55d0..4fd7d74fd6dc 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1779,7 +1779,8 @@ bool intel_sdvo_init(struct drm_device *dev,
>  /* intel_sprite.c */
>  int intel_usecs_to_scanlines(const struct drm_display_mode *adjusted_mode,
>  			     int usecs);
> -int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane);
> +struct intel_plane *intel_sprite_plane_create(struct drm_device *dev,
> +					      enum pipe pipe, int plane);
>  int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
>  			      struct drm_file *file_priv);
>  void intel_pipe_update_start(struct intel_crtc *crtc);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index ae1e3d47951b..41ae7f562eec 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1042,8 +1042,8 @@ static uint32_t skl_plane_formats[] = {
>  	DRM_FORMAT_VYUY,
>  };
>  
> -int
> -intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> +struct intel_plane *
> +intel_sprite_plane_create(struct drm_device *dev, enum pipe pipe, int plane)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *intel_plane = NULL;
> @@ -1160,11 +1160,11 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  
>  	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
>  
> -	return 0;
> +	return intel_plane;
>  
>  fail:
>  	kfree(state);
>  	kfree(intel_plane);
>  
> -	return ret;
> +	return ERR_PTR(ret);
>  }
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux