Re: [PATCH v2 1/3] drm/xe: Move xe_device_sysfs_init() to xe_pci_probe()

[Raag Jadav <raag.jadav@intel.com>]

On Thu, Apr 17, 2025 at 10:52:54AM +0530, Riana Tauro wrote:
> On 4/16/2025 10:07 PM, Raag Jadav wrote:
> > On Wed, Apr 16, 2025 at 08:15:48PM +0530, Riana Tauro wrote:
> > > Hi Raag
> > > 
> > > On 4/16/2025 4:19 PM, Raag Jadav wrote:
> > > > On Wed, Apr 16, 2025 at 03:34:43PM +0530, Riana Tauro wrote:
> > > > > On 4/16/2025 1:25 PM, Riana Tauro wrote:
> > > > > > Hi Raag
> > > > > > 
> > > > > > On 4/3/2025 11:17 PM, Raag Jadav wrote:
> > > > > > > Move xe_device_sysfs_init() to xe_pci_probe() so that we can expose
> > > > > > > non-PM attributes as well.
> > > > > > > 
> > > > > > > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > > > > > > ---
> > > > > > >     drivers/gpu/drm/xe/xe_device_sysfs.c | 11 +++++++----
> > > > > > >     drivers/gpu/drm/xe/xe_pci.c          |  6 ++++++
> > > > > > >     drivers/gpu/drm/xe/xe_pm.c           |  5 -----
> > > > > > >     3 files changed, 13 insertions(+), 9 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/xe/xe_device_sysfs.c b/drivers/gpu/drm/
> > > > > > > xe/xe_device_sysfs.c
> > > > > > > index 7efbd4c52791..d4c73acea1cf 100644
> > > > > > > --- a/drivers/gpu/drm/xe/xe_device_sysfs.c
> > > > > > > +++ b/drivers/gpu/drm/xe/xe_device_sysfs.c
> > > > > > > @@ -67,7 +67,8 @@ static void xe_device_sysfs_fini(void *arg)
> > > > > > >     {
> > > > > > >         struct xe_device *xe = arg;
> > > > > > > -    sysfs_remove_file(&xe->drm.dev->kobj,
> > > > > > > &dev_attr_vram_d3cold_threshold.attr);
> > > > > > > +    if (xe->d3cold.capable)
> > > > > > > +        sysfs_remove_file(&xe->drm.dev->kobj,
> > > > > > > &dev_attr_vram_d3cold_threshold.attr);
> > > > > > >     }
> > > > > > >     int xe_device_sysfs_init(struct xe_device *xe)
> > > > > > > @@ -75,9 +76,11 @@ int xe_device_sysfs_init(struct xe_device *xe)
> > > > > > >         struct device *dev = xe->drm.dev;
> > > > > > >         int ret;
> > > > > > > -    ret = sysfs_create_file(&dev->kobj,
> > > > > > > &dev_attr_vram_d3cold_threshold.attr);
> > > > > > > -    if (ret)
> > > > > > > -        return ret;
> > > > > > > +    if (xe->d3cold.capable) {
> > > > > > > +        ret = sysfs_create_file(&dev->kobj,
> > > > > > > &dev_attr_vram_d3cold_threshold.attr);
> > > > > > > +        if (ret)
> > > > > > > +            return ret;
> > > > > > > +    }
> > > > > > >         return devm_add_action_or_reset(dev, xe_device_sysfs_fini, xe);
> > > > > > >     }
> > > > > > > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> > > > > > > index 58347fa91841..246682b2a2be 100644
> > > > > > > --- a/drivers/gpu/drm/xe/xe_pci.c
> > > > > > > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > > > > > > @@ -18,6 +18,7 @@
> > > > > > >     #include "display/xe_display.h"
> > > > > > >     #include "regs/xe_gt_regs.h"
> > > > > > >     #include "xe_device.h"
> > > > > > > +#include "xe_device_sysfs.h"
> > > > > > >     #include "xe_drv.h"
> > > > > > >     #include "xe_gt.h"
> > > > > > >     #include "xe_gt_sriov_vf.h"
> > > > > > > @@ -864,6 +865,11 @@ static int xe_pci_probe(struct pci_dev *pdev,
> > > > > > > const struct pci_device_id *ent)
> > > > > > >         if (err)
> > > > > > >             goto err_driver_cleanup;
> > > > > > > +    /* We have PM related attributes, so do it after xe_pm_init(). */
> > > > > > > +    err = xe_device_sysfs_init(xe);
> > > > > > Layering doesn't seem right. This should in xe_device_probe.
> > > > > > d3cold capable can be moved to pm_init_early and this to device_probe
> > > > 
> > > > Are you sure if we can use ttm_manager_type() in pm_init_early()?
> > > 
> > > you need only d3cold.capable to setup sysfs. This call is using only pci
> > > calls. So this should work
> > > 
> > > xe->d3cold.capable = xe_pm_pci_d3cold_capable(xe);
> > 
> > Sure, but should we expose vram_d3cold_threshold before calculating its
> > value?
> 
> pm_init anyway fails if it's not able to set vram_d3cold_threshold.

Which means we should make sure it's valid before exposing it.

> Adding it to xe pci layer seemed wrong to me because there are no other
> sysfs init calls in probe.
> 
> But upto you

Sure, we can change if there's an alternative that doesn't break one rule
of thumb for another.

Raag