Re: [PATCH 4/4] drm/i915: Disable dynamic setup of device_info->num_rings

[Oscar Mateo <oscar.mateo@xxxxxxxxx>]

On 02/09/2018 02:35 PM, Chris Wilson wrote:
> In order to allow the compiler to use a known constant number of
> available engines, disable modification of intel_device_static_info
> during engine bring up. Instead of trying to gracefully hide the broken
> setup, error out -- in theory, this should be caught during power on.

We are about to have a case for dynamic number of available engines. 
It's one of the ICL patches:

drm/i915/icl: Check for fused-off VDBOX and VEBOX instances

intel_device_runtime_info as well?

> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/i915_pci.c        | 34 +++++++++++++++++-----------------
>   drivers/gpu/drm/i915/intel_engine_cs.c | 16 +++++-----------
>   2 files changed, 22 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index a10404686710..1e5bc971e975 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -31,6 +31,7 @@
>   
>   #define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1)
>   #define PLATFORM(x) .platform = (x), .platform_mask = BIT(x)
> +#define RINGS(x) .ring_mask = (x), .num_rings = hweight32(x)
>   
>   #define GEN_DEFAULT_PIPEOFFSETS \
>   	.pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
> @@ -67,12 +68,12 @@
>   
>   #define GEN2_FEATURES \
>   	GEN(2), \
> +	RINGS(RENDER_RING), \
>   	.num_pipes = 1, \
>   	.has_overlay = 1, .overlay_needs_physical = 1, \
>   	.has_gmch_display = 1, \
>   	.hws_needs_physical = 1, \
>   	.unfenced_needs_alignment = 1, \
> -	.ring_mask = RENDER_RING, \
>   	.has_snoop = true, \
>   	GEN_DEFAULT_PIPEOFFSETS, \
>   	GEN_DEFAULT_PAGE_SIZES, \
> @@ -106,9 +107,9 @@ static const struct intel_device_info intel_i865g_info = {
>   
>   #define GEN3_FEATURES \
>   	GEN(3), \
> +	RINGS(RENDER_RING), \
>   	.num_pipes = 2, \
>   	.has_gmch_display = 1, \
> -	.ring_mask = RENDER_RING, \
>   	.has_snoop = true, \
>   	GEN_DEFAULT_PIPEOFFSETS, \
>   	GEN_DEFAULT_PAGE_SIZES, \
> @@ -173,10 +174,10 @@ static const struct intel_device_info intel_pineview_info = {
>   
>   #define GEN4_FEATURES \
>   	GEN(4), \
> +	RINGS(RENDER_RING), \
>   	.num_pipes = 2, \
>   	.has_hotplug = 1, \
>   	.has_gmch_display = 1, \
> -	.ring_mask = RENDER_RING, \
>   	.has_snoop = true, \
>   	GEN_DEFAULT_PIPEOFFSETS, \
>   	GEN_DEFAULT_PAGE_SIZES, \
> @@ -203,22 +204,22 @@ static const struct intel_device_info intel_i965gm_info = {
>   static const struct intel_device_info intel_g45_info = {
>   	GEN4_FEATURES,
>   	PLATFORM(INTEL_G45),
> -	.ring_mask = RENDER_RING | BSD_RING,
> +	RINGS(RENDER_RING | BSD_RING),
>   };
>   
>   static const struct intel_device_info intel_gm45_info = {
>   	GEN4_FEATURES,
>   	PLATFORM(INTEL_GM45),
> +	RINGS(RENDER_RING | BSD_RING),
>   	.is_mobile = 1, .has_fbc = 1,
>   	.supports_tv = 1,
> -	.ring_mask = RENDER_RING | BSD_RING,
>   };
>   
>   #define GEN5_FEATURES \
>   	GEN(5), \
> +	RINGS(RENDER_RING | BSD_RING), \
>   	.num_pipes = 2, \
>   	.has_hotplug = 1, \
> -	.ring_mask = RENDER_RING | BSD_RING, \
>   	.has_snoop = true, \
>   	/* ilk does support rc6, but we do not implement [power] contexts */ \
>   	.has_rc6 = 0, \
> @@ -239,10 +240,10 @@ static const struct intel_device_info intel_ironlake_m_info = {
>   
>   #define GEN6_FEATURES \
>   	GEN(6), \
> +	RINGS(RENDER_RING | BSD_RING | BLT_RING), \
>   	.num_pipes = 2, \
>   	.has_hotplug = 1, \
>   	.has_fbc = 1, \
> -	.ring_mask = RENDER_RING | BSD_RING | BLT_RING, \
>   	.has_llc = 1, \
>   	.has_rc6 = 1, \
>   	.has_rc6p = 1, \
> @@ -283,10 +284,10 @@ static const struct intel_device_info intel_sandybridge_m_gt2_info = {
>   
>   #define GEN7_FEATURES  \
>   	GEN(7), \
> +	RINGS(RENDER_RING | BSD_RING | BLT_RING), \
>   	.num_pipes = 3, \
>   	.has_hotplug = 1, \
>   	.has_fbc = 1, \
> -	.ring_mask = RENDER_RING | BSD_RING | BLT_RING, \
>   	.has_llc = 1, \
>   	.has_rc6 = 1, \
>   	.has_rc6p = 1, \
> @@ -338,6 +339,7 @@ static const struct intel_device_info intel_ivybridge_q_info = {
>   static const struct intel_device_info intel_valleyview_info = {
>   	PLATFORM(INTEL_VALLEYVIEW),
>   	GEN(7),
> +	RINGS(RENDER_RING | BSD_RING | BLT_RING),
>   	.is_lp = 1,
>   	.num_pipes = 2,
>   	.has_psr = 1,
> @@ -348,7 +350,6 @@ static const struct intel_device_info intel_valleyview_info = {
>   	.has_aliasing_ppgtt = 1,
>   	.has_full_ppgtt = 1,
>   	.has_snoop = true,
> -	.ring_mask = RENDER_RING | BSD_RING | BLT_RING,
>   	.display_mmio_offset = VLV_DISPLAY_BASE,
>   	GEN_DEFAULT_PAGE_SIZES,
>   	GEN_DEFAULT_PIPEOFFSETS,
> @@ -357,7 +358,7 @@ static const struct intel_device_info intel_valleyview_info = {
>   
>   #define G75_FEATURES  \
>   	GEN7_FEATURES, \
> -	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING, \
> +	RINGS(RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING), \
>   	.has_ddi = 1, \
>   	.has_fpga_dbg = 1, \
>   	.has_psr = 1, \
> @@ -421,17 +422,17 @@ static const struct intel_device_info intel_broadwell_rsvd_info = {
>   
>   static const struct intel_device_info intel_broadwell_gt3_info = {
>   	BDW_PLATFORM,
> +	RINGS(RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING),
>   	.gt = 3,
> -	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
>   };
>   
>   static const struct intel_device_info intel_cherryview_info = {
>   	PLATFORM(INTEL_CHERRYVIEW),
>   	GEN(8),
> +	RINGS(RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING),
>   	.num_pipes = 3,
>   	.has_hotplug = 1,
>   	.is_lp = 1,
> -	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
>   	.has_64bit_reloc = 1,
>   	.has_psr = 1,
>   	.has_runtime_pm = 1,
> @@ -481,8 +482,7 @@ static const struct intel_device_info intel_skylake_gt2_info = {
>   
>   #define SKL_GT3_PLUS_PLATFORM \
>   	SKL_PLATFORM, \
> -	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING
> -
> +	RINGS(RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING)
>   
>   static const struct intel_device_info intel_skylake_gt3_info = {
>   	SKL_GT3_PLUS_PLATFORM,
> @@ -496,9 +496,9 @@ static const struct intel_device_info intel_skylake_gt4_info = {
>   
>   #define GEN9_LP_FEATURES \
>   	GEN(9), \
> +	RINGS(RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING), \
>   	.is_lp = 1, \
>   	.has_hotplug = 1, \
> -	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING, \
>   	.num_pipes = 3, \
>   	.has_64bit_reloc = 1, \
>   	.has_ddi = 1, \
> @@ -554,8 +554,8 @@ static const struct intel_device_info intel_kabylake_gt2_info = {
>   
>   static const struct intel_device_info intel_kabylake_gt3_info = {
>   	KBL_PLATFORM,
> +	RINGS(RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING),
>   	.gt = 3,
> -	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
>   };
>   
>   #define CFL_PLATFORM \
> @@ -575,7 +575,7 @@ static const struct intel_device_info intel_coffeelake_gt2_info = {
>   static const struct intel_device_info intel_coffeelake_gt3_info = {
>   	CFL_PLATFORM,
>   	.gt = 3,
> -	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
> +	RINGS(RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING),
>   };
>   
>   #define GEN10_FEATURES \
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 0ad9184eba97..c65fce102d65 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -263,18 +263,12 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
>    */
>   int intel_engines_init_mmio(struct drm_i915_private *dev_priv)
>   {
> -	struct intel_device_info *device_info = mkwrite_device_info(dev_priv);
> -	const unsigned int ring_mask = INTEL_INFO(dev_priv)->ring_mask;
>   	struct intel_engine_cs *engine;
>   	enum intel_engine_id id;
>   	unsigned int mask = 0;
>   	unsigned int i;
>   	int err;
>   
> -	WARN_ON(ring_mask == 0);
> -	WARN_ON(ring_mask &
> -		GENMASK(sizeof(mask) * BITS_PER_BYTE - 1, I915_NUM_ENGINES));
> -
>   	for (i = 0; i < ARRAY_SIZE(intel_engines); i++) {
>   		if (!HAS_ENGINE(dev_priv, i))
>   			continue;
> @@ -291,17 +285,17 @@ int intel_engines_init_mmio(struct drm_i915_private *dev_priv)
>   	 * are added to the driver by a warning and disabling the forgotten
>   	 * engines.
>   	 */
> -	if (WARN_ON(mask != ring_mask))
> -		device_info->ring_mask = mask;
> +	if (GEM_WARN_ON(mask != INTEL_INFO(dev_priv)->ring_mask)) {
> +		err = -ENODEV;
> +		goto cleanup;
> +	}
>   
>   	/* We always presume we have at least RCS available for later probing */
> -	if (WARN_ON(!HAS_ENGINE(dev_priv, RCS))) {
> +	if (GEM_WARN_ON(!HAS_ENGINE(dev_priv, RCS))) {
>   		err = -ENODEV;
>   		goto cleanup;
>   	}
>   
> -	device_info->num_rings = hweight32(mask);
> -
>   	i915_check_and_clear_faults(dev_priv);
>   
>   	return 0;

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx