intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>,
	DRI Development <dri-devel@lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH v2 11/11] drm/i915: Extract i915_module.c
Date: Tue, 27 Jul 2021 15:44:30 +0100	[thread overview]
Message-ID: <f3b20f36-a76d-032a-3fb7-1a7c3e839478@linux.intel.com> (raw)
In-Reply-To: <20210727121037.2041102-11-daniel.vetter@ffwll.ch>


On 27/07/2021 13:10, Daniel Vetter wrote:
> The module init code is somewhat misplaced in i915_pci.c, since it
> needs to pull in init/exit functions from every part of the driver and
> pollutes the include list a lot.
> 
> Extract an i915_module.c file which pulls all the bits together, and
> allows us to massively trim the include list of i915_pci.c.
> 
> The downside is that have to drop the error path check Jason added to
> catch when we set up the pci driver too early. I think that risk is
> acceptable for this pretty nice include.

i915_module.c is an improvement and the rest for me is not extremely 
objectionable by the end of this incarnation, but I also do not see it 
as an improvement really.

There was a bug to fix relating to mock tests, but that is where the 
exercise should have stopped for now. After that it IMHO spiraled out of 
control, not least the unjustifiably expedited removal of cache 
shrinking. On balance for me it is too churny and boils down to two 
extremely capable people spending time on kind of really unimportant 
side fiddles. And I do not intend to prescribe you what to do, just 
expressing my bewilderment. FWIW... I can only say my opinion as it, not 
that it matters a lot.

Regards,

Tvrtko

