Re: [Intel-xe] [PATCH 1/3] drm/xe: Introduce a module parameter to control XeLink

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

On Wed, Aug 09, 2023 at 01:26:22PM +0300, Jani Nikula wrote:
> On Tue, 08 Aug 2023, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > On Tue, Aug 08, 2023 at 12:45:32PM -0400, David Kershner wrote:
> >> Add a new module parameter to control enable/disable of the XeLink
> >> feature.
> >> 
> >> Signed-off-by: David Kershner <david.kershner@intel.com>
> >
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Basically the direction from upstream is crystal clear, and has been for
> years: never ask users to modify module parameters for normal operation.
> 
> Also, it's not possible to adjust this per-device.
> 
> Why are we still doing this?

Well, the last time that I looked to it was many years ago. So, someone
from the XeLink team can correct me if I am wrong or add more info.

From what I can remember, the DPA configuration needs to be set at
probe and after set I'm not confident that we have a way out of it.
And if that was set, the SRIOV cannot be used.

But right, it should be better if we could rewind the DPA settings,
and unload xelink when SRIOV starts?
Or do we have a way to block the 'child-modules'? Maybe that would
be better than the modparam itself in case we cannot rewind DPA
settings.

> 
> 
> BR,
> Jani.
> 
> 
> >
> >> ---
> >>  drivers/gpu/drm/xe/Makefile    |  3 ++-
> >>  drivers/gpu/drm/xe/xe_xelink.c | 17 +++++++++++++++++
> >>  2 files changed, 19 insertions(+), 1 deletion(-)
> >>  create mode 100644 drivers/gpu/drm/xe/xe_xelink.c
> >> 
> >> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> >> index 1b59702cd9f9..d46558cf12a9 100644
> >> --- a/drivers/gpu/drm/xe/Makefile
> >> +++ b/drivers/gpu/drm/xe/Makefile
> >> @@ -115,7 +115,8 @@ xe-y += xe_bb.o \
> >>  	xe_vm_madvise.o \
> >>  	xe_wait_user_fence.o \
> >>  	xe_wa.o \
> >> -	xe_wopcm.o
> >> +	xe_wopcm.o \
> >> +	xe_xelink.o
> >>  
> >>  # i915 Display compat #defines and #includes
> >>  subdir-ccflags-$(CONFIG_DRM_XE_DISPLAY) += \
> >> diff --git a/drivers/gpu/drm/xe/xe_xelink.c b/drivers/gpu/drm/xe/xe_xelink.c
> >> new file mode 100644
> >> index 000000000000..51a31f6a4740
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/xe/xe_xelink.c
> >> @@ -0,0 +1,17 @@
> >> +// SPDX-License-Identifier: MIT
> >> +/*
> >> + * Copyright © 2023 Intel Corporation
> >> + */
> >> +
> >> +#include <linux/moduleparam.h>
> >> +
> >> +/*
> >> + * This module parameter is needed because SRIOV PF and XeLink are mutually
> >> + * exclusive. The driver needs to be able to enable/disable the XeLink
> >> + * infrastructure (specifically Device Physical Addressing).  Since there
> >> + * will be no enable/disable for the SRIOV PF path, this parameter is
> >> + * needed to explicitly disable XeLink when SRIOV PF is required.
> >> + */
> >> +static bool xe_enable_xelink = true;
> >> +module_param_named(enable_xelink, xe_enable_xelink, bool, 0400);
> >> +MODULE_PARM_DESC(enable_xelink, "Enable XeLink feature (default: true)");
> >> -- 
> >> 2.35.1
> >> 
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center