diff mbox

drm/i915: Replace has_bsd/blt/vebox with a mask

Message ID 1381808782-14214-1-git-send-email-benjamin.widawsky@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ben Widawsky Oct. 15, 2013, 3:46 a.m. UTC 
From: Ben Widawsky <ben@bwidawsk.net>

I've sent this patch several times for various reasons. It essentially
cleans up a lot of code where we need to do something per ring, and want
to query whether or not the ring exists on that hardware.

It has various uses coming up, but for now it shouldn't be too
offensive.

v2: Big conflict resolution on Damien's DEV_INFO_FOR_EACH stuff

v3: Resolved vebox addition

v4: Rebased after months of disuse. Also made failed ringbuffer init
cleaner.

CC: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.c | 32 ++++++++++++++++++++------------
 drivers/gpu/drm/i915/i915_drv.h | 14 ++++++++------
 drivers/gpu/drm/i915/i915_gem.c | 27 ++++++++++++++-------------
 3 files changed, 42 insertions(+), 31 deletions(-)

Comments

Chris Wilson Oct. 15, 2013, 8:50 a.m. UTC | #1 
On Mon, Oct 14, 2013 at 08:46:22PM -0700, Ben Widawsky wrote:
> -cleanup_vebox_ring:
> -	intel_cleanup_ring_buffer(&dev_priv->ring[VECS]);
> -cleanup_blt_ring:
> -	intel_cleanup_ring_buffer(&dev_priv->ring[BCS]);
> -cleanup_bsd_ring:
> -	intel_cleanup_ring_buffer(&dev_priv->ring[VCS]);
> -cleanup_render_ring:
> -	intel_cleanup_ring_buffer(&dev_priv->ring[RCS]);
> +cleanup:
> +	for_each_ring(ring, dev_priv, i) {
> +		if (!(INTEL_INFO(dev)->ring_mask & (1<<i)) ||
> +		    !ring->name)
> +			continue;

This looks dubious. You don't need to check ring_mask here as that will
be implicit in whatever we test for completeness. ring->name is set at
the start of initialisation and is not cleaned upon error. A better
choice is ring->obj, which we already check in
intel_cleanup_ring_buffer.

So this becomes:
cleanup:
  for_each_ring(ring, dev_priv, i)
> +		intel_cleanup_ring_buffer(ring);

-Chris
Ben Widawsky Oct. 15, 2013, 3:03 p.m. UTC | #2 
On Tue, Oct 15, 2013 at 09:50:39AM +0100, Chris Wilson wrote:
> On Mon, Oct 14, 2013 at 08:46:22PM -0700, Ben Widawsky wrote:
> > -cleanup_vebox_ring:
> > -	intel_cleanup_ring_buffer(&dev_priv->ring[VECS]);
> > -cleanup_blt_ring:
> > -	intel_cleanup_ring_buffer(&dev_priv->ring[BCS]);
> > -cleanup_bsd_ring:
> > -	intel_cleanup_ring_buffer(&dev_priv->ring[VCS]);
> > -cleanup_render_ring:
> > -	intel_cleanup_ring_buffer(&dev_priv->ring[RCS]);
> > +cleanup:
> > +	for_each_ring(ring, dev_priv, i) {
> > +		if (!(INTEL_INFO(dev)->ring_mask & (1<<i)) ||
> > +		    !ring->name)
> > +			continue;
> 
> This looks dubious. You don't need to check ring_mask here as that will
> be implicit in whatever we test for completeness. ring->name is set at
> the start of initialisation and is not cleaned upon error. A better
> choice is ring->obj, which we already check in
> intel_cleanup_ring_buffer.
> 
> So this becomes:
> cleanup:
>   for_each_ring(ring, dev_priv, i)
> > +		intel_cleanup_ring_buffer(ring);
> 
> -Chris
> 

Actually, I just plopped this code in at the last minute because I
wanted to provide a sample usage in the patch too. I do have some issues
in the future of using for_each_ring(), hopefully you remember those...

In any event, I'll gladly drop this hunk. Can you review the rest?

P.S. if you want my acked-by on this above mentioned cleanup, have it.
Chris Wilson Oct. 15, 2013, 3:08 p.m. UTC | #3 
On Tue, Oct 15, 2013 at 08:03:25AM -0700, Ben Widawsky wrote:
> On Tue, Oct 15, 2013 at 09:50:39AM +0100, Chris Wilson wrote:
> > On Mon, Oct 14, 2013 at 08:46:22PM -0700, Ben Widawsky wrote:
> > > -cleanup_vebox_ring:
> > > -	intel_cleanup_ring_buffer(&dev_priv->ring[VECS]);
> > > -cleanup_blt_ring:
> > > -	intel_cleanup_ring_buffer(&dev_priv->ring[BCS]);
> > > -cleanup_bsd_ring:
> > > -	intel_cleanup_ring_buffer(&dev_priv->ring[VCS]);
> > > -cleanup_render_ring:
> > > -	intel_cleanup_ring_buffer(&dev_priv->ring[RCS]);
> > > +cleanup:
> > > +	for_each_ring(ring, dev_priv, i) {
> > > +		if (!(INTEL_INFO(dev)->ring_mask & (1<<i)) ||
> > > +		    !ring->name)
> > > +			continue;
> > 
> > This looks dubious. You don't need to check ring_mask here as that will
> > be implicit in whatever we test for completeness. ring->name is set at
> > the start of initialisation and is not cleaned upon error. A better
> > choice is ring->obj, which we already check in
> > intel_cleanup_ring_buffer.
> > 
> > So this becomes:
> > cleanup:
> >   for_each_ring(ring, dev_priv, i)
> > > +		intel_cleanup_ring_buffer(ring);
> > 
> > -Chris
> > 
> 
> Actually, I just plopped this code in at the last minute because I
> wanted to provide a sample usage in the patch too. I do have some issues
> in the future of using for_each_ring(), hopefully you remember those...
> 
> In any event, I'll gladly drop this hunk. Can you review the rest?

No comments on the rest and lgtm, so
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index db84e24..e9dfadc 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -160,49 +160,58 @@  extern int intel_agp_enabled;
 static const struct intel_device_info intel_i830_info = {
 	.gen = 2, .is_mobile = 1, .cursor_needs_physical = 1, .num_pipes = 2,
 	.has_overlay = 1, .overlay_needs_physical = 1,
+	.ring_mask = RENDER_RING,
 };
 
 static const struct intel_device_info intel_845g_info = {
 	.gen = 2, .num_pipes = 1,
 	.has_overlay = 1, .overlay_needs_physical = 1,
+	.ring_mask = RENDER_RING,
 };
 
 static const struct intel_device_info intel_i85x_info = {
 	.gen = 2, .is_i85x = 1, .is_mobile = 1, .num_pipes = 2,
 	.cursor_needs_physical = 1,
 	.has_overlay = 1, .overlay_needs_physical = 1,
+	.ring_mask = RENDER_RING,
 };
 
 static const struct intel_device_info intel_i865g_info = {
 	.gen = 2, .num_pipes = 1,
 	.has_overlay = 1, .overlay_needs_physical = 1,
+	.ring_mask = RENDER_RING,
 };
 
 static const struct intel_device_info intel_i915g_info = {
 	.gen = 3, .is_i915g = 1, .cursor_needs_physical = 1, .num_pipes = 2,
 	.has_overlay = 1, .overlay_needs_physical = 1,
+	.ring_mask = RENDER_RING,
 };
 static const struct intel_device_info intel_i915gm_info = {
 	.gen = 3, .is_mobile = 1, .num_pipes = 2,
 	.cursor_needs_physical = 1,
 	.has_overlay = 1, .overlay_needs_physical = 1,
 	.supports_tv = 1,
+	.ring_mask = RENDER_RING,
 };
 static const struct intel_device_info intel_i945g_info = {
 	.gen = 3, .has_hotplug = 1, .cursor_needs_physical = 1, .num_pipes = 2,
 	.has_overlay = 1, .overlay_needs_physical = 1,
+	.ring_mask = RENDER_RING,
 };
 static const struct intel_device_info intel_i945gm_info = {
 	.gen = 3, .is_i945gm = 1, .is_mobile = 1, .num_pipes = 2,
 	.has_hotplug = 1, .cursor_needs_physical = 1,
 	.has_overlay = 1, .overlay_needs_physical = 1,
 	.supports_tv = 1,
+	.ring_mask = RENDER_RING,
 };
 
 static const struct intel_device_info intel_i965g_info = {
 	.gen = 4, .is_broadwater = 1, .num_pipes = 2,
 	.has_hotplug = 1,
 	.has_overlay = 1,
+	.ring_mask = RENDER_RING,
 };
 
 static const struct intel_device_info intel_i965gm_info = {
@@ -210,18 +219,20 @@  static const struct intel_device_info intel_i965gm_info = {
 	.is_mobile = 1, .has_fbc = 1, .has_hotplug = 1,
 	.has_overlay = 1,
 	.supports_tv = 1,
+	.ring_mask = RENDER_RING,
 };
 
 static const struct intel_device_info intel_g33_info = {
 	.gen = 3, .is_g33 = 1, .num_pipes = 2,
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.has_overlay = 1,
+	.ring_mask = RENDER_RING,
 };
 
 static const struct intel_device_info intel_g45_info = {
 	.gen = 4, .is_g4x = 1, .need_gfx_hws = 1, .num_pipes = 2,
 	.has_pipe_cxsr = 1, .has_hotplug = 1,
-	.has_bsd_ring = 1,
+	.ring_mask = RENDER_RING | BSD_RING,
 };
 
 static const struct intel_device_info intel_gm45_info = {
@@ -229,7 +240,7 @@  static const struct intel_device_info intel_gm45_info = {
 	.is_mobile = 1, .need_gfx_hws = 1, .has_fbc = 1,
 	.has_pipe_cxsr = 1, .has_hotplug = 1,
 	.supports_tv = 1,
-	.has_bsd_ring = 1,
+	.ring_mask = RENDER_RING | BSD_RING,
 };
 
 static const struct intel_device_info intel_pineview_info = {
@@ -241,21 +252,20 @@  static const struct intel_device_info intel_pineview_info = {
 static const struct intel_device_info intel_ironlake_d_info = {
 	.gen = 5, .num_pipes = 2,
 	.need_gfx_hws = 1, .has_hotplug = 1,
-	.has_bsd_ring = 1,
+	.ring_mask = RENDER_RING | BSD_RING,
 };
 
 static const struct intel_device_info intel_ironlake_m_info = {
 	.gen = 5, .is_mobile = 1, .num_pipes = 2,
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.has_fbc = 1,
-	.has_bsd_ring = 1,
+	.ring_mask = RENDER_RING | BSD_RING,
 };
 
 static const struct intel_device_info intel_sandybridge_d_info = {
 	.gen = 6, .num_pipes = 2,
 	.need_gfx_hws = 1, .has_hotplug = 1,
-	.has_bsd_ring = 1,
-	.has_blt_ring = 1,
+	.ring_mask = RENDER_RING | BSD_RING | BLT_RING,
 	.has_llc = 1,
 };
 
@@ -263,16 +273,14 @@  static const struct intel_device_info intel_sandybridge_m_info = {
 	.gen = 6, .is_mobile = 1, .num_pipes = 2,
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.has_fbc = 1,
-	.has_bsd_ring = 1,
-	.has_blt_ring = 1,
+	.ring_mask = RENDER_RING | BSD_RING | BLT_RING,
 	.has_llc = 1,
 };
 
 #define GEN7_FEATURES  \
 	.gen = 7, .num_pipes = 3, \
 	.need_gfx_hws = 1, .has_hotplug = 1, \
-	.has_bsd_ring = 1, \
-	.has_blt_ring = 1, \
+	.ring_mask = RENDER_RING | BSD_RING | BLT_RING, \
 	.has_llc = 1
 
 static const struct intel_device_info intel_ivybridge_d_info = {
@@ -315,7 +323,7 @@  static const struct intel_device_info intel_haswell_d_info = {
 	.is_haswell = 1,
 	.has_ddi = 1,
 	.has_fpga_dbg = 1,
-	.has_vebox_ring = 1,
+	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
 };
 
 static const struct intel_device_info intel_haswell_m_info = {
@@ -325,7 +333,7 @@  static const struct intel_device_info intel_haswell_m_info = {
 	.has_ddi = 1,
 	.has_fpga_dbg = 1,
 	.has_fbc = 1,
-	.has_vebox_ring = 1,
+	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
 };
 
 /*
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 15fe963..c08ca52 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -448,9 +448,6 @@  struct intel_uncore {
 	func(has_overlay) sep \
 	func(overlay_needs_physical) sep \
 	func(supports_tv) sep \
-	func(has_bsd_ring) sep \
-	func(has_blt_ring) sep \
-	func(has_vebox_ring) sep \
 	func(has_llc) sep \
 	func(has_ddi) sep \
 	func(has_fpga_dbg)
@@ -462,6 +459,7 @@  struct intel_device_info {
 	u32 display_mmio_offset;
 	u8 num_pipes:3;
 	u8 gen;
+	u8 ring_mask; /* Rings supported by the HW */
 	DEV_INFO_FOR_EACH_FLAG(DEFINE_FLAG, SEP_SEMICOLON);
 };
 
@@ -1688,9 +1686,13 @@  struct drm_i915_file_private {
 #define IS_GEN6(dev)	(INTEL_INFO(dev)->gen == 6)
 #define IS_GEN7(dev)	(INTEL_INFO(dev)->gen == 7)
 
-#define HAS_BSD(dev)            (INTEL_INFO(dev)->has_bsd_ring)
-#define HAS_BLT(dev)            (INTEL_INFO(dev)->has_blt_ring)
-#define HAS_VEBOX(dev)          (INTEL_INFO(dev)->has_vebox_ring)
+#define RENDER_RING		(1<<RCS)
+#define BSD_RING		(1<<VCS)
+#define BLT_RING		(1<<BCS)
+#define VEBOX_RING		(1<<VECS)
+#define HAS_BSD(dev)            (INTEL_INFO(dev)->ring_mask & BSD_RING)
+#define HAS_BLT(dev)            (INTEL_INFO(dev)->ring_mask & BLT_RING)
+#define HAS_VEBOX(dev)            (INTEL_INFO(dev)->ring_mask & VEBOX_RING)
 #define HAS_LLC(dev)            (INTEL_INFO(dev)->has_llc)
 #define HAS_WT(dev)            (IS_HASWELL(dev) && to_i915(dev)->ellc_size)
 #define I915_NEED_GFX_HWS(dev)	(INTEL_INFO(dev)->need_gfx_hws)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ab8293c..643ef27 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4363,7 +4363,8 @@  intel_enable_blt(struct drm_device *dev)
 static int i915_gem_init_rings(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int ret;
+	struct intel_ring_buffer *ring;
+	int ret, i;
 
 	ret = intel_init_render_ring_buffer(dev);
 	if (ret)
@@ -4372,36 +4373,36 @@  static int i915_gem_init_rings(struct drm_device *dev)
 	if (HAS_BSD(dev)) {
 		ret = intel_init_bsd_ring_buffer(dev);
 		if (ret)
-			goto cleanup_render_ring;
+			goto cleanup;
 	}
 
 	if (intel_enable_blt(dev)) {
 		ret = intel_init_blt_ring_buffer(dev);
 		if (ret)
-			goto cleanup_bsd_ring;
+			goto cleanup;
 	}
 
 	if (HAS_VEBOX(dev)) {
 		ret = intel_init_vebox_ring_buffer(dev);
 		if (ret)
-			goto cleanup_blt_ring;
+			goto cleanup;
 	}
 
 
 	ret = i915_gem_set_seqno(dev, ((u32)~0 - 0x1000));
 	if (ret)
-		goto cleanup_vebox_ring;
+		goto cleanup;
 
 	return 0;
 
-cleanup_vebox_ring:
-	intel_cleanup_ring_buffer(&dev_priv->ring[VECS]);
-cleanup_blt_ring:
-	intel_cleanup_ring_buffer(&dev_priv->ring[BCS]);
-cleanup_bsd_ring:
-	intel_cleanup_ring_buffer(&dev_priv->ring[VCS]);
-cleanup_render_ring:
-	intel_cleanup_ring_buffer(&dev_priv->ring[RCS]);
+cleanup:
+	for_each_ring(ring, dev_priv, i) {
+		if (!(INTEL_INFO(dev)->ring_mask & (1<<i)) ||
+		    !ring->name)
+			continue;
+
+		intel_cleanup_ring_buffer(ring);
+	}
 
 	return ret;
 }