All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/amd/amdgpu: Make debugfs write compliment read
@ 2016-10-10 12:51 Tom St Denis
       [not found] ` <20161010125128.18303-1-tom.stdenis-5C7GfCeVMHo@public.gmane.org>
  0 siblings, 1 reply; 9+ messages in thread
From: Tom St Denis @ 2016-10-10 12:51 UTC (permalink / raw)
  To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW; +Cc: Tom St Denis

Add PG lock support as well as bank selection to
the MMIO write function.

Signed-off-by: Tom St Denis <tom.stdenis@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 43 ++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 04c4aee70452..a9d83c1d7a43 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2637,10 +2637,45 @@ static ssize_t amdgpu_debugfs_regs_write(struct file *f, const char __user *buf,
 	struct amdgpu_device *adev = f->f_inode->i_private;
 	ssize_t result = 0;
 	int r;
+	bool pm_pg_lock, use_bank;
+	unsigned instance_bank, sh_bank, se_bank;
 
 	if (size & 0x3 || *pos & 0x3)
 		return -EINVAL;
 
+	/* are we reading registers for which a PG lock is necessary? */
+	pm_pg_lock = (*pos >> 23) & 1;
+
+	if (*pos & (1ULL << 62)) {
+		se_bank = (*pos >> 24) & 0x3FF;
+		sh_bank = (*pos >> 34) & 0x3FF;
+		instance_bank = (*pos >> 44) & 0x3FF;
+
+		if (se_bank == 0x3FF)
+			se_bank = 0xFFFFFFFF;
+		if (sh_bank == 0x3FF)
+			sh_bank = 0xFFFFFFFF;
+		if (instance_bank == 0x3FF)
+			instance_bank = 0xFFFFFFFF;
+		use_bank = 1;
+	} else {
+		use_bank = 0;
+	}
+
+	*pos &= 0x3FFFF;
+
+	if (use_bank) {
+		if ((sh_bank != 0xFFFFFFFF && sh_bank >= adev->gfx.config.max_sh_per_se) ||
+		    (se_bank != 0xFFFFFFFF && se_bank >= adev->gfx.config.max_shader_engines))
+			return -EINVAL;
+		mutex_lock(&adev->grbm_idx_mutex);
+		amdgpu_gfx_select_se_sh(adev, se_bank,
+					sh_bank, instance_bank);
+	}
+
+	if (pm_pg_lock)
+		mutex_lock(&adev->pm.mutex);
+
 	while (size) {
 		uint32_t value;
 
@@ -2659,6 +2694,14 @@ static ssize_t amdgpu_debugfs_regs_write(struct file *f, const char __user *buf,
 		size -= 4;
 	}
 
+	if (use_bank) {
+		amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff, 0xffffffff);
+		mutex_unlock(&adev->grbm_idx_mutex);
+	}
+
+	if (pm_pg_lock)
+		mutex_unlock(&adev->pm.mutex);
+
 	return result;
 }
 
-- 
2.10.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/amd/amdgpu: Make debugfs write compliment read
       [not found] ` <20161010125128.18303-1-tom.stdenis-5C7GfCeVMHo@public.gmane.org>
@ 2016-10-10 13:25   ` Christian König
       [not found]     ` <91da2060-e6d4-70c9-2ceb-69218e1058ba-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
  0 siblings, 1 reply; 9+ messages in thread
From: Christian König @ 2016-10-10 13:25 UTC (permalink / raw)
  To: Tom St Denis, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW; +Cc: Tom St Denis

Mhm, that userspace controls if the lock is taken or not is a bit 
problematic, on the other hand only root could access that.

Christian.

Am 10.10.2016 um 14:51 schrieb Tom St Denis:
> Add PG lock support as well as bank selection to
> the MMIO write function.
>
> Signed-off-by: Tom St Denis <tom.stdenis@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 43 ++++++++++++++++++++++++++++++
>   1 file changed, 43 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 04c4aee70452..a9d83c1d7a43 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2637,10 +2637,45 @@ static ssize_t amdgpu_debugfs_regs_write(struct file *f, const char __user *buf,
>   	struct amdgpu_device *adev = f->f_inode->i_private;
>   	ssize_t result = 0;
>   	int r;
> +	bool pm_pg_lock, use_bank;
> +	unsigned instance_bank, sh_bank, se_bank;
>   
>   	if (size & 0x3 || *pos & 0x3)
>   		return -EINVAL;
>   
> +	/* are we reading registers for which a PG lock is necessary? */
> +	pm_pg_lock = (*pos >> 23) & 1;
> +
> +	if (*pos & (1ULL << 62)) {
> +		se_bank = (*pos >> 24) & 0x3FF;
> +		sh_bank = (*pos >> 34) & 0x3FF;
> +		instance_bank = (*pos >> 44) & 0x3FF;
> +
> +		if (se_bank == 0x3FF)
> +			se_bank = 0xFFFFFFFF;
> +		if (sh_bank == 0x3FF)
> +			sh_bank = 0xFFFFFFFF;
> +		if (instance_bank == 0x3FF)
> +			instance_bank = 0xFFFFFFFF;
> +		use_bank = 1;
> +	} else {
> +		use_bank = 0;
> +	}
> +
> +	*pos &= 0x3FFFF;
> +
> +	if (use_bank) {
> +		if ((sh_bank != 0xFFFFFFFF && sh_bank >= adev->gfx.config.max_sh_per_se) ||
> +		    (se_bank != 0xFFFFFFFF && se_bank >= adev->gfx.config.max_shader_engines))
> +			return -EINVAL;
> +		mutex_lock(&adev->grbm_idx_mutex);
> +		amdgpu_gfx_select_se_sh(adev, se_bank,
> +					sh_bank, instance_bank);
> +	}
> +
> +	if (pm_pg_lock)
> +		mutex_lock(&adev->pm.mutex);
> +
>   	while (size) {
>   		uint32_t value;
>   
> @@ -2659,6 +2694,14 @@ static ssize_t amdgpu_debugfs_regs_write(struct file *f, const char __user *buf,
>   		size -= 4;
>   	}
>   
> +	if (use_bank) {
> +		amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff, 0xffffffff);
> +		mutex_unlock(&adev->grbm_idx_mutex);
> +	}
> +
> +	if (pm_pg_lock)
> +		mutex_unlock(&adev->pm.mutex);
> +
>   	return result;
>   }
>   


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/amd/amdgpu: Make debugfs write compliment read
       [not found]     ` <91da2060-e6d4-70c9-2ceb-69218e1058ba-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
@ 2016-10-10 13:38       ` Tom St Denis
       [not found]         ` <CAAzXoRJKusZrvv7obpZ-0tR39pVmdc-7Y8prKCsRC6bchmsOcA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 9+ messages in thread
From: Tom St Denis @ 2016-10-10 13:38 UTC (permalink / raw)
  To: Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
  Cc: Tom St Denis


[-- Attachment #1.1: Type: text/plain, Size: 3219 bytes --]

that's kind of the point.  Its for debugging only.  Also reads can already
lock

On Mon, Oct 10, 2016, 09:25 Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org> wrote:

> Mhm, that userspace controls if the lock is taken or not is a bit
> problematic, on the other hand only root could access that.
>
> Christian.
>
> Am 10.10.2016 um 14:51 schrieb Tom St Denis:
> > Add PG lock support as well as bank selection to
> > the MMIO write function.
> >
> > Signed-off-by: Tom St Denis <tom.stdenis-5C7GfCeVMHo@public.gmane.org>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 43
> ++++++++++++++++++++++++++++++
> >   1 file changed, 43 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 04c4aee70452..a9d83c1d7a43 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -2637,10 +2637,45 @@ static ssize_t amdgpu_debugfs_regs_write(struct
> file *f, const char __user *buf,
> >       struct amdgpu_device *adev = f->f_inode->i_private;
> >       ssize_t result = 0;
> >       int r;
> > +     bool pm_pg_lock, use_bank;
> > +     unsigned instance_bank, sh_bank, se_bank;
> >
> >       if (size & 0x3 || *pos & 0x3)
> >               return -EINVAL;
> >
> > +     /* are we reading registers for which a PG lock is necessary? */
> > +     pm_pg_lock = (*pos >> 23) & 1;
> > +
> > +     if (*pos & (1ULL << 62)) {
> > +             se_bank = (*pos >> 24) & 0x3FF;
> > +             sh_bank = (*pos >> 34) & 0x3FF;
> > +             instance_bank = (*pos >> 44) & 0x3FF;
> > +
> > +             if (se_bank == 0x3FF)
> > +                     se_bank = 0xFFFFFFFF;
> > +             if (sh_bank == 0x3FF)
> > +                     sh_bank = 0xFFFFFFFF;
> > +             if (instance_bank == 0x3FF)
> > +                     instance_bank = 0xFFFFFFFF;
> > +             use_bank = 1;
> > +     } else {
> > +             use_bank = 0;
> > +     }
> > +
> > +     *pos &= 0x3FFFF;
> > +
> > +     if (use_bank) {
> > +             if ((sh_bank != 0xFFFFFFFF && sh_bank >=
> adev->gfx.config.max_sh_per_se) ||
> > +                 (se_bank != 0xFFFFFFFF && se_bank >=
> adev->gfx.config.max_shader_engines))
> > +                     return -EINVAL;
> > +             mutex_lock(&adev->grbm_idx_mutex);
> > +             amdgpu_gfx_select_se_sh(adev, se_bank,
> > +                                     sh_bank, instance_bank);
> > +     }
> > +
> > +     if (pm_pg_lock)
> > +             mutex_lock(&adev->pm.mutex);
> > +
> >       while (size) {
> >               uint32_t value;
> >
> > @@ -2659,6 +2694,14 @@ static ssize_t amdgpu_debugfs_regs_write(struct
> file *f, const char __user *buf,
> >               size -= 4;
> >       }
> >
> > +     if (use_bank) {
> > +             amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff,
> 0xffffffff);
> > +             mutex_unlock(&adev->grbm_idx_mutex);
> > +     }
> > +
> > +     if (pm_pg_lock)
> > +             mutex_unlock(&adev->pm.mutex);
> > +
> >       return result;
> >   }
> >
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 5813 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/amd/amdgpu: Make debugfs write compliment read
       [not found]         ` <CAAzXoRJKusZrvv7obpZ-0tR39pVmdc-7Y8prKCsRC6bchmsOcA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-10-10 13:49           ` Christian König
       [not found]             ` <3c1d969b-0141-ef29-1e0f-5267d8a7cae9-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
  0 siblings, 1 reply; 9+ messages in thread
From: Christian König @ 2016-10-10 13:49 UTC (permalink / raw)
  To: Tom St Denis, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW; +Cc: Tom St Denis


[-- Attachment #1.1: Type: text/plain, Size: 3831 bytes --]

Would it hurt us to always take the looks (both the pm as well as grbm 
idx lock)?

I don't think that would be much of an issue and I would have a better 
feeling with that.

Christian.

Am 10.10.2016 um 15:38 schrieb Tom St Denis:
>
> that's kind of the point.  Its for debugging only. Also reads can 
> already lock
>
>
> On Mon, Oct 10, 2016, 09:25 Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org 
> <mailto:deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>> wrote:
>
>     Mhm, that userspace controls if the lock is taken or not is a bit
>     problematic, on the other hand only root could access that.
>
>     Christian.
>
>     Am 10.10.2016 um 14:51 schrieb Tom St Denis:
>     > Add PG lock support as well as bank selection to
>     > the MMIO write function.
>     >
>     > Signed-off-by: Tom St Denis <tom.stdenis-5C7GfCeVMHo@public.gmane.org
>     <mailto:tom.stdenis-5C7GfCeVMHo@public.gmane.org>>
>     > ---
>     >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 43
>     ++++++++++++++++++++++++++++++
>     >   1 file changed, 43 insertions(+)
>     >
>     > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>     b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>     > index 04c4aee70452..a9d83c1d7a43 100644
>     > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>     > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>     > @@ -2637,10 +2637,45 @@ static ssize_t
>     amdgpu_debugfs_regs_write(struct file *f, const char __user *buf,
>     >       struct amdgpu_device *adev = f->f_inode->i_private;
>     >       ssize_t result = 0;
>     >       int r;
>     > +     bool pm_pg_lock, use_bank;
>     > +     unsigned instance_bank, sh_bank, se_bank;
>     >
>     >       if (size & 0x3 || *pos & 0x3)
>     >               return -EINVAL;
>     >
>     > +     /* are we reading registers for which a PG lock is
>     necessary? */
>     > +     pm_pg_lock = (*pos >> 23) & 1;
>     > +
>     > +     if (*pos & (1ULL << 62)) {
>     > +             se_bank = (*pos >> 24) & 0x3FF;
>     > +             sh_bank = (*pos >> 34) & 0x3FF;
>     > +             instance_bank = (*pos >> 44) & 0x3FF;
>     > +
>     > +             if (se_bank == 0x3FF)
>     > +                     se_bank = 0xFFFFFFFF;
>     > +             if (sh_bank == 0x3FF)
>     > +                     sh_bank = 0xFFFFFFFF;
>     > +             if (instance_bank == 0x3FF)
>     > +                     instance_bank = 0xFFFFFFFF;
>     > +             use_bank = 1;
>     > +     } else {
>     > +             use_bank = 0;
>     > +     }
>     > +
>     > +     *pos &= 0x3FFFF;
>     > +
>     > +     if (use_bank) {
>     > +             if ((sh_bank != 0xFFFFFFFF && sh_bank >=
>     adev->gfx.config.max_sh_per_se) ||
>     > +                 (se_bank != 0xFFFFFFFF && se_bank >=
>     adev->gfx.config.max_shader_engines))
>     > +                     return -EINVAL;
>     > +             mutex_lock(&adev->grbm_idx_mutex);
>     > +             amdgpu_gfx_select_se_sh(adev, se_bank,
>     > +                                     sh_bank, instance_bank);
>     > +     }
>     > +
>     > +     if (pm_pg_lock)
>     > +             mutex_lock(&adev->pm.mutex);
>     > +
>     >       while (size) {
>     >               uint32_t value;
>     >
>     > @@ -2659,6 +2694,14 @@ static ssize_t
>     amdgpu_debugfs_regs_write(struct file *f, const char __user *buf,
>     >               size -= 4;
>     >       }
>     >
>     > +     if (use_bank) {
>     > +             amdgpu_gfx_select_se_sh(adev, 0xffffffff,
>     0xffffffff, 0xffffffff);
>     > +             mutex_unlock(&adev->grbm_idx_mutex);
>     > +     }
>     > +
>     > +     if (pm_pg_lock)
>     > +             mutex_unlock(&adev->pm.mutex);
>     > +
>     >       return result;
>     >   }
>     >
>
>


[-- Attachment #1.2: Type: text/html, Size: 7847 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/amd/amdgpu: Make debugfs write compliment read
       [not found]             ` <3c1d969b-0141-ef29-1e0f-5267d8a7cae9-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
@ 2016-10-10 13:52               ` StDenis, Tom
  2016-10-10 20:21               ` StDenis, Tom
  1 sibling, 0 replies; 9+ messages in thread
From: StDenis, Tom @ 2016-10-10 13:52 UTC (permalink / raw)
  To: Christian König, Tom St Denis,
	amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW


[-- Attachment #1.1: Type: text/plain, Size: 3880 bytes --]

The pg lock is needed to read pgfsm registers so you wouldn't normally take grbm too.

Tom



-------- Original message --------
From: Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
Date: 2016-10-10 09:49 (GMT-05:00)
To: Tom St Denis <tstdenis82-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Cc: "StDenis, Tom" <Tom.StDenis-5C7GfCeVMHo@public.gmane.org>
Subject: Re: [PATCH] drm/amd/amdgpu: Make debugfs write compliment read

Would it hurt us to always take the looks (both the pm as well as grbm idx lock)?

I don't think that would be much of an issue and I would have a better feeling with that.

Christian.

Am 10.10.2016 um 15:38 schrieb Tom St Denis:

that's kind of the point.  Its for debugging only.  Also reads can already lock

On Mon, Oct 10, 2016, 09:25 Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org<mailto:deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>> wrote:
Mhm, that userspace controls if the lock is taken or not is a bit
problematic, on the other hand only root could access that.

Christian.

Am 10.10.2016 um 14:51 schrieb Tom St Denis:
> Add PG lock support as well as bank selection to
> the MMIO write function.
>
> Signed-off-by: Tom St Denis <tom.stdenis-5C7GfCeVMHo@public.gmane.org<mailto:tom.stdenis@amd.com>>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 43 ++++++++++++++++++++++++++++++
>   1 file changed, 43 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 04c4aee70452..a9d83c1d7a43 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2637,10 +2637,45 @@ static ssize_t amdgpu_debugfs_regs_write(struct file *f, const char __user *buf,
>       struct amdgpu_device *adev = f->f_inode->i_private;
>       ssize_t result = 0;
>       int r;
> +     bool pm_pg_lock, use_bank;
> +     unsigned instance_bank, sh_bank, se_bank;
>
>       if (size & 0x3 || *pos & 0x3)
>               return -EINVAL;
>
> +     /* are we reading registers for which a PG lock is necessary? */
> +     pm_pg_lock = (*pos >> 23) & 1;
> +
> +     if (*pos & (1ULL << 62)) {
> +             se_bank = (*pos >> 24) & 0x3FF;
> +             sh_bank = (*pos >> 34) & 0x3FF;
> +             instance_bank = (*pos >> 44) & 0x3FF;
> +
> +             if (se_bank == 0x3FF)
> +                     se_bank = 0xFFFFFFFF;
> +             if (sh_bank == 0x3FF)
> +                     sh_bank = 0xFFFFFFFF;
> +             if (instance_bank == 0x3FF)
> +                     instance_bank = 0xFFFFFFFF;
> +             use_bank = 1;
> +     } else {
> +             use_bank = 0;
> +     }
> +
> +     *pos &= 0x3FFFF;
> +
> +     if (use_bank) {
> +             if ((sh_bank != 0xFFFFFFFF && sh_bank >= adev->gfx.config.max_sh_per_se) ||
> +                 (se_bank != 0xFFFFFFFF && se_bank >= adev->gfx.config.max_shader_engines))
> +                     return -EINVAL;
> +             mutex_lock(&adev->grbm_idx_mutex);
> +             amdgpu_gfx_select_se_sh(adev, se_bank,
> +                                     sh_bank, instance_bank);
> +     }
> +
> +     if (pm_pg_lock)
> +             mutex_lock(&adev->pm.mutex);
> +
>       while (size) {
>               uint32_t value;
>
> @@ -2659,6 +2694,14 @@ static ssize_t amdgpu_debugfs_regs_write(struct file *f, const char __user *buf,
>               size -= 4;
>       }
>
> +     if (use_bank) {
> +             amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff, 0xffffffff);
> +             mutex_unlock(&adev->grbm_idx_mutex);
> +     }
> +
> +     if (pm_pg_lock)
> +             mutex_unlock(&adev->pm.mutex);
> +
>       return result;
>   }
>




[-- Attachment #1.2: Type: text/html, Size: 8533 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/amd/amdgpu: Make debugfs write compliment read
       [not found]             ` <3c1d969b-0141-ef29-1e0f-5267d8a7cae9-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
  2016-10-10 13:52               ` StDenis, Tom
@ 2016-10-10 20:21               ` StDenis, Tom
       [not found]                 ` <CY4PR12MB1768813E554E6DB1A40F5848F7DB0-rpdhrqHFk06yjjPBNVDk/QdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
  1 sibling, 1 reply; 9+ messages in thread
From: StDenis, Tom @ 2016-10-10 20:21 UTC (permalink / raw)
  To: Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW


[-- Attachment #1.1: Type: text/plain, Size: 3931 bytes --]

The only downside to taking the locks all the time is for 99% of debug reads it's unnecessary and in theory could lead to performance/behaviour problems.


I've been using this on read for a while now and haven't noticed a problem.


Tom


________________________________
From: Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
Sent: Monday, October 10, 2016 09:49
To: Tom St Denis; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Cc: StDenis, Tom
Subject: Re: [PATCH] drm/amd/amdgpu: Make debugfs write compliment read

Would it hurt us to always take the looks (both the pm as well as grbm idx lock)?

I don't think that would be much of an issue and I would have a better feeling with that.

Christian.

Am 10.10.2016 um 15:38 schrieb Tom St Denis:

that's kind of the point.  Its for debugging only.  Also reads can already lock

On Mon, Oct 10, 2016, 09:25 Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org<mailto:deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>> wrote:
Mhm, that userspace controls if the lock is taken or not is a bit
problematic, on the other hand only root could access that.

Christian.

Am 10.10.2016 um 14:51 schrieb Tom St Denis:
> Add PG lock support as well as bank selection to
> the MMIO write function.
>
> Signed-off-by: Tom St Denis <tom.stdenis-5C7GfCeVMHo@public.gmane.org<mailto:tom.stdenis@amd.com>>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 43 ++++++++++++++++++++++++++++++
>   1 file changed, 43 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 04c4aee70452..a9d83c1d7a43 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2637,10 +2637,45 @@ static ssize_t amdgpu_debugfs_regs_write(struct file *f, const char __user *buf,
>       struct amdgpu_device *adev = f->f_inode->i_private;
>       ssize_t result = 0;
>       int r;
> +     bool pm_pg_lock, use_bank;
> +     unsigned instance_bank, sh_bank, se_bank;
>
>       if (size & 0x3 || *pos & 0x3)
>               return -EINVAL;
>
> +     /* are we reading registers for which a PG lock is necessary? */
> +     pm_pg_lock = (*pos >> 23) & 1;
> +
> +     if (*pos & (1ULL << 62)) {
> +             se_bank = (*pos >> 24) & 0x3FF;
> +             sh_bank = (*pos >> 34) & 0x3FF;
> +             instance_bank = (*pos >> 44) & 0x3FF;
> +
> +             if (se_bank == 0x3FF)
> +                     se_bank = 0xFFFFFFFF;
> +             if (sh_bank == 0x3FF)
> +                     sh_bank = 0xFFFFFFFF;
> +             if (instance_bank == 0x3FF)
> +                     instance_bank = 0xFFFFFFFF;
> +             use_bank = 1;
> +     } else {
> +             use_bank = 0;
> +     }
> +
> +     *pos &= 0x3FFFF;
> +
> +     if (use_bank) {
> +             if ((sh_bank != 0xFFFFFFFF && sh_bank >= adev->gfx.config.max_sh_per_se) ||
> +                 (se_bank != 0xFFFFFFFF && se_bank >= adev->gfx.config.max_shader_engines))
> +                     return -EINVAL;
> +             mutex_lock(&adev->grbm_idx_mutex);
> +             amdgpu_gfx_select_se_sh(adev, se_bank,
> +                                     sh_bank, instance_bank);
> +     }
> +
> +     if (pm_pg_lock)
> +             mutex_lock(&adev->pm.mutex);
> +
>       while (size) {
>               uint32_t value;
>
> @@ -2659,6 +2694,14 @@ static ssize_t amdgpu_debugfs_regs_write(struct file *f, const char __user *buf,
>               size -= 4;
>       }
>
> +     if (use_bank) {
> +             amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff, 0xffffffff);
> +             mutex_unlock(&adev->grbm_idx_mutex);
> +     }
> +
> +     if (pm_pg_lock)
> +             mutex_unlock(&adev->pm.mutex);
> +
>       return result;
>   }
>




[-- Attachment #1.2: Type: text/html, Size: 8881 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/amd/amdgpu: Make debugfs write compliment read
       [not found]                 ` <CY4PR12MB1768813E554E6DB1A40F5848F7DB0-rpdhrqHFk06yjjPBNVDk/QdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
@ 2016-10-11 12:11                   ` Christian König
       [not found]                     ` <50dc12b3-a6bd-ed35-ac8f-e55909d184d2-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
  0 siblings, 1 reply; 9+ messages in thread
From: Christian König @ 2016-10-11 12:11 UTC (permalink / raw)
  To: StDenis, Tom, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW


[-- Attachment #1.1: Type: text/plain, Size: 5314 bytes --]

> The only downside to taking the locks all the time is for 99% of debug 
> reads it's unnecessary and in theory could lead to 
> performance/behaviour problems.
>
Yeah, but performance isn't critical here and taking two locks compared 
to the rest of what we (IOCTL etc...) do is completely negligibly.

> I've been using this on read for a while now and haven't noticed a 
> problem.
>
My concern is rather that somebody tries to write to the PM regs on 
accident or purpose without taking the lock.

See, the kernel is usually responsible to keep people from doing stupid 
things with the hardware, even if those people are root and could do 
stupid things anyway. Well, you know what I mean?

Regards,
Christian.

Am 10.10.2016 um 22:21 schrieb StDenis, Tom:
>
> The only downside to taking the locks all the time is for 99% of debug 
> reads it's unnecessary and in theory could lead to 
> performance/behaviour problems.
>
>
> I've been using this on read for a while now and haven't noticed a 
> problem.
>
>
> Tom
>
>
>
> ------------------------------------------------------------------------
> *From:* Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
> *Sent:* Monday, October 10, 2016 09:49
> *To:* Tom St Denis; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> *Cc:* StDenis, Tom
> *Subject:* Re: [PATCH] drm/amd/amdgpu: Make debugfs write compliment read
> Would it hurt us to always take the looks (both the pm as well as grbm 
> idx lock)?
>
> I don't think that would be much of an issue and I would have a better 
> feeling with that.
>
> Christian.
>
> Am 10.10.2016 um 15:38 schrieb Tom St Denis:
>>
>> that's kind of the point.  Its for debugging only.  Also reads can 
>> already lock
>>
>>
>> On Mon, Oct 10, 2016, 09:25 Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org 
>> <mailto:deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>> wrote:
>>
>>     Mhm, that userspace controls if the lock is taken or not is a bit
>>     problematic, on the other hand only root could access that.
>>
>>     Christian.
>>
>>     Am 10.10.2016 um 14:51 schrieb Tom St Denis:
>>     > Add PG lock support as well as bank selection to
>>     > the MMIO write function.
>>     >
>>     > Signed-off-by: Tom St Denis <tom.stdenis-5C7GfCeVMHo@public.gmane.org
>>     <mailto:tom.stdenis-5C7GfCeVMHo@public.gmane.org>>
>>     > ---
>>     >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 43
>>     ++++++++++++++++++++++++++++++
>>     >   1 file changed, 43 insertions(+)
>>     >
>>     > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>     b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>     > index 04c4aee70452..a9d83c1d7a43 100644
>>     > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>     > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>     > @@ -2637,10 +2637,45 @@ static ssize_t
>>     amdgpu_debugfs_regs_write(struct file *f, const char __user *buf,
>>     >       struct amdgpu_device *adev = f->f_inode->i_private;
>>     >       ssize_t result = 0;
>>     >       int r;
>>     > +     bool pm_pg_lock, use_bank;
>>     > +     unsigned instance_bank, sh_bank, se_bank;
>>     >
>>     >       if (size & 0x3 || *pos & 0x3)
>>     >               return -EINVAL;
>>     >
>>     > +     /* are we reading registers for which a PG lock is
>>     necessary? */
>>     > +     pm_pg_lock = (*pos >> 23) & 1;
>>     > +
>>     > +     if (*pos & (1ULL << 62)) {
>>     > +             se_bank = (*pos >> 24) & 0x3FF;
>>     > +             sh_bank = (*pos >> 34) & 0x3FF;
>>     > +             instance_bank = (*pos >> 44) & 0x3FF;
>>     > +
>>     > +             if (se_bank == 0x3FF)
>>     > +                     se_bank = 0xFFFFFFFF;
>>     > +             if (sh_bank == 0x3FF)
>>     > +                     sh_bank = 0xFFFFFFFF;
>>     > +             if (instance_bank == 0x3FF)
>>     > +                     instance_bank = 0xFFFFFFFF;
>>     > +             use_bank = 1;
>>     > +     } else {
>>     > +             use_bank = 0;
>>     > +     }
>>     > +
>>     > +     *pos &= 0x3FFFF;
>>     > +
>>     > +     if (use_bank) {
>>     > +             if ((sh_bank != 0xFFFFFFFF && sh_bank >=
>>     adev->gfx.config.max_sh_per_se) ||
>>     > +                 (se_bank != 0xFFFFFFFF && se_bank >=
>>     adev->gfx.config.max_shader_engines))
>>     > +                     return -EINVAL;
>>     > +  mutex_lock(&adev->grbm_idx_mutex);
>>     > +             amdgpu_gfx_select_se_sh(adev, se_bank,
>>     > +                                     sh_bank, instance_bank);
>>     > +     }
>>     > +
>>     > +     if (pm_pg_lock)
>>     > +             mutex_lock(&adev->pm.mutex);
>>     > +
>>     >       while (size) {
>>     >               uint32_t value;
>>     >
>>     > @@ -2659,6 +2694,14 @@ static ssize_t
>>     amdgpu_debugfs_regs_write(struct file *f, const char __user *buf,
>>     >               size -= 4;
>>     >       }
>>     >
>>     > +     if (use_bank) {
>>     > +             amdgpu_gfx_select_se_sh(adev, 0xffffffff,
>>     0xffffffff, 0xffffffff);
>>     > +  mutex_unlock(&adev->grbm_idx_mutex);
>>     > +     }
>>     > +
>>     > +     if (pm_pg_lock)
>>     > +  mutex_unlock(&adev->pm.mutex);
>>     > +
>>     >       return result;
>>     >   }
>>     >
>>
>>
>


[-- Attachment #1.2: Type: text/html, Size: 11911 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/amd/amdgpu: Make debugfs write compliment read
       [not found]                     ` <50dc12b3-a6bd-ed35-ac8f-e55909d184d2-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
@ 2016-10-11 12:19                       ` StDenis, Tom
       [not found]                         ` <CY4PR12MB1768134851FB1F505F746843F7DA0-rpdhrqHFk06yjjPBNVDk/QdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
  0 siblings, 1 reply; 9+ messages in thread
From: StDenis, Tom @ 2016-10-11 12:19 UTC (permalink / raw)
  To: Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW


[-- Attachment #1.1: Type: text/plain, Size: 6686 bytes --]

Hi Christian,


I agree with the crux of your argument but you can crash the system with carefully timed read/writes from the GFX registers, or MC, or ... That's why the entries are root only.


The issue with locking excessively is you might change behaviour of something you're trying to debug because the kernel then has to wait on the lock (or you're waiting on the lock).  For instance, suppose you want to read DCE registers and the kernel is doing some gfx/grbm related your DCE read will either block the GFX activity or you're waiting on it which means the behaviour of the system could be different than from the case where you're not probing the registers.


The goal is to be as unobtrusive as possible.  Heck I often mmap the BAR so I can read MMIO registers directly and not waste CPU cycles on syscalls.

The PM lock is because reading PGFSM registers during a powerup/down results in a GPU hang, and we only take the GRBM lock when users want to read from specific banks.  For any other case we don't need a lock.  In the kernel they should be mutually exclusive.  The PM lock is only taken in PP when performing a transition (or initializing the core) and I have yet to see a place where PP/PM related code also grabs the grbm index lock.

I don't have strong feelings against your proposal but I do prefer not to take the locks when I don't need to.  Ultimately I'll leave the decision up to you and Alex.  I'd like to get the 2nd patch through though as it makes the write side of the mmio reg complimentary (e.g. bank selection/pm lock).

Cheers,
Tom

________________________________
From: Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
Sent: Tuesday, October 11, 2016 08:11
To: StDenis, Tom; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH] drm/amd/amdgpu: Make debugfs write compliment read


The only downside to taking the locks all the time is for 99% of debug reads it's unnecessary and in theory could lead to performance/behaviour problems.

Yeah, but performance isn't critical here and taking two locks compared to the rest of what we (IOCTL etc...) do is completely negligibly.


I've been using this on read for a while now and haven't noticed a problem.

My concern is rather that somebody tries to write to the PM regs on accident or purpose without taking the lock.

See, the kernel is usually responsible to keep people from doing stupid things with the hardware, even if those people are root and could do stupid things anyway. Well, you know what I mean?

Regards,
Christian.

Am 10.10.2016 um 22:21 schrieb StDenis, Tom:

The only downside to taking the locks all the time is for 99% of debug reads it's unnecessary and in theory could lead to performance/behaviour problems.


I've been using this on read for a while now and haven't noticed a problem.


Tom


________________________________
From: Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org><mailto:deathsimple@vodafone.de>
Sent: Monday, October 10, 2016 09:49
To: Tom St Denis; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org<mailto:amd-gfx-PD4FTy7X32lMiVNPc3mojA@public.gmane.orgsktop.org>
Cc: StDenis, Tom
Subject: Re: [PATCH] drm/amd/amdgpu: Make debugfs write compliment read

Would it hurt us to always take the looks (both the pm as well as grbm idx lock)?

I don't think that would be much of an issue and I would have a better feeling with that.

Christian.

Am 10.10.2016 um 15:38 schrieb Tom St Denis:

that's kind of the point.  Its for debugging only.  Also reads can already lock

On Mon, Oct 10, 2016, 09:25 Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org<mailto:deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>> wrote:
Mhm, that userspace controls if the lock is taken or not is a bit
problematic, on the other hand only root could access that.

Christian.

Am 10.10.2016 um 14:51 schrieb Tom St Denis:
> Add PG lock support as well as bank selection to
> the MMIO write function.
>
> Signed-off-by: Tom St Denis <tom.stdenis-5C7GfCeVMHo@public.gmane.org<mailto:tom.stdenis@amd.com>>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 43 ++++++++++++++++++++++++++++++
>   1 file changed, 43 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 04c4aee70452..a9d83c1d7a43 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2637,10 +2637,45 @@ static ssize_t amdgpu_debugfs_regs_write(struct file *f, const char __user *buf,
>       struct amdgpu_device *adev = f->f_inode->i_private;
>       ssize_t result = 0;
>       int r;
> +     bool pm_pg_lock, use_bank;
> +     unsigned instance_bank, sh_bank, se_bank;
>
>       if (size & 0x3 || *pos & 0x3)
>               return -EINVAL;
>
> +     /* are we reading registers for which a PG lock is necessary? */
> +     pm_pg_lock = (*pos >> 23) & 1;
> +
> +     if (*pos & (1ULL << 62)) {
> +             se_bank = (*pos >> 24) & 0x3FF;
> +             sh_bank = (*pos >> 34) & 0x3FF;
> +             instance_bank = (*pos >> 44) & 0x3FF;
> +
> +             if (se_bank == 0x3FF)
> +                     se_bank = 0xFFFFFFFF;
> +             if (sh_bank == 0x3FF)
> +                     sh_bank = 0xFFFFFFFF;
> +             if (instance_bank == 0x3FF)
> +                     instance_bank = 0xFFFFFFFF;
> +             use_bank = 1;
> +     } else {
> +             use_bank = 0;
> +     }
> +
> +     *pos &= 0x3FFFF;
> +
> +     if (use_bank) {
> +             if ((sh_bank != 0xFFFFFFFF && sh_bank >= adev->gfx.config.max_sh_per_se) ||
> +                 (se_bank != 0xFFFFFFFF && se_bank >= adev->gfx.config.max_shader_engines))
> +                     return -EINVAL;
> +             mutex_lock(&adev->grbm_idx_mutex);
> +             amdgpu_gfx_select_se_sh(adev, se_bank,
> +                                     sh_bank, instance_bank);
> +     }
> +
> +     if (pm_pg_lock)
> +             mutex_lock(&adev->pm.mutex);
> +
>       while (size) {
>               uint32_t value;
>
> @@ -2659,6 +2694,14 @@ static ssize_t amdgpu_debugfs_regs_write(struct file *f, const char __user *buf,
>               size -= 4;
>       }
>
> +     if (use_bank) {
> +             amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff, 0xffffffff);
> +             mutex_unlock(&adev->grbm_idx_mutex);
> +     }
> +
> +     if (pm_pg_lock)
> +             mutex_unlock(&adev->pm.mutex);
> +
>       return result;
>   }
>





[-- Attachment #1.2: Type: text/html, Size: 12580 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/amd/amdgpu: Make debugfs write compliment read
       [not found]                         ` <CY4PR12MB1768134851FB1F505F746843F7DA0-rpdhrqHFk06yjjPBNVDk/QdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
@ 2016-10-11 12:35                           ` Christian König
  0 siblings, 0 replies; 9+ messages in thread
From: Christian König @ 2016-10-11 12:35 UTC (permalink / raw)
  To: StDenis, Tom, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW


[-- Attachment #1.1: Type: text/plain, Size: 7810 bytes --]

Hi Tom,

yeah, the timing issue is a good argument. In this case just go ahead 
and add my Acked-by to the patch.

Regards,
Christian.

Am 11.10.2016 um 14:19 schrieb StDenis, Tom:
>
> Hi Christian,
>
>
> I agree with the crux of your argument but you can crash the system 
> with carefully timed read/writes from the GFX registers, or MC, or ... 
> That's why the entries are root only.
>
>
> The issue with locking excessively is you might change behaviour of 
> something you're trying to debug because the kernel then has to wait 
> on the lock (or you're waiting on the lock).  For instance, suppose 
> you want to read DCE registers and the kernel is doing some gfx/grbm 
> related your DCE read will either block the GFX activity or you're 
> waiting on it which means the behaviour of the system could be 
> different than from the case where you're not probing the registers.
>
>
> The goal is to be as unobtrusive as possible.  Heck I often mmap the 
> BAR so I can read MMIO registers directly and not waste CPU cycles on 
> syscalls.
>
>
> The PM lock is because reading PGFSM registers during a powerup/down 
> results in a GPU hang, and we only take the GRBM lock when users want 
> to read from specific banks.  For any other case we don't need a lock. 
>  In the kernel they should be mutually exclusive.  The PM lock is only 
> taken in PP when performing a transition (or initializing the core) 
> and I have yet to see a place where PP/PM related code also grabs the 
> grbm index lock.
>
> I don't have strong feelings against your proposal but I do prefer not 
> to take the locks when I don't need to.  Ultimately I'll leave the 
> decision up to you and Alex.  I'd like to get the 2nd patch through 
> though as it makes the write side of the mmio reg complimentary (e.g. 
> bank selection/pm lock).
>
> Cheers,
> Tom
>
> ------------------------------------------------------------------------
> *From:* Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
> *Sent:* Tuesday, October 11, 2016 08:11
> *To:* StDenis, Tom; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> *Subject:* Re: [PATCH] drm/amd/amdgpu: Make debugfs write compliment read
>>
>> The only downside to taking the locks all the time is for 99% of 
>> debug reads it's unnecessary and in theory could lead to 
>> performance/behaviour problems.
>>
> Yeah, but performance isn't critical here and taking two locks 
> compared to the rest of what we (IOCTL etc...) do is completely 
> negligibly.
>
>> I've been using this on read for a while now and haven't noticed a 
>> problem.
>>
> My concern is rather that somebody tries to write to the PM regs on 
> accident or purpose without taking the lock.
>
> See, the kernel is usually responsible to keep people from doing 
> stupid things with the hardware, even if those people are root and 
> could do stupid things anyway. Well, you know what I mean?
>
> Regards,
> Christian.
>
> Am 10.10.2016 um 22:21 schrieb StDenis, Tom:
>>
>> The only downside to taking the locks all the time is for 99% of 
>> debug reads it's unnecessary and in theory could lead to 
>> performance/behaviour problems.
>>
>>
>> I've been using this on read for a while now and haven't noticed a 
>> problem.
>>
>>
>> Tom
>>
>>
>>
>> ------------------------------------------------------------------------
>> *From:* Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
>> *Sent:* Monday, October 10, 2016 09:49
>> *To:* Tom St Denis; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> *Cc:* StDenis, Tom
>> *Subject:* Re: [PATCH] drm/amd/amdgpu: Make debugfs write compliment 
>> read
>> Would it hurt us to always take the looks (both the pm as well as 
>> grbm idx lock)?
>>
>> I don't think that would be much of an issue and I would have a 
>> better feeling with that.
>>
>> Christian.
>>
>> Am 10.10.2016 um 15:38 schrieb Tom St Denis:
>>>
>>> that's kind of the point.  Its for debugging only.  Also reads can 
>>> already lock
>>>
>>>
>>> On Mon, Oct 10, 2016, 09:25 Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org 
>>> <mailto:deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>> wrote:
>>>
>>>     Mhm, that userspace controls if the lock is taken or not is a bit
>>>     problematic, on the other hand only root could access that.
>>>
>>>     Christian.
>>>
>>>     Am 10.10.2016 um 14:51 schrieb Tom St Denis:
>>>     > Add PG lock support as well as bank selection to
>>>     > the MMIO write function.
>>>     >
>>>     > Signed-off-by: Tom St Denis <tom.stdenis-5C7GfCeVMHo@public.gmane.org
>>>     <mailto:tom.stdenis-5C7GfCeVMHo@public.gmane.org>>
>>>     > ---
>>>     >  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 43
>>>     ++++++++++++++++++++++++++++++
>>>     >   1 file changed, 43 insertions(+)
>>>     >
>>>     > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>     b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>     > index 04c4aee70452..a9d83c1d7a43 100644
>>>     > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>     > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>     > @@ -2637,10 +2637,45 @@ static ssize_t
>>>     amdgpu_debugfs_regs_write(struct file *f, const char __user *buf,
>>>     >       struct amdgpu_device *adev = f->f_inode->i_private;
>>>     >       ssize_t result = 0;
>>>     >       int r;
>>>     > +     bool pm_pg_lock, use_bank;
>>>     > +     unsigned instance_bank, sh_bank, se_bank;
>>>     >
>>>     >       if (size & 0x3 || *pos & 0x3)
>>>     >               return -EINVAL;
>>>     >
>>>     > +     /* are we reading registers for which a PG lock is
>>>     necessary? */
>>>     > +     pm_pg_lock = (*pos >> 23) & 1;
>>>     > +
>>>     > +     if (*pos & (1ULL << 62)) {
>>>     > +             se_bank = (*pos >> 24) & 0x3FF;
>>>     > +             sh_bank = (*pos >> 34) & 0x3FF;
>>>     > +             instance_bank = (*pos >> 44) & 0x3FF;
>>>     > +
>>>     > +             if (se_bank == 0x3FF)
>>>     > +                     se_bank = 0xFFFFFFFF;
>>>     > +             if (sh_bank == 0x3FF)
>>>     > +                     sh_bank = 0xFFFFFFFF;
>>>     > +             if (instance_bank == 0x3FF)
>>>     > +                     instance_bank = 0xFFFFFFFF;
>>>     > +             use_bank = 1;
>>>     > +     } else {
>>>     > +             use_bank = 0;
>>>     > +     }
>>>     > +
>>>     > +     *pos &= 0x3FFFF;
>>>     > +
>>>     > +     if (use_bank) {
>>>     > +             if ((sh_bank != 0xFFFFFFFF && sh_bank >=
>>>     adev->gfx.config.max_sh_per_se) ||
>>>     > +                 (se_bank != 0xFFFFFFFF && se_bank >=
>>>     adev->gfx.config.max_shader_engines))
>>>     > +                     return -EINVAL;
>>>     > +  mutex_lock(&adev->grbm_idx_mutex);
>>>     > +  amdgpu_gfx_select_se_sh(adev, se_bank,
>>>     > +  sh_bank, instance_bank);
>>>     > +     }
>>>     > +
>>>     > +     if (pm_pg_lock)
>>>     > +  mutex_lock(&adev->pm.mutex);
>>>     > +
>>>     >       while (size) {
>>>     >               uint32_t value;
>>>     >
>>>     > @@ -2659,6 +2694,14 @@ static ssize_t
>>>     amdgpu_debugfs_regs_write(struct file *f, const char __user *buf,
>>>     >               size -= 4;
>>>     >       }
>>>     >
>>>     > +     if (use_bank) {
>>>     > +  amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff,
>>>     0xffffffff);
>>>     > +  mutex_unlock(&adev->grbm_idx_mutex);
>>>     > +     }
>>>     > +
>>>     > +     if (pm_pg_lock)
>>>     > +  mutex_unlock(&adev->pm.mutex);
>>>     > +
>>>     >       return result;
>>>     >   }
>>>     >
>>>
>>>
>>
>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx



[-- Attachment #1.2: Type: text/html, Size: 20338 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-10-11 12:35 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-10 12:51 [PATCH] drm/amd/amdgpu: Make debugfs write compliment read Tom St Denis
     [not found] ` <20161010125128.18303-1-tom.stdenis-5C7GfCeVMHo@public.gmane.org>
2016-10-10 13:25   ` Christian König
     [not found]     ` <91da2060-e6d4-70c9-2ceb-69218e1058ba-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
2016-10-10 13:38       ` Tom St Denis
     [not found]         ` <CAAzXoRJKusZrvv7obpZ-0tR39pVmdc-7Y8prKCsRC6bchmsOcA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-10-10 13:49           ` Christian König
     [not found]             ` <3c1d969b-0141-ef29-1e0f-5267d8a7cae9-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
2016-10-10 13:52               ` StDenis, Tom
2016-10-10 20:21               ` StDenis, Tom
     [not found]                 ` <CY4PR12MB1768813E554E6DB1A40F5848F7DB0-rpdhrqHFk06yjjPBNVDk/QdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2016-10-11 12:11                   ` Christian König
     [not found]                     ` <50dc12b3-a6bd-ed35-ac8f-e55909d184d2-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
2016-10-11 12:19                       ` StDenis, Tom
     [not found]                         ` <CY4PR12MB1768134851FB1F505F746843F7DA0-rpdhrqHFk06yjjPBNVDk/QdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2016-10-11 12:35                           ` Christian König

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.