Re: [Intel-gfx] [PATCH 4/4] drm/i915: Disable dynamic setup of device_info->num_rings
On Sat, Feb 10, 2018 at 08:39:08AM +, Chris Wilson wrote: > Quoting Oscar Mateo (2018-02-09 23:46:31) > > > > 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? > > Hmm, ring_mask is more widely used than I was expecting. I think we want > both, static_info if we ever think we can benefit from single-platform > LTO of the engines, but whether to use runtime_info or i915->gt.engine_mask > (and move the engine maps to i915->gt as well). > > Advantage of runtime_info, centralised place for debugging. > Disadvantage of runtime_info, centralised place far from code. > > Maybe we don't need to say everything is inside runtime_info (just > anything that doesn't fit elsewhere?), but use the hooks for debugging? > Maybe having a central runtime_info is simply a bad idea? Yeah, I don't like the "far from the code" aspect of runtime_info (or even the static info in some cases). Another counter argument is perhaps that people are more likely to update the info for new platforms if it's in a central location, as opposed having to trawl the entire codebase. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Disable dynamic setup of device_info->num_rings
Quoting Oscar Mateo (2018-02-09 23:46:31) > > 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? Hmm, ring_mask is more widely used than I was expecting. I think we want both, static_info if we ever think we can benefit from single-platform LTO of the engines, but whether to use runtime_info or i915->gt.engine_mask (and move the engine maps to i915->gt as well). Advantage of runtime_info, centralised place for debugging. Disadvantage of runtime_info, centralised place far from code. Maybe we don't need to say everything is inside runtime_info (just anything that doesn't fit elsewhere?), but use the hooks for debugging? Maybe having a central runtime_info is simply a bad idea? -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Disable dynamic setup of device_info->num_rings
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
Cc: Tvrtko Ursulin
---
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_R
[Intel-gfx] [PATCH 4/4] drm/i915: Disable dynamic setup of device_info->num_rings
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.
Signed-off-by: Chris Wilson
Cc: Tvrtko Ursulin
---
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 = {
