On Tue, Aug 27, 2024 at 07:05:05PM +0200, Daniel Vetter wrote: > On Mon, Aug 19, 2024 at 01:31:40PM +0200, Andi Shyti wrote: > > The i915 driver generates sysfs entries for each engine of the > > GPU in /sys/class/drm/cardX/engines/. > > > > The process is straightforward: we loop over the UABI engines and > > for each one, we: > > > > - Create the object. > > - Create basic files. > > - If the engine supports timeslicing, create timeslice duration files. > > - If the engine supports preemption, create preemption-related files. > > - Create default value files. > > > > Currently, if any of these steps fail, the process stops, and no > > further sysfs files are created. > > > > However, it's not necessary to stop the process on failure. > > Instead, we can continue creating the remaining sysfs files for > > the other engines. Even if some files fail to be created, the > > list of engines can still be retrieved by querying i915. > > > > Signed-off-by: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx> > > Uh, sysfs is uapi. Either we need it, and it _must_ be there, or it's not > needed, and we should delete those files probably. > > This is different from debugfs, where failures are consistently ignored > because that's the conscious design choice Greg made and wants supported. > Because debugfs is optional. > > So please make sure we correctly fail driver load if these don't register. > Even better would be if sysfs files are registered atomically as attribute > blocks, but that's an entire different can of worms. But that would really > clean up this code and essentially put any failure handling onto core > driver model and sysfs code. Indeed very good point. Sorry for having missed this perspective. > -Sima > > > --- > > Hi, > > > > It might make sense to create an "inv-<engine_name>" if something > > goes wrong, so that the user is aware that the engine exists, but > > the sysfs file is not present. > > > > One further improvement would be to provide more information > > about thei failure reason the dev_warn() message. > > > > Andi > > > > drivers/gpu/drm/i915/gt/sysfs_engines.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/sysfs_engines.c b/drivers/gpu/drm/i915/gt/sysfs_engines.c > > index 021f51d9b456..aab2759067d2 100644 > > --- a/drivers/gpu/drm/i915/gt/sysfs_engines.c > > +++ b/drivers/gpu/drm/i915/gt/sysfs_engines.c > > @@ -530,9 +530,8 @@ void intel_engines_add_sysfs(struct drm_i915_private *i915) > > err_object: > > kobject_put(kobj); > > err_engine: > > - dev_err(kdev, "Failed to add sysfs engine '%s'\n", > > - engine->name); > > - break; > > + dev_warn(kdev, "Failed to add sysfs engine '%s'\n", > > + engine->name); > > } > > } > > } > > -- > > 2.45.2 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
