[PATCH v2] radeon: Make sure CS mutex is held across GPU reset.
Jerome Glisse
j.glisse at gmail.com
Wed Nov 9 12:26:16 PST 2011
On Tue, Nov 08, 2011 at 06:50:19PM +0100, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer at amd.com>
>
> 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 at amd.com>
Beside not so important comment below
Reviewed-by: Jerome Glisse <jglisse at redhat.com>
> ---
>
> 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list