[Intel-xe] [PATCH 0/4] RFC: Add new device configuration infrastructure to

[Summers, Stuart]

On Thu, 2023-04-20 at 13:55 -0700, Lucas De Marchi wrote:
> On Thu, Apr 20, 2023 at 08:44:12PM +0000, Stuart Summers wrote:
> > On Thu, 2023-04-20 at 11:53 -0700, Lucas De Marchi wrote:
> > > On Thu, Apr 20, 2023 at 12:32:55PM +0300, Jani Nikula wrote:
> > > > On Wed, 19 Apr 2023, Stuart Summers <stuart.summers at intel.com>
> > > > wrote:
> > > > > This is attempting to take the best parts of i915 module
> > > > > parameters
> > > > > (minus the actual module parameters) and add to xe to allow
> > > > > for
> > > > > better
> > > > > debuggability and configuration in order to help isolate
> > > > > problems
> > > > > on a per-device level instead of global module parameters.
> > > > > 
> > > > > Note that I did review a few options here: configfs (not
> > > > > generally
> > > > > used by the drm stack), module parameters (we have some
> > > > > negative
> > > > > history here), sysfs (not the right approach given the focus
> > > > > on
> > > > > user interface here). Debugfs is used in various drm drivers
> > > > > to
> > > > > configure various device characteristics. The infrastructure
> > > > > being
> > > > > presented here has at a high level been present in the i915
> > > > > driver
> > > > > for some years now, so provides a good starting point for
> > > > > quick
> > > > > debug additions without exposing users to some of the
> > > > > challenges
> > > > > faced with module parameters in the past.
> > > > 
> > > > Looking into configfs has been on my todo list for a long time.
> > > > It's
> > > > something that often gets recommended as a replacement to
> > > > module
> > > > parameters, but the documentation as well as the existing
> > > > examples
> > > > in
> > > > the kernel are, I think, less than stellar. Basically would
> > > > require
> > > > implementing it and seeing how it actually works.
> > > > 
> > > > In any case, I don't think it should be dismissed with just
> > > > "not
> > > > generally used by the drm stack". A decent implementation could
> > > > set
> > > > the
> > > > example going forward.
> > > > 
> > > > The main problem with debugfs is the inability to set the
> > > > default
> > > > values
> > > > prior to probing the device. This is where module parameters
> > > > are
> > > > handy,
> > > > but they aren't device specific (and, as you note, generally
> > > > discouraged).
> > > > 
> > > > Looking at the patches, I'm not sure I understand what the
> > > > procedure for
> > > > setting the debugfs values before probing the device would be.
> > > > Can
> > > > you
> > > > provide an example sequence on the command-line please?
> > > 
> > > I think that for any solution != parameters is:  split the module
> > > load
> > > and the module bind phases.  I've been using that for a long time
> > > since
> > > it's a general solution for attaching tracepoints on early probe
> > > and
> > > it applies to any kernel module (differently than e.g. adding a
> > > "nomodeset"
> > > parameter to achive similar thing). For a debugging thing,
> > > usually
> > > for early
> > > bring up that would be acceptable I think.
> > 
> > Right I know we've done this many times with i915 without issue.
> > This
> > is the way we're configuring these things per device now. But it's
> > still a good point about configfs.
> > 
> > Anyway it isn't working with xe today, but that's why I sent this
> > as an
> > RFC first to get feedback.
> 
> yes, I know it doesn't work. My point is that any solution we've come
> up
> with that is not per-module like parameters, means that when testing,
> the
> user will have to split the module loading from module binding to the
> device
> under test. See the issue I pointed where we discussed a little bit
> about the options. The separation between load/bind is the same I
> use for tracepoints that I blogged about some time ago
> (https://politreco.com/2022/03/tracepoints-and-kernel-modules/)
> 
> For the approach with debugfs it would be no different:  first you
> have
> to a) load the module, then b) set the options then c) bind it to the
> device. But if you accept the split between load/bind, doing it via
> debugfs is basically reinventing configfs.

Right, but then again, these aren't meant for the typical user. Again,
these are intended to be debug specific settings. As such, the extra
steps needed to set up the file system in whatever way is either on the
kernel developer or possibly the CI developer. In both cases, this will
be very targeted and not something we would want to document as a
normal end-user experience.

As for reinventing configfs, point taken there. Let me look a bit
deeper there and get back.

Thanks,
Stuart

> 
> Lucas De Marchi
> 
> > I was also thinking the same about debug... really all of these
> > should
> > be considered debugging features/functions since the driver should
> > to
> > able to autodetect the default settings for each device
> > configuration
> > based on various registers and per-platform definitions. Still, let
> > me
> > dig into configfs a bit more before making that final
> > determination.
> > 
> > Really appreciate the thoughts here though!
> > 
> > -Stuart
> > 
> > > More context:
> > > https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/230
> > > 
> > > Lucas De Marchi