From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 1AC15C64ED8 for ; Mon, 27 Feb 2023 12:11:01 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id DC92510E3CE; Mon, 27 Feb 2023 12:11:00 +0000 (UTC) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id 31FF810E3CE for ; Mon, 27 Feb 2023 12:10:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1677499859; x=1709035859; h=from:to:cc:subject:in-reply-to:references:date: message-id:mime-version:content-transfer-encoding; bh=m9s6C80ibPNd9eN+qQtS64GVnr1HjbC2YNJRifsrfGQ=; b=VueVyDbqmYBZSlyK2rRRE20MqPK/RPCeCAeG7s/oZvaWIdgc9eMC5RFt kkH54SoIRRnJ2Ty5RzO6QBXu0U/CBBg//1hvfW8r55L5TBCJBcxZUVn1X DGX2v/5PCPMLO+9RJen5MmBjx/Bgkk3aRQXvYMpP8FybaUU1LhL9yJs3I PXk+LwHZnNNBik5FLCSFDGCYeDQebETnNKhm6n9tuyTXtSd7Qp+iLGd7o BKlt11YfHn7oT2vdDhnAONcRq0JpIf4lzppJHHL4D5+7zIXcjKwqvsd5H QGsR081zM0v3ci0qSbxqCBnGkENiQanMx01sHlYEZ4OGYH2wZgeQCWNFA g==; X-IronPort-AV: E=McAfee;i="6500,9779,10633"; a="361401030" X-IronPort-AV: E=Sophos;i="5.97,331,1669104000"; d="scan'208";a="361401030" Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Feb 2023 04:10:58 -0800 X-IronPort-AV: E=McAfee;i="6500,9779,10633"; a="762688967" X-IronPort-AV: E=Sophos;i="5.97,331,1669104000"; d="scan'208";a="762688967" Received: from jkaisrli-mobl1.ger.corp.intel.com (HELO localhost) ([10.252.56.158]) by fmsmga003-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Feb 2023 04:10:56 -0800 From: Jani Nikula To: Rodrigo Vivi In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <20230203202742.577659-1-rodrigo.vivi@intel.com> <20230203202742.577659-3-rodrigo.vivi@intel.com> <87cz6921qq.fsf@intel.com> Date: Mon, 27 Feb 2023 14:10:53 +0200 Message-ID: <87pm9v1f76.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Intel-xe] [PATCH 2/6] drm/xe: place all modprobe parameters at the same place X-BeenThere: intel-xe@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel Xe graphics driver List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-xe@lists.freedesktop.org Errors-To: intel-xe-bounces@lists.freedesktop.org Sender: "Intel-xe" On Fri, 17 Feb 2023, Rodrigo Vivi wrote: > On Thu, Feb 16, 2023 at 03:12:13PM +0200, Jani Nikula wrote: >> On Fri, 03 Feb 2023, Rodrigo Vivi wrote: >> > From: Mauro Carvalho Chehab >> > >> > It makes easier to identify the module parameters if they're >> > placed it at the same place. >> > >> > Place them together with the module author/description/license. >> > >> > While here, fix a checkpatch.pl warning on force_probe description: >> > WARNING: quoted string split across lines >> > >> > Signed-off-by: Mauro Carvalho Chehab >> > Reviewed-by: Rodrigo Vivi >> > Signed-off-by: Rodrigo Vivi >>=20 >> I don't really see why this needs to reinvent the wheel when we could >> use the same model as i915. Separate file, all params in a struct that >> provides a namespace for the params. It's hideous to have them all in >> the driver local namespace. > > I was willing to avoid any special infra in order to discourage the > addition of many parameters. > > But if we start growing we can definitely have the split and all. > > Also, I'd like to avoid i915isms. if we need those macros and all like > we have in i915 we should probably get that into a common kernel > infra for debugfs because they would likely benefit other drivers as > well. Putting all the params in a single file is already an i915-ism. Doing that without also embedding them in a struct for namespace is copying the bad parts without the good. If you don't want to do that, I suggest making all the parameters *static* in the file they're needed, and if you need access elsewhere, add accessors. And, indeed, the best approach would be to reject (almost) all module paramters. BR, Jani. > >>=20 >> > --- >> > drivers/gpu/drm/xe/xe_device.c | 10 +--------- >> > drivers/gpu/drm/xe/xe_guc_log.c | 5 +---- >> > drivers/gpu/drm/xe/xe_mmio.c | 5 +---- >> > drivers/gpu/drm/xe/xe_module.c | 22 ++++++++++++++++++++++ >> > drivers/gpu/drm/xe/xe_module.h | 13 +++++++++++++ >> > drivers/gpu/drm/xe/xe_pci.c | 7 +------ >> > 6 files changed, 39 insertions(+), 23 deletions(-) >> > create mode 100644 drivers/gpu/drm/xe/xe_module.h >> > >> > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_de= vice.c >> > index 2e1f4beba9b0..60938c2deee2 100644 >> > --- a/drivers/gpu/drm/xe/xe_device.c >> > +++ b/drivers/gpu/drm/xe/xe_device.c >> > @@ -20,6 +20,7 @@ >> > #include "xe_exec.h" >> > #include "xe_gt.h" >> > #include "xe_irq.h" >> > +#include "xe_module.h" >> > #include "xe_mmio.h" >> > #include "xe_pcode.h" >> > #include "xe_pm.h" >> > @@ -42,15 +43,6 @@ >> > #include "display/ext/intel_pm.h" >> > #endif >> >=20=20 >> > -/* FIXME: Move to common param infrastructure */ >> > -static bool enable_guc =3D true; >> > -module_param_named_unsafe(enable_guc, enable_guc, bool, 0444); >> > -MODULE_PARM_DESC(enable_guc, "Enable GuC submission"); >> > - >> > -static bool enable_display =3D true; >> > -module_param_named(enable_display, enable_display, bool, 0444); >> > -MODULE_PARM_DESC(enable_display, "Enable display"); >> > - >> > static int xe_file_open(struct drm_device *dev, struct drm_file *file) >> > { >> > struct xe_file *xef; >> > diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/xe_g= uc_log.c >> > index 3f19fbf243d1..7ec1b2bb1f8e 100644 >> > --- a/drivers/gpu/drm/xe/xe_guc_log.c >> > +++ b/drivers/gpu/drm/xe/xe_guc_log.c >> > @@ -9,10 +9,7 @@ >> > #include "xe_gt.h" >> > #include "xe_guc_log.h" >> > #include "xe_map.h" >> > - >> > -static int xe_guc_log_level =3D 5; >> > -module_param_named(guc_log_level, xe_guc_log_level, int, 0600); >> > -MODULE_PARM_DESC(guc_log_level, "GuC firmware logging level (0=3Ddisa= ble, 1..5=3Denable with verbosity min..max)"); >> > +#include "xe_module.h" >> >=20=20 >> > static struct xe_gt * >> > log_to_gt(struct xe_guc_log *log) >> > diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio= .c >> > index f20734cf15ba..8a953df2b468 100644 >> > --- a/drivers/gpu/drm/xe/xe_mmio.c >> > +++ b/drivers/gpu/drm/xe/xe_mmio.c >> > @@ -12,6 +12,7 @@ >> > #include "xe_gt.h" >> > #include "xe_gt_mcr.h" >> > #include "xe_macros.h" >> > +#include "xe_module.h" >> >=20=20 >> > #include "i915_reg.h" >> > #include "gt/intel_engine_regs.h" >> > @@ -21,10 +22,6 @@ >> > #define TILE_COUNT REG_GENMASK(15, 8) >> > #define GEN12_LMEM_BAR 2 >> >=20=20 >> > -static u32 xe_force_lmem_bar_size; >> > -module_param_named(lmem_bar_size, xe_force_lmem_bar_size, uint, 0600); >> > -MODULE_PARM_DESC(lmem_bar_size, "Set the lmem bar size(in MiB)"); >> > - >> > static int xe_set_dma_info(struct xe_device *xe) >> > { >> > unsigned int mask_size =3D xe->info.dma_mask_size; >> > diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/xe_mo= dule.c >> > index d6b50f1c2a05..9cd1663f83f6 100644 >> > --- a/drivers/gpu/drm/xe/xe_module.c >> > +++ b/drivers/gpu/drm/xe/xe_module.c >> > @@ -8,9 +8,31 @@ >> >=20=20 >> > #include "xe_drv.h" >> > #include "xe_hw_fence.h" >> > +#include "xe_module.h" >> > #include "xe_pci.h" >> > #include "xe_sched_job.h" >> >=20=20 >> > +bool enable_guc =3D true; >> > +module_param_named_unsafe(enable_guc, enable_guc, bool, 0444); >> > +MODULE_PARM_DESC(enable_guc, "Enable GuC submission"); >> > + >> > +bool enable_display =3D true; >> > +module_param_named(enable_display, enable_display, bool, 0444); >> > +MODULE_PARM_DESC(enable_display, "Enable display"); >> > + >> > +u32 xe_force_lmem_bar_size; >> > +module_param_named(lmem_bar_size, xe_force_lmem_bar_size, uint, 0600); >> > +MODULE_PARM_DESC(lmem_bar_size, "Set the lmem bar size(in MiB)"); >> > + >> > +int xe_guc_log_level =3D 5; >> > +module_param_named(guc_log_level, xe_guc_log_level, int, 0600); >> > +MODULE_PARM_DESC(guc_log_level, "GuC firmware logging level (0=3Ddisa= ble, 1..5=3Denable with verbosity min..max)"); >> > + >> > +char *xe_param_force_probe =3D CONFIG_DRM_XE_FORCE_PROBE; >> > +module_param_named_unsafe(force_probe, xe_param_force_probe, charp, 0= 400); >> > +MODULE_PARM_DESC(force_probe, >> > + "Force probe options for specified devices. See CONFIG_DRM_XE_FORC= E_PROBE for details."); >> > + >> > struct init_funcs { >> > int (*init)(void); >> > void (*exit)(void); >> > diff --git a/drivers/gpu/drm/xe/xe_module.h b/drivers/gpu/drm/xe/xe_mo= dule.h >> > new file mode 100644 >> > index 000000000000..2c6ee46f5595 >> > --- /dev/null >> > +++ b/drivers/gpu/drm/xe/xe_module.h >> > @@ -0,0 +1,13 @@ >> > +/* SPDX-License-Identifier: MIT */ >> > +/* >> > + * Copyright =C2=A9 2023 Intel Corporation >> > + */ >> > + >> > +#include >> > + >> > +/* Module modprobe variables */ >> > +extern bool enable_guc; >> > +extern bool enable_display; >> > +extern u32 xe_force_lmem_bar_size; >> > +extern int xe_guc_log_level; >> > +extern char *xe_param_force_probe; >> > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c >> > index 1d5b6afed2c3..20aa2b5ca9ac 100644 >> > --- a/drivers/gpu/drm/xe/xe_pci.c >> > +++ b/drivers/gpu/drm/xe/xe_pci.c >> > @@ -17,17 +17,12 @@ >> > #include "xe_drv.h" >> > #include "xe_device.h" >> > #include "xe_macros.h" >> > +#include "xe_module.h" >> > #include "xe_pm.h" >> > #include "xe_step.h" >> >=20=20 >> > #include "i915_reg.h" >> >=20=20 >> > -static char *xe_param_force_probe =3D CONFIG_DRM_XE_FORCE_PROBE; >> > -module_param_named_unsafe(force_probe, xe_param_force_probe, charp, 0= 400); >> > -MODULE_PARM_DESC(force_probe, >> > - "Force probe options for specified devices. " >> > - "See CONFIG_DRM_XE_FORCE_PROBE for details."); >> > - >> > #define DEV_INFO_FOR_EACH_FLAG(func) \ >> > func(require_force_probe); \ >> > func(is_dgfx); \ >>=20 >> --=20 >> Jani Nikula, Intel Open Source Graphics Center --=20 Jani Nikula, Intel Open Source Graphics Center