[Intel-gfx] [PATCH v3 0/7] drm: fdinfo memory stats

[Intel-gfx] [PATCH v3 0/7] drm: fdinfo memory stats

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Similar motivation to other similar recent attempt[1].  But with an
attempt to have some shared code for this.  As well as documentation.

It is probably a bit UMA-centric, I guess devices with VRAM might want
some placement stats as well.  But this seems like a reasonable start.

Basic gputop support: https://patchwork.freedesktop.org/series/116236/
And already nvtop support: https://github.com/Syllo/nvtop/pull/204

[1] https://patchwork.freedesktop.org/series/112397/

Rob Clark (7):
  drm: Add common fdinfo helper
  drm/msm: Switch to fdinfo helper
  drm/amdgpu: Switch to fdinfo helper
  drm/i915: Switch to fdinfo helper
  drm/etnaviv: Switch to fdinfo helper
  drm: Add fdinfo memory stats
  drm/msm: Add memory stats to fdinfo

 Documentation/gpu/drm-usage-stats.rst      |  21 ++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |   3 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  16 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.h |   2 +-
 drivers/gpu/drm/drm_file.c                 | 115 +++++++++++++++++++++
 drivers/gpu/drm/etnaviv/etnaviv_drv.c      |  10 +-
 drivers/gpu/drm/i915/i915_driver.c         |   3 +-
 drivers/gpu/drm/i915/i915_drm_client.c     |  18 +---
 drivers/gpu/drm/i915/i915_drm_client.h     |   2 +-
 drivers/gpu/drm/msm/msm_drv.c              |  11 +-
 drivers/gpu/drm/msm/msm_gem.c              |  15 +++
 drivers/gpu/drm/msm/msm_gpu.c              |   2 -
 include/drm/drm_drv.h                      |   7 ++
 include/drm/drm_file.h                     |   5 +
 include/drm/drm_gem.h                      |  19 ++++
 15 files changed, 208 insertions(+), 41 deletions(-)

-- 
2.39.2

[Intel-gfx] [PATCH v3 4/7] drm/i915: Switch to fdinfo helper

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/i915/i915_driver.c     |  3 ++-
 drivers/gpu/drm/i915/i915_drm_client.c | 18 +++++-------------
 drivers/gpu/drm/i915/i915_drm_client.h |  2 +-
 3 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index db7a86def7e2..37eacaa3064b 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1696,7 +1696,7 @@ static const struct file_operations i915_driver_fops = {
 	.compat_ioctl = i915_ioc32_compat_ioctl,
 	.llseek = noop_llseek,
 #ifdef CONFIG_PROC_FS
-	.show_fdinfo = i915_drm_client_fdinfo,
+	.show_fdinfo = drm_fop_show_fdinfo,
 #endif
 };
 
@@ -1796,6 +1796,7 @@ static const struct drm_driver i915_drm_driver = {
 	.open = i915_driver_open,
 	.lastclose = i915_driver_lastclose,
 	.postclose = i915_driver_postclose,
+	.show_fdinfo = i915_drm_client_fdinfo,
 
 	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
 	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c
index b09d1d386574..4a77e5e47f79 100644
--- a/drivers/gpu/drm/i915/i915_drm_client.c
+++ b/drivers/gpu/drm/i915/i915_drm_client.c
@@ -101,7 +101,7 @@ static u64 busy_add(struct i915_gem_context *ctx, unsigned int class)
 }
 
 static void
