On Wed, Aug 21, 2024 at 09:32:48AM +0200, Andi Shyti wrote:
> Hi Rodrigo,
>
> On Tue, Aug 20, 2024 at 05:22:40PM -0400, Rodrigo Vivi 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>
> > > ---
> > > 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.
> >
> > well, if the sysfs dir/files creation is failing, then it will
> > probably be unreliable anyway right?
>
> Are you suggesting that "inv-<engine_name>" is OK?
it is okay I guess.
But my point is more on, how are we going to create this if
the creation mechanism is what is likely failing here.
>
> > > One further improvement would be to provide more information
> > > about thei failure reason the dev_warn() message.
> >
> > So, perhaps this patch should already go there and remove
> > the dev_err and add individual dev_warn for each failing path?
>
> That's a suggestion, but it doesn't mean that it necessarily
> improves things as it might add some innecessary information.
> Just thinking.
okay then.
>
> > Also it looks something is off with the goto paths...
> >
> > That if (0) is also ugly... probably better to use a
> > kobject_put with continue on every failing point as well...
>
> ehehe... I came to like it, to be honest. Besides I like single
> exit paths instead of distributed returns. In this particular
> case we would replcate the same "kobject_put() ... dev_warn()" in
> several places, so that I'm not sure it's better.
>
> If you like more we could do:
>
> for (...) {
> ...
> ...
> /* everything goes fine */
> continue
>
> err_engine:
> kobject_put(...);
> dev_warn(...);
> }
>
> And we avoid using the "if (0)" that you don't like.
nah, no strong feeling from my side. It is there, let's
avoid unnecessary refactors.
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
on this patch as is. And sorry for the delay.
>
> Thanks,
> Andi