> Cc: Jason Ekstrand <jason@jlekstrand.net>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>   drivers/gpu/drm/i915/Makefile      |   1 +
>   drivers/gpu/drm/i915/i915_module.c | 113 ++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_pci.c    | 117 +----------------------------
>   drivers/gpu/drm/i915/i915_pci.h    |   8 ++
>   4 files changed, 125 insertions(+), 114 deletions(-)
>   create mode 100644 drivers/gpu/drm/i915/i915_module.c
>   create mode 100644 drivers/gpu/drm/i915/i915_pci.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 9022dc638ed6..4ebd9f417ddb 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -38,6 +38,7 @@ i915-y += i915_drv.o \
>   	  i915_irq.o \
>   	  i915_getparam.o \
>   	  i915_mitigations.o \
> +	  i915_module.o \
>   	  i915_params.o \
>   	  i915_pci.o \
>   	  i915_scatterlist.o \
> diff --git a/drivers/gpu/drm/i915/i915_module.c b/drivers/gpu/drm/i915/i915_module.c
> new file mode 100644
> index 000000000000..c578ea8f56a0
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_module.c
> @@ -0,0 +1,113 @@
> +/*
> + * SPDX-License-Identifier: MIT
> + *
> + * Copyright © 2021 Intel Corporation
> + */
> +
> +#include <linux/console.h>
> +
> +#include "gem/i915_gem_context.h"
> +#include "gem/i915_gem_object.h"
> +#include "i915_active.h"
> +#include "i915_buddy.h"
> +#include "i915_params.h"
> +#include "i915_pci.h"
> +#include "i915_perf.h"
> +#include "i915_request.h"
> +#include "i915_scheduler.h"
> +#include "i915_selftest.h"
> +#include "i915_vma.h"
> +
> +static int i915_check_nomodeset(void)
> +{
> +	bool use_kms = true;
> +
> +	/*
> +	 * Enable KMS by default, unless explicitly overriden by
> +	 * either the i915.modeset prarameter or by the
> +	 * vga_text_mode_force boot option.
> +	 */
> +
> +	if (i915_modparams.modeset == 0)
> +		use_kms = false;
> +
> +	if (vgacon_text_force() && i915_modparams.modeset == -1)
> +		use_kms = false;
> +
> +	if (!use_kms) {
> +		/* Silently fail loading to not upset userspace. */
> +		DRM_DEBUG_DRIVER("KMS disabled.\n");
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct {
> +   int (*init)(void);
> +   void (*exit)(void);
> +} init_funcs[] = {
> +	{ i915_check_nomodeset, NULL },
> +	{ i915_active_module_init, i915_active_module_exit },
> +	{ i915_buddy_module_init, i915_buddy_module_exit },
> +	{ i915_context_module_init, i915_context_module_exit },
> +	{ i915_gem_context_module_init, i915_gem_context_module_exit },
> +	{ i915_objects_module_init, i915_objects_module_exit },
> +	{ i915_request_module_init, i915_request_module_exit },
> +	{ i915_scheduler_module_init, i915_scheduler_module_exit },
> +	{ i915_vma_module_init, i915_vma_module_exit },
> +	{ i915_mock_selftests, NULL },
> +	{ i915_pmu_init, i915_pmu_exit },
> +	{ i915_register_pci_driver, i915_unregister_pci_driver },
> +	{ i915_perf_sysctl_register, i915_perf_sysctl_unregister },
> +};
> +static int init_progress;
> +
> +static int __init i915_init(void)
> +{
> +	int err, i;
> +
> +	for (i = 0; i < ARRAY_SIZE(init_funcs); i++) {
> +		err = init_funcs[i].init();
> +		if (err < 0) {
> +			while (i--) {
> +				if (init_funcs[i].exit)
> +					init_funcs[i].exit();
> +			}
> +			return err;
> +		} else if (err > 0) {
> +			/*
> +			 * Early-exit success is reserved for things which
> +			 * don't have an exit() function because we have no
> +			 * idea how far they got or how to partially tear
> +			 * them down.
> +			 */
> +			WARN_ON(init_funcs[i].exit);
> +			break;
> +		}
> +	}
> +
> +	init_progress = i;
> +
> +	return 0;
> +}
> +
> +static void __exit i915_exit(void)
> +{
> +	int i;
> +
> +	for (i = init_progress - 1; i >= 0; i--) {
> +		GEM_BUG_ON(i >= ARRAY_SIZE(init_funcs));
> +		if (init_funcs[i].exit)
> +			init_funcs[i].exit();
> +	}
> +}
> +
> +module_init(i915_init);
> +module_exit(i915_exit);
> +
> +MODULE_AUTHOR("Tungsten Graphics, Inc.");
> +MODULE_AUTHOR("Intel Corporation");
> +
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_LICENSE("GPL and additional rights");
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index b4f5e88aaae6..08651ca03478 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -22,24 +22,13 @@
>    *
>    */
>   
> -#include <linux/console.h>
>   #include <linux/vga_switcheroo.h>
>   
>   #include <drm/drm_drv.h>
>   #include <drm/i915_pciids.h>
>   
> -#include "display/intel_fbdev.h"
> -
> -#include "i915_active.h"
> -#include "i915_buddy.h"
>   #include "i915_drv.h"
> -#include "gem/i915_gem_context.h"
> -#include "gem/i915_gem_object.h"
> -#include "i915_request.h"
> -#include "i915_perf.h"
> -#include "i915_selftest.h"
> -#include "i915_scheduler.h"
> -#include "i915_vma.h"
> +#include "i915_pci.h"
>   
>   #define PLATFORM(x) .platform = (x)
>   #define GEN(x) \
> @@ -1251,31 +1240,6 @@ static void i915_pci_shutdown(struct pci_dev *pdev)
>   	i915_driver_shutdown(i915);
>   }
>   
> -static int i915_check_nomodeset(void)
> -{
> -	bool use_kms = true;
> -
> -	/*
> -	 * Enable KMS by default, unless explicitly overriden by
> -	 * either the i915.modeset prarameter or by the
> -	 * vga_text_mode_force boot option.
> -	 */
> -
> -	if (i915_modparams.modeset == 0)
> -		use_kms = false;
> -
> -	if (vgacon_text_force() && i915_modparams.modeset == -1)
> -		use_kms = false;
> -
> -	if (!use_kms) {
> -		/* Silently fail loading to not upset userspace. */
> -		DRM_DEBUG_DRIVER("KMS disabled.\n");
> -		return 1;
> -	}
> -
> -	return 0;
> -}
> -
>   static struct pci_driver i915_pci_driver = {
>   	.name = DRIVER_NAME,
>   	.id_table = pciidlist,
> @@ -1285,87 +1249,12 @@ static struct pci_driver i915_pci_driver = {
>   	.driver.pm = &i915_pm_ops,
>   };
>   
> -static int i915_register_pci_driver(void)
> +int i915_register_pci_driver(void)
>   {
>   	return pci_register_driver(&i915_pci_driver);
>   }
>   
> -static void i915_unregister_pci_driver(void)
> +void i915_unregister_pci_driver(void)
>   {
>   	pci_unregister_driver(&i915_pci_driver);
>   }
> -
> -static const struct {
> -   int (*init)(void);
> -   void (*exit)(void);
> -} init_funcs[] = {
> -	{ i915_check_nomodeset, NULL },
> -	{ i915_active_module_init, i915_active_module_exit },
> -	{ i915_buddy_module_init, i915_buddy_module_exit },
> -	{ i915_context_module_init, i915_context_module_exit },
> -	{ i915_gem_context_module_init, i915_gem_context_module_exit },
> -	{ i915_objects_module_init, i915_objects_module_exit },
> -	{ i915_request_module_init, i915_request_module_exit },
> -	{ i915_scheduler_module_init, i915_scheduler_module_exit },
> -	{ i915_vma_module_init, i915_vma_module_exit },
> -	{ i915_mock_selftests, NULL },
> -	{ i915_pmu_init, i915_pmu_exit },
> -	{ i915_register_pci_driver, i915_unregister_pci_driver },
> -	{ i915_perf_sysctl_register, i915_perf_sysctl_unregister },
> -};
> -static int init_progress;
> -
> -static int __init i915_init(void)
> -{
> -	int err, i;
> -
> -	for (i = 0; i < ARRAY_SIZE(init_funcs); i++) {
> -		err = init_funcs[i].init();
> -		if (err < 0) {
> -			while (i--) {
> -				if (init_funcs[i].exit)
> -					init_funcs[i].exit();
> -			}
> -			return err;
> -		} else if (err > 0) {
> -			/*
> -			 * Early-exit success is reserved for things which
> -			 * don't have an exit() function because we have no
> -			 * idea how far they got or how to partially tear
> -			 * them down.
> -			 */
> -			WARN_ON(init_funcs[i].exit);
> -
> -			/*
> -			 * We don't want to advertise devices with an only
> -			 * partially initialized driver.
> -			 */
> -			WARN_ON(i915_pci_driver.driver.owner);
> -			break;
> -		}
> -	}
> -
> -	init_progress = i;
> -
> -	return 0;
> -}
> -
> -static void __exit i915_exit(void)
> -{
> -	int i;
> -
> -	for (i = init_progress - 1; i >= 0; i--) {
> -		GEM_BUG_ON(i >= ARRAY_SIZE(init_funcs));
> -		if (init_funcs[i].exit)
> -			init_funcs[i].exit();
> -	}
> -}
> -
> -module_init(i915_init);
> -module_exit(i915_exit);
> -
> -MODULE_AUTHOR("Tungsten Graphics, Inc.");
> -MODULE_AUTHOR("Intel Corporation");
> -
> -MODULE_DESCRIPTION(DRIVER_DESC);
> -MODULE_LICENSE("GPL and additional rights");
> diff --git a/drivers/gpu/drm/i915/i915_pci.h b/drivers/gpu/drm/i915/i915_pci.h
> new file mode 100644
> index 000000000000..b386f319f52e
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_pci.h
> @@ -0,0 +1,8 @@
> +/*
> + * SPDX-License-Identifier: MIT
> + *
> + * Copyright © 2021 Intel Corporation
> + */
> +
> +int i915_register_pci_driver(void);
> +void i915_unregister_pci_driver(void);
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2021-07-27 14:44 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-27 12:10 [Intel-gfx] [PATCH v2 01/11] drm/i915: Check for nomodeset in i915_init() first Daniel Vetter
2021-07-27 12:10 ` [Intel-gfx] [PATCH v2 02/11] drm/i915: move i915_active slab to direct module init/exit Daniel Vetter
2021-07-27 12:10 ` [Intel-gfx] [PATCH v2 03/11] drm/i915: move i915_buddy " Daniel Vetter
2021-07-27 12:10 ` [Intel-gfx] [PATCH v2 04/11] drm/i915: move intel_context " Daniel Vetter
2021-07-27 12:10 ` [Intel-gfx] [PATCH v2 05/11] drm/i915: move gem_context " Daniel Vetter
2021-07-27 12:10 ` [Intel-gfx] [PATCH v2 06/11] drm/i915: move gem_objects " Daniel Vetter
2021-07-27 12:10 ` [Intel-gfx] [PATCH v2 07/11] drm/i915: move request slabs " Daniel Vetter
2021-07-27 12:10 ` [Intel-gfx] [PATCH v2 08/11] drm/i915: move scheduler " Daniel Vetter
2021-07-27 12:10 ` [Intel-gfx] [PATCH v2 09/11] drm/i915: move vma slab " Daniel Vetter
2021-07-27 12:10 ` [Intel-gfx] [PATCH v2 10/11] drm/i915: Remove i915_globals Daniel Vetter
2021-07-27 12:10 ` [Intel-gfx] [PATCH v2 11/11] drm/i915: Extract i915_module.c Daniel Vetter
2021-07-27 14:44   ` Tvrtko Ursulin [this message]
2021-07-27 18:25     ` Jason Ekstrand
2021-08-23 15:22   ` Guenter Roeck
2021-08-23 16:15     ` Jani Nikula
2021-08-25 14:51   ` Jani Nikula
2021-07-27 14:09 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [v2,01/11] drm/i915: Check for nomodeset in i915_init() first Patchwork
2021-07-27 14:11 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2021-07-27 14:37 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2021-07-27 22:02 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f3b20f36-a76d-032a-3fb7-1a7c3e839478@linux.intel.com \
    --to=tvrtko.ursulin@linux.intel.com \
    --cc=daniel.vetter@ffwll.ch \
    --cc=daniel.vetter@intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --subject='Re: [Intel-gfx] [PATCH v2 11/11] drm/i915: Extract i915_module.c' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).