| Message ID | 1399637360-4277-3-git-send-email-oscar.mateo@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 09, 2014 at 01:08:32PM +0100, oscar.mateo@intel.com wrote: > From: Ben Widawsky <benjamin.widawsky@intel.com> > > for_each_ring() iterates over all rings supported by the hardware, not > just those which have been initialized as in for_each_active_ring() > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > Acked-by: Oscar Mateo <oscar.mateo@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index a53a028..b1725c6 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1544,6 +1544,17 @@ static inline struct drm_i915_private *to_i915(const struct drm_device *dev) > return dev->dev_private; > } > > +/* NB: Typically you want to use for_each_ring in init code before ringbuffers > + * are setup, or in debug code. for_each_active_ring is more suited for code > + * which is dynamically handling active rings, ie. normal code. In most > + * (currently all cases except on pre-production hardware) for_each_ring will > + * work even if it's a bad idea to use it - so be careful. > + */ Proper kerneldoc comment would look neater imo instead of an "NB:". Bonus points if you pull it into the drm docbook (just the header files with the !I directive in the DocBook template). -Daniel > +#define for_each_ring(ring__, dev_priv__, i__) \ > + for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \ > + if (((ring__) = &(dev_priv__)->ring[(i__)]), \ > + INTEL_INFO((dev_priv__)->dev)->ring_mask & (1<<(i__))) > + > /* Iterate over initialised rings */ > #define for_each_active_ring(ring__, dev_priv__, i__) \ > for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \ > -- > 1.9.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, May 09, 2014 at 05:08:32AM -0700, oscar.mateo@intel.com wrote: > From: Ben Widawsky <benjamin.widawsky@intel.com> > > for_each_ring() iterates over all rings supported by the hardware, not > just those which have been initialized as in for_each_active_ring() I think we should give this a new name; something like for_each_supported_ring. My concern is that, with all of the patches in flight, we'll merge something that uses for_each_ring when it should have been changed to for_each_active_ring. Better that such a patch not even compile. Brad > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > Acked-by: Oscar Mateo <oscar.mateo@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index a53a028..b1725c6 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1544,6 +1544,17 @@ static inline struct drm_i915_private *to_i915(const struct drm_device *dev) > return dev->dev_private; > } > > +/* NB: Typically you want to use for_each_ring in init code before ringbuffers > + * are setup, or in debug code. for_each_active_ring is more suited for code > + * which is dynamically handling active rings, ie. normal code. In most > + * (currently all cases except on pre-production hardware) for_each_ring will > + * work even if it's a bad idea to use it - so be careful. > + */ > +#define for_each_ring(ring__, dev_priv__, i__) \ > + for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \ > + if (((ring__) = &(dev_priv__)->ring[(i__)]), \ > + INTEL_INFO((dev_priv__)->dev)->ring_mask & (1<<(i__))) > + > /* Iterate over initialised rings */ > #define for_each_active_ring(ring__, dev_priv__, i__) \ > for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \ > -- > 1.9.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -----Original Message----- > From: Volkin, Bradley D > Sent: Monday, May 19, 2014 5:34 PM > To: Mateo Lozano, Oscar > Cc: intel-gfx@lists.freedesktop.org; Ben Widawsky; Widawsky, Benjamin > Subject: Re: [Intel-gfx] [PATCH 02/50] drm/i915: for_each_ring > > On Fri, May 09, 2014 at 05:08:32AM -0700, oscar.mateo@intel.com wrote: > > From: Ben Widawsky <benjamin.widawsky@intel.com> > > > > for_each_ring() iterates over all rings supported by the hardware, not > > just those which have been initialized as in for_each_active_ring() > > I think we should give this a new name; something like > for_each_supported_ring. > My concern is that, with all of the patches in flight, we'll merge something that > uses for_each_ring when it should have been changed to for_each_active_ring. > Better that such a patch not even compile. I can kill this patch off completely: when we started with Execlists, it simplified a lot of things and made life easier in general, but with each new iteration it just becomes more and more useless... > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > Acked-by: Oscar Mateo <oscar.mateo@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h index a53a028..b1725c6 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1544,6 +1544,17 @@ static inline struct drm_i915_private > *to_i915(const struct drm_device *dev) > > return dev->dev_private; > > } > > > > +/* NB: Typically you want to use for_each_ring in init code before > > +ringbuffers > > + * are setup, or in debug code. for_each_active_ring is more suited > > +for code > > + * which is dynamically handling active rings, ie. normal code. In > > +most > > + * (currently all cases except on pre-production hardware) > > +for_each_ring will > > + * work even if it's a bad idea to use it - so be careful. > > + */ > > +#define for_each_ring(ring__, dev_priv__, i__) \ > > + for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \ > > + if (((ring__) = &(dev_priv__)->ring[(i__)]), \ > > + INTEL_INFO((dev_priv__)->dev)->ring_mask & (1<<(i__))) > > + > > /* Iterate over initialised rings */ > > #define for_each_active_ring(ring__, dev_priv__, i__) \ > > for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \ > > -- > > 1.9.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index a53a028..b1725c6 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1544,6 +1544,17 @@ static inline struct drm_i915_private *to_i915(const struct drm_device *dev) return dev->dev_private; } +/* NB: Typically you want to use for_each_ring in init code before ringbuffers + * are setup, or in debug code. for_each_active_ring is more suited for code + * which is dynamically handling active rings, ie. normal code. In most + * (currently all cases except on pre-production hardware) for_each_ring will + * work even if it's a bad idea to use it - so be careful. + */ +#define for_each_ring(ring__, dev_priv__, i__) \ + for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \ + if (((ring__) = &(dev_priv__)->ring[(i__)]), \ + INTEL_INFO((dev_priv__)->dev)->ring_mask & (1<<(i__))) + /* Iterate over initialised rings */ #define for_each_active_ring(ring__, dev_priv__, i__) \ for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \