Re: [v3 0/2] Add memory sysfs directory and attributes

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

On Mon, 2023-12-11 at 15:08 -0800, Matt Roper wrote:
> On Mon, Dec 11, 2023 at 05:57:09PM -0500, Rodrigo Vivi wrote:
> > On Mon, Dec 11, 2023 at 02:41:12PM -0800, Matt Roper wrote:
> > > On Thu, Dec 07, 2023 at 08:47:37PM +0530, Sujaritha Sundaresan
> > > wrote:
> > > > Create another layer of sub directories to clean up memory/vram
> > > > related sysfs path. Additionally, introduce the vram frequency
> > > > sysfs 
> > > > under a the new path.
> > > 
> > > What's the userspace consumer of this uAPI?  How will this wind
> > > up
> > > getting used in a real-world software stack?  Isn't stuff like
> > > the
> > > memory size already returned via the query interface?
> > 
> > Please notice that this is more for frequency management, nothing
> > related
> > to the UMD information that we expose in the ioctl queries.
> 
> Are you sure?  The first patch here is moving a
> 'physical_vram_size_bytes' entry.  I believe we can get each tile's
> VRAM
> size from the memory regions reported by the query ioctl as well,
> which
> makes me wonder why this is also in sysfs and who's using it.  It was
> already in sysfs before and is just moving location now, but if it's
> not
> needed we could probably just remove it?

you are right! I had missed this here, but I had bumped into IGT using
this sysfs entry and I agree we actually want to remove it entirely.

> 
> The frequency stuff shows up in the second patch, and "this is meant
> for
> the end user to tweak directly" is fine, but that should probably be
> stated explicitly.
> 
> 
> Matt
> 
> > 
> > Like every other sysfs interface, it doesn't actually matter what
> > wrapper
> > ends up using it. In general they are for system administrators to
> > tune
> > the hardware, so we just need to ensure that it makes sense, it is
> > clean
> > and something we will be able to maintain long term without
> > breaking
> > compatibility.
> > 
> > > 
> > > 
> > > Matt
> > > 
> > > > 
> > > > Sujaritha Sundaresan (2):
> > > >   drm/xe: Add a new memory directory under tile
> > > >   drm/xe: Add vram frequency sysfs attributes
> > > > 
> > > >  drivers/gpu/drm/xe/xe_pcode_api.h  |   8 +++
> > > >  drivers/gpu/drm/xe/xe_tile_sysfs.c | 101
> > > > +++++++++++++++++++++++++++--
> > > >  2 files changed, 104 insertions(+), 5 deletions(-)
> > > > 
> > > > -- 
> > > > 2.25.1
> > > > 
> > > 
> > > -- 
> > > Matt Roper
> > > Graphics Software Engineer
> > > Linux GPU Platform Enablement
> > > Intel Corporation
>