-show_client_class(struct seq_file *m,
+show_client_class(struct drm_printer *p,
 		  struct i915_drm_client *client,
 		  unsigned int class)
 {
@@ -117,22 +117,20 @@ show_client_class(struct seq_file *m,
 	rcu_read_unlock();
 
 	if (capacity)
-		seq_printf(m, "drm-engine-%s:\t%llu ns\n",
+		drm_printf(p, "drm-engine-%s:\t%llu ns\n",
 			   uabi_class_names[class], total);
 
 	if (capacity > 1)
-		seq_printf(m, "drm-engine-capacity-%s:\t%u\n",
+		drm_printf(p, "drm-engine-capacity-%s:\t%u\n",
 			   uabi_class_names[class],
 			   capacity);
 }
 
-void i915_drm_client_fdinfo(struct seq_file *m, struct file *f)
+void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file)
 {
-	struct drm_file *file = f->private_data;
 	struct drm_i915_file_private *file_priv = file->driver_priv;
 	struct drm_i915_private *i915 = file_priv->dev_priv;
 	struct i915_drm_client *client = file_priv->client;
-	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
 	unsigned int i;
 
 	/*
@@ -141,12 +139,6 @@ void i915_drm_client_fdinfo(struct seq_file *m, struct file *f)
 	 * ******************************************************************
 	 */
 
-	seq_printf(m, "drm-driver:\t%s\n", i915->drm.driver->name);
-	seq_printf(m, "drm-pdev:\t%04x:%02x:%02x.%d\n",
-		   pci_domain_nr(pdev->bus), pdev->bus->number,
-		   PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
-	seq_printf(m, "drm-client-id:\t%u\n", client->id);
-
 	/*
 	 * Temporarily skip showing client engine information with GuC submission till
 	 * fetching engine busyness is implemented in the GuC submission backend
@@ -155,6 +147,6 @@ void i915_drm_client_fdinfo(struct seq_file *m, struct file *f)
 		return;
 
 	for (i = 0; i < ARRAY_SIZE(uabi_class_names); i++)
-		show_client_class(m, client, i);
+		show_client_class(p, client, i);
 }
 #endif
diff --git a/drivers/gpu/drm/i915/i915_drm_client.h b/drivers/gpu/drm/i915/i915_drm_client.h
index 69496af996d9..ef85fef45de5 100644
--- a/drivers/gpu/drm/i915/i915_drm_client.h
+++ b/drivers/gpu/drm/i915/i915_drm_client.h
@@ -60,7 +60,7 @@ static inline void i915_drm_client_put(struct i915_drm_client *client)
 struct i915_drm_client *i915_drm_client_add(struct i915_drm_clients *clients);
 
 #ifdef CONFIG_PROC_FS
-void i915_drm_client_fdinfo(struct seq_file *m, struct file *f);
+void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file);
 #endif
 
 void i915_drm_clients_fini(struct i915_drm_clients *clients);
-- 
2.39.2

Re: [Intel-gfx] [PATCH v3 0/7] drm: fdinfo memory stats

[Christian König]

Am 12.04.23 um 00:56 schrieb Rob Clark:
> From: Rob Clark <robdclark@chromium.org>
>
> Similar motivation to other similar recent attempt[1].  But with an
> attempt to have some shared code for this.  As well as documentation.
>
> It is probably a bit UMA-centric, I guess devices with VRAM might want
> some placement stats as well.  But this seems like a reasonable start.
>
> Basic gputop support: https://patchwork.freedesktop.org/series/116236/
> And already nvtop support: https://github.com/Syllo/nvtop/pull/204
>
> [1] https://patchwork.freedesktop.org/series/112397/

I think the extra client id looks a bit superfluous since the ino of the 
file should already be unique and IIRC we have been already using that one.

Apart from that looks good to me,
Christian.

PS: For some reason only the two patches I was CCed on ended up in my 
inbox, dri-devel swallowed all the rest and hasn't spit it out yet. Had 
to dig up the rest from patchwork.


>
> Rob Clark (7):
>    drm: Add common fdinfo helper
>    drm/msm: Switch to fdinfo helper
>    drm/amdgpu: Switch to fdinfo helper
>    drm/i915: Switch to fdinfo helper
>    drm/etnaviv: Switch to fdinfo helper
>    drm: Add fdinfo memory stats
>    drm/msm: Add memory stats to fdinfo
>
>   Documentation/gpu/drm-usage-stats.rst      |  21 ++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |   3 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  16 ++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.h |   2 +-
>   drivers/gpu/drm/drm_file.c                 | 115 +++++++++++++++++++++
>   drivers/gpu/drm/etnaviv/etnaviv_drv.c      |  10 +-
>   drivers/gpu/drm/i915/i915_driver.c         |   3 +-
>   drivers/gpu/drm/i915/i915_drm_client.c     |  18 +---
>   drivers/gpu/drm/i915/i915_drm_client.h     |   2 +-
>   drivers/gpu/drm/msm/msm_drv.c              |  11 +-
>   drivers/gpu/drm/msm/msm_gem.c              |  15 +++
>   drivers/gpu/drm/msm/msm_gpu.c              |   2 -
>   include/drm/drm_drv.h                      |   7 ++
>   include/drm/drm_file.h                     |   5 +
>   include/drm/drm_gem.h                      |  19 ++++
>   15 files changed, 208 insertions(+), 41 deletions(-)
>

Re: [Intel-gfx] [PATCH v3 0/7] drm: fdinfo memory stats

[Tvrtko Ursulin]

On 12/04/2023 10:34, Christian König wrote:
> Am 12.04.23 um 00:56 schrieb Rob Clark:
>> From: Rob Clark <robdclark@chromium.org>
>>
>> Similar motivation to other similar recent attempt[1].  But with an
>> attempt to have some shared code for this.  As well as documentation.
>>
>> It is probably a bit UMA-centric, I guess devices with VRAM might want
>> some placement stats as well.  But this seems like a reasonable start.
>>
>> Basic gputop support: https://patchwork.freedesktop.org/series/116236/
>> And already nvtop support: https://github.com/Syllo/nvtop/pull/204
>>
>> [1] https://patchwork.freedesktop.org/series/112397/
> 
> I think the extra client id looks a bit superfluous since the ino of the 
> file should already be unique and IIRC we have been already using that one.

Do you mean file_inode(struct drm_file->filp)->i_ino ? That one would be 
the same number for all clients which open the same device node so 
wouldn't work.

I also don't think the atomic_add_return for client id works either, 
since it can alias on overflow.

In i915 I use an xarray and __xa_alloc_cyclic.

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH v3 0/7] drm: fdinfo memory stats

[Christian König]

Am 12.04.23 um 14:10 schrieb Tvrtko Ursulin:
>
> On 12/04/2023 10:34, Christian König wrote:
>> Am 12.04.23 um 00:56 schrieb Rob Clark:
>>> From: Rob Clark <robdclark@chromium.org>
>>>
>>> Similar motivation to other similar recent attempt[1].  But with an
>>> attempt to have some shared code for this.  As well as documentation.
>>>
>>> It is probably a bit UMA-centric, I guess devices with VRAM might want
>>> some placement stats as well.  But this seems like a reasonable start.
>>>
>>> Basic gputop support: https://patchwork.freedesktop.org/series/116236/
>>> And already nvtop support: https://github.com/Syllo/nvtop/pull/204
>>>
>>> [1] https://patchwork.freedesktop.org/series/112397/
>>
>> I think the extra client id looks a bit superfluous since the ino of 
>> the file should already be unique and IIRC we have been already using 
>> that one.
>
> Do you mean file_inode(struct drm_file->filp)->i_ino ? That one would 
> be the same number for all clients which open the same device node so 
> wouldn't work.

Ah, right. DMA-buf used a separate ino per buffer, but we don't do that 
for the drm_file.

>
> I also don't think the atomic_add_return for client id works either, 
> since it can alias on overflow.

Yeah, we might want to use a 64bit number here if any.

Christian.

>
> In i915 I use an xarray and __xa_alloc_cyclic.
>
> Regards,
>
> Tvrtko

Re: [Intel-gfx] [PATCH v3 4/7] drm/i915: Switch to fdinfo helper

[Tvrtko Ursulin]

On 11/04/2023 23:56, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>   drivers/gpu/drm/i915/i915_driver.c     |  3 ++-
>   drivers/gpu/drm/i915/i915_drm_client.c | 18 +++++-------------
>   drivers/gpu/drm/i915/i915_drm_client.h |  2 +-
>   3 files changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index db7a86def7e2..37eacaa3064b 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1696,7 +1696,7 @@ static const struct file_operations i915_driver_fops = {
>   	.compat_ioctl = i915_ioc32_compat_ioctl,
>   	.llseek = noop_llseek,
>   #ifdef CONFIG_PROC_FS
> -	.show_fdinfo = i915_drm_client_fdinfo,
> +	.show_fdinfo = drm_fop_show_fdinfo,
>   #endif
>   };
>   
> @@ -1796,6 +1796,7 @@ static const struct drm_driver i915_drm_driver = {
>   	.open = i915_driver_open,
>   	.lastclose = i915_driver_lastclose,
>   	.postclose = i915_driver_postclose,
> +	.show_fdinfo = i915_drm_client_fdinfo,
>   
>   	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>   	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c
> index b09d1d386574..4a77e5e47f79 100644
> --- a/drivers/gpu/drm/i915/i915_drm_client.c
> +++ b/drivers/gpu/drm/i915/i915_drm_client.c
> @@ -101,7 +101,7 @@ static u64 busy_add(struct i915_gem_context *ctx, unsigned int class)
>   }
>   
>   static void
> -show_client_class(struct seq_file *m,
> +show_client_class(struct drm_printer *p,
>   		  struct i915_drm_client *client,
>   		  unsigned int class)
>   {
> @@ -117,22 +117,20 @@ show_client_class(struct seq_file *m,
>   	rcu_read_unlock();
>   
>   	if (capacity)
> -		seq_printf(m, "drm-engine-%s:\t%llu ns\n",
> +		drm_printf(p, "drm-engine-%s:\t%llu ns\n",
>   			   uabi_class_names[class], total);
>   
>   	if (capacity > 1)
> -		seq_printf(m, "drm-engine-capacity-%s:\t%u\n",
> +		drm_printf(p, "drm-engine-capacity-%s:\t%u\n",
>   			   uabi_class_names[class],
>   			   capacity);
>   }
>   
> -void i915_drm_client_fdinfo(struct seq_file *m, struct file *f)
> +void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file)
>   {
> -	struct drm_file *file = f->private_data;
>   	struct drm_i915_file_private *file_priv = file->driver_priv;
>   	struct drm_i915_private *i915 = file_priv->dev_priv;
>   	struct i915_drm_client *client = file_priv->client;
> -	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
>   	unsigned int i;
>   
>   	/*
> @@ -141,12 +139,6 @@ void i915_drm_client_fdinfo(struct seq_file *m, struct file *f)
>   	 * ******************************************************************
>   	 */
>   
> -	seq_printf(m, "drm-driver:\t%s\n", i915->drm.driver->name);
> -	seq_printf(m, "drm-pdev:\t%04x:%02x:%02x.%d\n",
> -		   pci_domain_nr(pdev->bus), pdev->bus->number,
> -		   PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> -	seq_printf(m, "drm-client-id:\t%u\n", client->id);

As mentioned in my reply to the cover letter, I think the i915 
implementation is the right one. At least the semantics of it.

Granted it is a super set of the minimum required as documented by 
drm-usage-stats.rst - not only 1:1 to current instances of struct file, 
but also avoids immediate id recycling.

Former could perhaps be achieved with a simple pointer hash, but latter 
helps userspace detect when a client has exited and id re-allocated to a 
new client within a single scanning period.

Without this I don't think userspace can implement a fail safe method of 
detecting which clients are new ones and so wouldn't be able to track 
history correctly.

I think we should rather extend the documented contract to include the 
cyclical property than settle for a weaker common implementation.

Regards,

Tvrtko

> -
>   	/*
>   	 * Temporarily skip showing client engine information with GuC submission till
>   	 * fetching engine busyness is implemented in the GuC submission backend
> @@ -155,6 +147,6 @@ void i915_drm_client_fdinfo(struct seq_file *m, struct file *f)
>   		return;
>   
>   	for (i = 0; i < ARRAY_SIZE(uabi_class_names); i++)
> -		show_client_class(m, client, i);
> +		show_client_class(p, client, i);
>   }
>   #endif
> diff --git a/drivers/gpu/drm/i915/i915_drm_client.h b/drivers/gpu/drm/i915/i915_drm_client.h
> index 69496af996d9..ef85fef45de5 100644
> --- a/drivers/gpu/drm/i915/i915_drm_client.h
> +++ b/drivers/gpu/drm/i915/i915_drm_client.h
> @@ -60,7 +60,7 @@ static inline void i915_drm_client_put(struct i915_drm_client *client)
>   struct i915_drm_client *i915_drm_client_add(struct i915_drm_clients *clients);
>   
>   #ifdef CONFIG_PROC_FS
> -void i915_drm_client_fdinfo(struct seq_file *m, struct file *f);
> +void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file);
>   #endif
>   
>   void i915_drm_clients_fini(struct i915_drm_clients *clients);

Re: [Intel-gfx] [PATCH v3 4/7] drm/i915: Switch to fdinfo helper

[Daniel Vetter]

On Wed, Apr 12, 2023 at 01:32:43PM +0100, Tvrtko Ursulin wrote:
> 
> On 11/04/2023 23:56, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> > 
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >   drivers/gpu/drm/i915/i915_driver.c     |  3 ++-
> >   drivers/gpu/drm/i915/i915_drm_client.c | 18 +++++-------------
> >   drivers/gpu/drm/i915/i915_drm_client.h |  2 +-
> >   3 files changed, 8 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > index db7a86def7e2..37eacaa3064b 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -1696,7 +1696,7 @@ static const struct file_operations i915_driver_fops = {
> >   	.compat_ioctl = i915_ioc32_compat_ioctl,
> >   	.llseek = noop_llseek,
> >   #ifdef CONFIG_PROC_FS
> > -	.show_fdinfo = i915_drm_client_fdinfo,
> > +	.show_fdinfo = drm_fop_show_fdinfo,
> >   #endif
> >   };
> > @@ -1796,6 +1796,7 @@ static const struct drm_driver i915_drm_driver = {
> >   	.open = i915_driver_open,
> >   	.lastclose = i915_driver_lastclose,
> >   	.postclose = i915_driver_postclose,
> > +	.show_fdinfo = i915_drm_client_fdinfo,
> >   	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> >   	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> > diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c
> > index b09d1d386574..4a77e5e47f79 100644
> > --- a/drivers/gpu/drm/i915/i915_drm_client.c
> > +++ b/drivers/gpu/drm/i915/i915_drm_client.c
> > @@ -101,7 +101,7 @@ static u64 busy_add(struct i915_gem_context *ctx, unsigned int class)
> >   }
> >   static void
> > -show_client_class(struct seq_file *m,
> > +show_client_class(struct drm_printer *p,
> >   		  struct i915_drm_client *client,
> >   		  unsigned int class)
> >   {
> > @@ -117,22 +117,20 @@ show_client_class(struct seq_file *m,
> >   	rcu_read_unlock();
> >   	if (capacity)
> > -		seq_printf(m, "drm-engine-%s:\t%llu ns\n",
> > +		drm_printf(p, "drm-engine-%s:\t%llu ns\n",
> >   			   uabi_class_names[class], total);
> >   	if (capacity > 1)
> > -		seq_printf(m, "drm-engine-capacity-%s:\t%u\n",
> > +		drm_printf(p, "drm-engine-capacity-%s:\t%u\n",
> >   			   uabi_class_names[class],
> >   			   capacity);
> >   }
> > -void i915_drm_client_fdinfo(struct seq_file *m, struct file *f)
> > +void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file)
> >   {
> > -	struct drm_file *file = f->private_data;
> >   	struct drm_i915_file_private *file_priv = file->driver_priv;
> >   	struct drm_i915_private *i915 = file_priv->dev_priv;
> >   	struct i915_drm_client *client = file_priv->client;
> > -	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> >   	unsigned int i;
> >   	/*
> > @@ -141,12 +139,6 @@ void i915_drm_client_fdinfo(struct seq_file *m, struct file *f)
> >   	 * ******************************************************************
> >   	 */
> > -	seq_printf(m, "drm-driver:\t%s\n", i915->drm.driver->name);
> > -	seq_printf(m, "drm-pdev:\t%04x:%02x:%02x.%d\n",
> > -		   pci_domain_nr(pdev->bus), pdev->bus->number,
> > -		   PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> > -	seq_printf(m, "drm-client-id:\t%u\n", client->id);
> 
> As mentioned in my reply to the cover letter, I think the i915
> implementation is the right one. At least the semantics of it.
> 
> Granted it is a super set of the minimum required as documented by
> drm-usage-stats.rst - not only 1:1 to current instances of struct file, but
> also avoids immediate id recycling.
> 
> Former could perhaps be achieved with a simple pointer hash, but latter
> helps userspace detect when a client has exited and id re-allocated to a new
> client within a single scanning period.
> 
> Without this I don't think userspace can implement a fail safe method of
> detecting which clients are new ones and so wouldn't be able to track
> history correctly.
> 
> I think we should rather extend the documented contract to include the
> cyclical property than settle for a weaker common implementation.

atomic64_t never wraps, so you don't have any recycling issues?

The other piece and imo much more important is that I really don't want
the i915_drm_client design to spread, it conceptually makes no sense.
drm_file is the uapi object, once that's gone userspace will never be able
to look at anything, having a separate free-standing object that's
essentially always dead is backwards.

I went a bit more in-depth in a different thread on scheduler fd_info
stats, but essentially fd_info needs to pull stats, you should never push
stats towards the drm_file (or i915_drm_client). That avoids all the
refcounting issues and rcu needs and everything else like that.

Maybe you want to jump into that thread:
https://lore.kernel.org/dri-devel/CAKMK7uE=m3sSTQrLCeDg0vG8viODOecUsYDK1oC++f5pQi0e8Q@mail.gmail.com/

So retiring i915_drm_client infrastructure is the right direction I think.
-Daniel

> Regards,
> 
> Tvrtko
> 
> > -
> >   	/*
> >   	 * Temporarily skip showing client engine information with GuC submission till
> >   	 * fetching engine busyness is implemented in the GuC submission backend
> > @@ -155,6 +147,6 @@ void i915_drm_client_fdinfo(struct seq_file *m, struct file *f)
> >   		return;
> >   	for (i = 0; i < ARRAY_SIZE(uabi_class_names); i++)
> > -		show_client_class(m, client, i);
> > +		show_client_class(p, client, i);
> >   }
> >   #endif
> > diff --git a/drivers/gpu/drm/i915/i915_drm_client.h b/drivers/gpu/drm/i915/i915_drm_client.h
> > index 69496af996d9..ef85fef45de5 100644
> > --- a/drivers/gpu/drm/i915/i915_drm_client.h
> > +++ b/drivers/gpu/drm/i915/i915_drm_client.h
> > @@ -60,7 +60,7 @@ static inline void i915_drm_client_put(struct i915_drm_client *client)
> >   struct i915_drm_client *i915_drm_client_add(struct i915_drm_clients *clients);
> >   #ifdef CONFIG_PROC_FS
> > -void i915_drm_client_fdinfo(struct seq_file *m, struct file *f);
> > +void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file);
> >   #endif
> >   void i915_drm_clients_fini(struct i915_drm_clients *clients);

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH v3 4/7] drm/i915: Switch to fdinfo helper

[Tvrtko Ursulin]

On 12/04/2023 14:51, Daniel Vetter wrote:
> On Wed, Apr 12, 2023 at 01:32:43PM +0100, Tvrtko Ursulin wrote:
>>
>> On 11/04/2023 23:56, Rob Clark wrote:
>>> From: Rob Clark <robdclark@chromium.org>
>>>
>>> Signed-off-by: Rob Clark <robdclark@chromium.org>
>>> ---
>>>    drivers/gpu/drm/i915/i915_driver.c     |  3 ++-
>>>    drivers/gpu/drm/i915/i915_drm_client.c | 18 +++++-------------
>>>    drivers/gpu/drm/i915/i915_drm_client.h |  2 +-
>>>    3 files changed, 8 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
>>> index db7a86def7e2..37eacaa3064b 100644
>>> --- a/drivers/gpu/drm/i915/i915_driver.c
>>> +++ b/drivers/gpu/drm/i915/i915_driver.c
>>> @@ -1696,7 +1696,7 @@ static const struct file_operations i915_driver_fops = {
>>>    	.compat_ioctl = i915_ioc32_compat_ioctl,
>>>    	.llseek = noop_llseek,
>>>    #ifdef CONFIG_PROC_FS
>>> -	.show_fdinfo = i915_drm_client_fdinfo,
>>> +	.show_fdinfo = drm_fop_show_fdinfo,
>>>    #endif
>>>    };
>>> @@ -1796,6 +1796,7 @@ static const struct drm_driver i915_drm_driver = {
>>>    	.open = i915_driver_open,
>>>    	.lastclose = i915_driver_lastclose,
>>>    	.postclose = i915_driver_postclose,
>>> +	.show_fdinfo = i915_drm_client_fdinfo,
>>>    	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>>>    	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>>> diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c
>>> index b09d1d386574..4a77e5e47f79 100644
>>> --- a/drivers/gpu/drm/i915/i915_drm_client.c
>>> +++ b/drivers/gpu/drm/i915/i915_drm_client.c
>>> @@ -101,7 +101,7 @@ static u64 busy_add(struct i915_gem_context *ctx, unsigned int class)
>>>    }
>>>    static void
>>> -show_client_class(struct seq_file *m,
>>> +show_client_class(struct drm_printer *p,
>>>    		  struct i915_drm_client *client,
>>>    		  unsigned int class)
>>>    {
>>> @@ -117,22 +117,20 @@ show_client_class(struct seq_file *m,
>>>    	rcu_read_unlock();
>>>    	if (capacity)
>>> -		seq_printf(m, "drm-engine-%s:\t%llu ns\n",
>>> +		drm_printf(p, "drm-engine-%s:\t%llu ns\n",
>>>    			   uabi_class_names[class], total);
>>>    	if (capacity > 1)
>>> -		seq_printf(m, "drm-engine-capacity-%s:\t%u\n",
>>> +		drm_printf(p, "drm-engine-capacity-%s:\t%u\n",
>>>    			   uabi_class_names[class],
>>>    			   capacity);
>>>    }
>>> -void i915_drm_client_fdinfo(struct seq_file *m, struct file *f)
>>> +void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file)
>>>    {
>>> -	struct drm_file *file = f->private_data;
>>>    	struct drm_i915_file_private *file_priv = file->driver_priv;
>>>    	struct drm_i915_private *i915 = file_priv->dev_priv;
>>>    	struct i915_drm_client *client = file_priv->client;
>>> -	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
>>>    	unsigned int i;
>>>    	/*
>>> @@ -141,12 +139,6 @@ void i915_drm_client_fdinfo(struct seq_file *m, struct file *f)
>>>    	 * ******************************************************************
>>>    	 */
>>> -	seq_printf(m, "drm-driver:\t%s\n", i915->drm.driver->name);
>>> -	seq_printf(m, "drm-pdev:\t%04x:%02x:%02x.%d\n",
>>> -		   pci_domain_nr(pdev->bus), pdev->bus->number,
>>> -		   PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>>> -	seq_printf(m, "drm-client-id:\t%u\n", client->id);
>>
>> As mentioned in my reply to the cover letter, I think the i915
>> implementation is the right one. At least the semantics of it.
>>
>> Granted it is a super set of the minimum required as documented by
>> drm-usage-stats.rst - not only 1:1 to current instances of struct file, but
>> also avoids immediate id recycling.
>>
>> Former could perhaps be achieved with a simple pointer hash, but latter
>> helps userspace detect when a client has exited and id re-allocated to a new
>> client within a single scanning period.
>>
>> Without this I don't think userspace can implement a fail safe method of
>> detecting which clients are new ones and so wouldn't be able to track
>> history correctly.
>>
>> I think we should rather extend the documented contract to include the
>> cyclical property than settle for a weaker common implementation.
> 
> atomic64_t never wraps, so you don't have any recycling issues?

Okay yes, with 64 bits there aren't any practical recycling issues.

> The other piece and imo much more important is that I really don't want
> the i915_drm_client design to spread, it conceptually makes no sense.
> drm_file is the uapi object, once that's gone userspace will never be able
> to look at anything, having a separate free-standing object that's
> essentially always dead is backwards.
> 
> I went a bit more in-depth in a different thread on scheduler fd_info
> stats, but essentially fd_info needs to pull stats, you should never push
> stats towards the drm_file (or i915_drm_client). That avoids all the
> refcounting issues and rcu needs and everything else like that.
> 
> Maybe you want to jump into that thread:
> https://lore.kernel.org/dri-devel/CAKMK7uE=m3sSTQrLCeDg0vG8viODOecUsYDK1oC++f5pQi0e8Q@mail.gmail.com/
> 
> So retiring i915_drm_client infrastructure is the right direction I think.

Hmmm.. it is a _mostly_ pull model that we have in i915 ie. data is 
pulled on fdinfo queries.

_Mostly_ because it cannot be fully pull based when you look at some 
internal flows. We have to save some data at runtime at times not driven 
by the fdinfo queries.

For instance context close needs to record the GPU utilisation against 
the client so that it is not lost. Also in the execlists backend we must 
transfer the hardware tracked runtime into the software state when hw 
contexts are switched out.

The fact i915_drm_client is detached from file_priv is a consequence of 
the fact i915 GEM contexts can outlive drm_file, and that when such 
contexts are closed, we need a to record their runtimes.

So I think there are three options: how it is now, fully krefed 
drm_file, or prohibit persistent contexts. Last one don't think we can 
do due ABI and 2nd felt heavy handed so I choose a lightweight 
i915_drm_client option.

Maybe there is a fourth option of somehow detecting during context 
destruction that drm_file is gone and skip the runtime recording, but 
avoiding races and all did not make me want to entertain it much. Is 
this actually what you are proposing?

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH v3 4/7] drm/i915: Switch to fdinfo helper

[Daniel Vetter]

On Wed, Apr 12, 2023 at 04:12:41PM +0100, Tvrtko Ursulin wrote:
> 
> On 12/04/2023 14:51, Daniel Vetter wrote:
> > On Wed, Apr 12, 2023 at 01:32:43PM +0100, Tvrtko Ursulin wrote:
> > > 
> > > On 11/04/2023 23:56, Rob Clark wrote:
> > > > From: Rob Clark <robdclark@chromium.org>
> > > > 
> > > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > > ---
> > > >    drivers/gpu/drm/i915/i915_driver.c     |  3 ++-
> > > >    drivers/gpu/drm/i915/i915_drm_client.c | 18 +++++-------------
> > > >    drivers/gpu/drm/i915/i915_drm_client.h |  2 +-
> > > >    3 files changed, 8 insertions(+), 15 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > > > index db7a86def7e2..37eacaa3064b 100644
> > > > --- a/drivers/gpu/drm/i915/i915_driver.c
> > > > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > > > @@ -1696,7 +1696,7 @@ static const struct file_operations i915_driver_fops = {
> > > >    	.compat_ioctl = i915_ioc32_compat_ioctl,
> > > >    	.llseek = noop_llseek,
> > > >    #ifdef CONFIG_PROC_FS
> > > > -	.show_fdinfo = i915_drm_client_fdinfo,
> > > > +	.show_fdinfo = drm_fop_show_fdinfo,
> > > >    #endif
> > > >    };
> > > > @@ -1796,6 +1796,7 @@ static const struct drm_driver i915_drm_driver = {
> > > >    	.open = i915_driver_open,
> > > >    	.lastclose = i915_driver_lastclose,
> > > >    	.postclose = i915_driver_postclose,
> > > > +	.show_fdinfo = i915_drm_client_fdinfo,
> > > >    	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> > > >    	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> > > > diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c
> > > > index b09d1d386574..4a77e5e47f79 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drm_client.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drm_client.c
> > > > @@ -101,7 +101,7 @@ static u64 busy_add(struct i915_gem_context *ctx, unsigned int class)
> > > >    }
> > > >    static void
> > > > -show_client_class(struct seq_file *m,
> > > > +show_client_class(struct drm_printer *p,
> > > >    		  struct i915_drm_client *client,
> > > >    		  unsigned int class)
> > > >    {
> > > > @@ -117,22 +117,20 @@ show_client_class(struct seq_file *m,
> > > >    	rcu_read_unlock();
> > > >    	if (capacity)
> > > > -		seq_printf(m, "drm-engine-%s:\t%llu ns\n",
> > > > +		drm_printf(p, "drm-engine-%s:\t%llu ns\n",
> > > >    			   uabi_class_names[class], total);
> > > >    	if (capacity > 1)
> > > > -		seq_printf(m, "drm-engine-capacity-%s:\t%u\n",
> > > > +		drm_printf(p, "drm-engine-capacity-%s:\t%u\n",
> > > >    			   uabi_class_names[class],
> > > >    			   capacity);
> > > >    }
> > > > -void i915_drm_client_fdinfo(struct seq_file *m, struct file *f)
> > > > +void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file)
> > > >    {
> > > > -	struct drm_file *file = f->private_data;
> > > >    	struct drm_i915_file_private *file_priv = file->driver_priv;
> > > >    	struct drm_i915_private *i915 = file_priv->dev_priv;
> > > >    	struct i915_drm_client *client = file_priv->client;
> > > > -	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> > > >    	unsigned int i;
> > > >    	/*
> > > > @@ -141,12 +139,6 @@ void i915_drm_client_fdinfo(struct seq_file *m, struct file *f)
> > > >    	 * ******************************************************************
> > > >    	 */
> > > > -	seq_printf(m, "drm-driver:\t%s\n", i915->drm.driver->name);
> > > > -	seq_printf(m, "drm-pdev:\t%04x:%02x:%02x.%d\n",
> > > > -		   pci_domain_nr(pdev->bus), pdev->bus->number,
> > > > -		   PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> > > > -	seq_printf(m, "drm-client-id:\t%u\n", client->id);
> > > 
> > > As mentioned in my reply to the cover letter, I think the i915
> > > implementation is the right one. At least the semantics of it.
> > > 
> > > Granted it is a super set of the minimum required as documented by
> > > drm-usage-stats.rst - not only 1:1 to current instances of struct file, but
> > > also avoids immediate id recycling.
> > > 
> > > Former could perhaps be achieved with a simple pointer hash, but latter
> > > helps userspace detect when a client has exited and id re-allocated to a new
> > > client within a single scanning period.
> > > 
> > > Without this I don't think userspace can implement a fail safe method of
> > > detecting which clients are new ones and so wouldn't be able to track
> > > history correctly.
> > > 
> > > I think we should rather extend the documented contract to include the
> > > cyclical property than settle for a weaker common implementation.
> > 
> > atomic64_t never wraps, so you don't have any recycling issues?
> 
> Okay yes, with 64 bits there aren't any practical recycling issues.
> 
> > The other piece and imo much more important is that I really don't want
> > the i915_drm_client design to spread, it conceptually makes no sense.
> > drm_file is the uapi object, once that's gone userspace will never be able
> > to look at anything, having a separate free-standing object that's
> > essentially always dead is backwards.
> > 
> > I went a bit more in-depth in a different thread on scheduler fd_info
> > stats, but essentially fd_info needs to pull stats, you should never push
> > stats towards the drm_file (or i915_drm_client). That avoids all the
> > refcounting issues and rcu needs and everything else like that.
> > 
> > Maybe you want to jump into that thread:
> > https://lore.kernel.org/dri-devel/CAKMK7uE=m3sSTQrLCeDg0vG8viODOecUsYDK1oC++f5pQi0e8Q@mail.gmail.com/
> > 
> > So retiring i915_drm_client infrastructure is the right direction I think.
> 
> Hmmm.. it is a _mostly_ pull model that we have in i915 ie. data is pulled
> on fdinfo queries.
> 
> _Mostly_ because it cannot be fully pull based when you look at some
> internal flows. We have to save some data at runtime at times not driven by
> the fdinfo queries.
> 
> For instance context close needs to record the GPU utilisation against the
> client so that it is not lost. Also in the execlists backend we must
> transfer the hardware tracked runtime into the software state when hw
> contexts are switched out.
> 
> The fact i915_drm_client is detached from file_priv is a consequence of the
> fact i915 GEM contexts can outlive drm_file, and that when such contexts are
> closed, we need a to record their runtimes.
> 
> So I think there are three options: how it is now, fully krefed drm_file, or
> prohibit persistent contexts. Last one don't think we can do due ABI and 2nd
> felt heavy handed so I choose a lightweight i915_drm_client option.
> 
> Maybe there is a fourth option of somehow detecting during context
> destruction that drm_file is gone and skip the runtime recording, but
> avoiding races and all did not make me want to entertain it much. Is this
> actually what you are proposing?

Hm right, persistent context, the annoying thing I missed again. From a
quick look amdgpu gets away with that by shooting all contexts
synchronously on drmfd close, which is the thing i915 can't because uapi.

The other part of the trick is to ... not care :-) See
amdgpu_ctx_fence_time(). I guess what would work a bit better is a
drm_file context list under a spinlock (which would need to be per
drm_device probably), which is cleaned up both when the final context ref
drops and when the drmfd closes, and you push back the final tally just
under that spinlock. But that's not how drm_sched_entity works right now,
that disappears before the final in-flight jobs have finished.

But yeah unless we just shrug and accept an accounting hole some minimal
push-back (at least while the drm_file is still alive) is needed to add
back the final tally when a context is destroyed.

Anyway I think it'd be good if you can follow that sched fd_info thread a
bit, to make sure it's not too silly :-) i915 won't use it, but xe will
eventually.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch