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

[Chris Wilson]

On Fri, Nov 04, 2016 at 11:07:26PM +0200, Ville Syrjälä wrote:
> On Fri, Nov 04, 2016 at 08:48:21PM +, Chris Wilson wrote:
> > On Tue, Oct 25, 2016 at 06:58:02PM +0300, ville.syrj...@linux.intel.com 
> > wrote:
> > > From: Ville Syrjälä 
> > > 
> > > 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.
> > 
> > smatch spotted ERR_PTR(0)
> > 
> > > @@ -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);
> > 
> > Here...
> 
> This looks correct to me, but the cursor and sprite paths are clearly
> crap.

Brain had already turned off. Yes, it was the plane and cursor, I just
goofed in trimming.
-Chris

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

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

[Ville Syrjälä]

On Fri, Nov 04, 2016 at 08:48:21PM +, Chris Wilson wrote:
> On Tue, Oct 25, 2016 at 06:58:02PM +0300, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä 
> > 
> > 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.
> 
> smatch spotted ERR_PTR(0)
> 
> > @@ -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);
> 
> Here...

This looks correct to me, but the cursor and sprite paths are clearly
crap.

> 
> > 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);
> 
> and here.
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

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

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

[Chris Wilson]

On Tue, Oct 25, 2016 at 06:58:02PM +0300, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
> 
> 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.

smatch spotted ERR_PTR(0)

> @@ -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);

Here...

>   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);

and here.

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

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

[Daniel Vetter]

On Tue, Oct 25, 2016 at 06:58:02PM +0300, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
> 
> 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ä 

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

Reviewed-by: Daniel Vetter 
> ---
>  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(>base);
> - if (!state)
> + if (!state) {
> + ret = -ENOMEM;
>   goto fail;
> + }
> +
>   primary->base.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(>base, _plane_helper_funcs);
>  
> - return >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)
>  {
>