Re: [PATCH v2] radeon: Make sure CS mutex is held across GPU reset.

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

 



On Tue, Nov 08, 2011 at 06:50:19PM +0100, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@xxxxxxx>
> 
> This was only the case if the GPU reset was triggered from the CS ioctl,
> otherwise other processes could happily enter the CS ioctl and wreak havoc
> during the GPU reset.
> 
> This is a little complicated because the GPU reset can be triggered from the
> CS ioctl, in which case we're already holding the mutex, or from other call
> paths, in which case we need to lock the mutex. AFAICT the mutex API doesn't
> allow recursive locking or finding out the mutex owner, so we need to handle
> this with helper functions which allow recursive locking from the same
> process.
> 
> Signed-off-by: Michel Dänzer <michel.daenzer@xxxxxxx>

Beside not so important comment below

Reviewed-by: Jerome Glisse <jglisse@xxxxxxxxxx>
> ---
> 
> v2: Use generic radeon_mutex_(un)lock helpers which allow recursive locking
> from the same process. Eliminates int vs. bool return type issue in v1, and
> maybe these helpers can be used similarly for more locks in the future.
> 
>  drivers/gpu/drm/radeon/radeon.h        |   45 +++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/radeon/radeon_cs.c     |   14 +++++-----
>  drivers/gpu/drm/radeon/radeon_device.c |   16 ++++++++---
>  3 files changed, 63 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> index c1e056b..671d5a5 100644
> --- a/drivers/gpu/drm/radeon/radeon.h
> +++ b/drivers/gpu/drm/radeon/radeon.h
> @@ -1151,6 +1151,48 @@ struct r700_vram_scratch {
>  	volatile uint32_t		*ptr;
>  };
>  
> +
> +/*
> + * Mutex which allows recursive locking from the same process.
> + */
> +struct radeon_mutex {
> +	struct mutex		mutex;
> +	struct task_struct	*owner;
> +	int			level;
> +};
> +
> +static inline void radeon_mutex_init(struct radeon_mutex *mutex)
> +{
> +	mutex_init(&mutex->mutex);
> +	mutex->owner = NULL;
> +	mutex->level = 0;
> +}
> +
> +static inline void radeon_mutex_lock(struct radeon_mutex *mutex)
> +{
> +	if (mutex_trylock(&mutex->mutex)) {
> +		/* The mutex was unlocked before, so it's ours now */
> +		mutex->owner = current;
> +	} else if (mutex->owner != current) {
> +		/* Another process locked the mutex, take it */
> +		mutex_lock(&mutex->mutex);
> +		mutex->owner = current;
> +	}
> +	/* Otherwise the mutex was already locked by this process */
> +
> +	mutex->level++;
> +}
> +
> +static inline void radeon_mutex_unlock(struct radeon_mutex *mutex)
> +{
> +	if (--mutex->level > 0)
> +		return;
> +
> +	mutex->owner = NULL;
> +	mutex_unlock(&mutex->mutex);
> +}
> +
> +
>  /*
>   * Core structure, functions and helpers.
>   */
> @@ -1206,7 +1248,7 @@ struct radeon_device {
>  	struct radeon_gem		gem;
>  	struct radeon_pm		pm;
>  	uint32_t			bios_scratch[RADEON_BIOS_NUM_SCRATCH];
> -	struct mutex			cs_mutex;
> +	struct radeon_mutex		cs_mutex;
>  	struct radeon_wb		wb;
>  	struct radeon_dummy_page	dummy_page;
>  	bool				gpu_lockup;
> @@ -1245,6 +1287,7 @@ struct radeon_device {
>  	struct radeon_i2c_chan *i2c_bus[RADEON_MAX_I2C_BUS];
>  };
>  
> +
Adding empty line ?
>  int radeon_device_init(struct radeon_device *rdev,
>  		       struct drm_device *ddev,
>  		       struct pci_dev *pdev,
> diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c
> index fae00c0..ccaa243 100644
> --- a/drivers/gpu/drm/radeon/radeon_cs.c
> +++ b/drivers/gpu/drm/radeon/radeon_cs.c
> @@ -222,7 +222,7 @@ int radeon_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>  	struct radeon_cs_chunk *ib_chunk;
>  	int r;
>  
> -	mutex_lock(&rdev->cs_mutex);
> +	radeon_mutex_lock(&rdev->cs_mutex);
>  	/* initialize parser */
>  	memset(&parser, 0, sizeof(struct radeon_cs_parser));
>  	parser.filp = filp;
> @@ -233,14 +233,14 @@ int radeon_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>  	if (r) {
>  		DRM_ERROR("Failed to initialize parser !\n");
>  		radeon_cs_parser_fini(&parser, r);
> -		mutex_unlock(&rdev->cs_mutex);
> +		radeon_mutex_unlock(&rdev->cs_mutex);
>  		return r;
>  	}
>  	r =  radeon_ib_get(rdev, &parser.ib);
>  	if (r) {
>  		DRM_ERROR("Failed to get ib !\n");
>  		radeon_cs_parser_fini(&parser, r);
> -		mutex_unlock(&rdev->cs_mutex);
> +		radeon_mutex_unlock(&rdev->cs_mutex);
>  		return r;
>  	}
>  	r = radeon_cs_parser_relocs(&parser);
> @@ -248,7 +248,7 @@ int radeon_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>  		if (r != -ERESTARTSYS)
>  			DRM_ERROR("Failed to parse relocation %d!\n", r);
>  		radeon_cs_parser_fini(&parser, r);
> -		mutex_unlock(&rdev->cs_mutex);
> +		radeon_mutex_unlock(&rdev->cs_mutex);
>  		return r;
>  	}
>  	/* Copy the packet into the IB, the parser will read from the
> @@ -260,14 +260,14 @@ int radeon_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>  	if (r || parser.parser_error) {
>  		DRM_ERROR("Invalid command stream !\n");
>  		radeon_cs_parser_fini(&parser, r);
> -		mutex_unlock(&rdev->cs_mutex);
> +		radeon_mutex_unlock(&rdev->cs_mutex);
>  		return r;
>  	}
>  	r = radeon_cs_finish_pages(&parser);
>  	if (r) {
>  		DRM_ERROR("Invalid command stream !\n");
>  		radeon_cs_parser_fini(&parser, r);
> -		mutex_unlock(&rdev->cs_mutex);
> +		radeon_mutex_unlock(&rdev->cs_mutex);
>  		return r;
>  	}
>  	r = radeon_ib_schedule(rdev, parser.ib);
> @@ -275,7 +275,7 @@ int radeon_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>  		DRM_ERROR("Failed to schedule IB !\n");
>  	}
>  	radeon_cs_parser_fini(&parser, r);
> -	mutex_unlock(&rdev->cs_mutex);
> +	radeon_mutex_unlock(&rdev->cs_mutex);
>  	return r;
>  }
>  
> diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
> index b51e157..b632d57 100644
> --- a/drivers/gpu/drm/radeon/radeon_device.c
> +++ b/drivers/gpu/drm/radeon/radeon_device.c
> @@ -716,7 +716,7 @@ int radeon_device_init(struct radeon_device *rdev,
>  
>  	/* mutex initialization are all done here so we
>  	 * can recall function without having locking issues */
> -	mutex_init(&rdev->cs_mutex);
> +	radeon_mutex_init(&rdev->cs_mutex);
>  	mutex_init(&rdev->ib_pool.mutex);
>  	mutex_init(&rdev->cp.mutex);
>  	mutex_init(&rdev->dc_hw_i2c_mutex);
> @@ -954,6 +954,9 @@ int radeon_gpu_reset(struct radeon_device *rdev)
>  	int r;
>  	int resched;
>  
> +	/* Prevent CS ioctl from interfering */
> +	radeon_mutex_lock(&rdev->cs_mutex);
> +
>  	radeon_save_bios_scratch_regs(rdev);
>  	/* block TTM */
>  	resched = ttm_bo_lock_delayed_workqueue(&rdev->mman.bdev);
> @@ -966,10 +969,15 @@ int radeon_gpu_reset(struct radeon_device *rdev)
>  		radeon_restore_bios_scratch_regs(rdev);
>  		drm_helper_resume_force_mode(rdev->ddev);
>  		ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched);
> -		return 0;
>  	}
> -	/* bad news, how to tell it to userspace ? */
> -	dev_info(rdev->dev, "GPU reset failed\n");
> +
> +	radeon_mutex_unlock(&rdev->cs_mutex);
> +
> +	if (r) {
> +		/* bad news, how to tell it to userspace ? */
> +		dev_info(rdev->dev, "GPU reset failed\n");
> +	}
> +
>  	return r;
>  }
>  
> -- 
> 1.7.7.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux