| Message ID | 1425135249-3225-2-git-send-email-damien.lespiau@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Feb 28, 2015 at 02:54:08PM +0000, Damien Lespiau wrote: > Implicit usage of local variables in macros isn't exactly the greatest > thing in the world, especially when that variable is the drm device and > we want to move towards a broader use of the i915 device structure. > > Let's make for_each_plane() take dev_priv as its first argument then. > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> Both patches are Reviewed-by: Chris Wilson <chris-wilson.co.uk> Just a quick question... > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index e07a1cb..c204e30 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -223,8 +223,10 @@ enum hpd_pin { > > #define for_each_pipe(__dev_priv, __p) \ > for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++) > -#define for_each_plane(pipe, p) \ > - for ((p) = 0; (p) < INTEL_INFO(dev)->num_sprites[(pipe)] + 1; (p)++) > +#define for_each_plane(__dev_priv, __pipe, __p) \ > + for ((__p) = 0; \ > + (__p) < INTEL_INFO(__dev_priv)->num_sprites[(__pipe)] + 1; \ > + (__p)++) > #define for_each_sprite(p, s) for ((s) = 0; (s) < INTEL_INFO(dev)->num_sprites[(p)]; (s)++) Is num_sprites explained anywhere? In particular the plane = num_sprites+1? -Chris
On Sat, Feb 28, 2015 at 09:04:44PM +0000, Chris Wilson wrote: > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index e07a1cb..c204e30 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -223,8 +223,10 @@ enum hpd_pin { > > > > #define for_each_pipe(__dev_priv, __p) \ > > for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++) > > -#define for_each_plane(pipe, p) \ > > - for ((p) = 0; (p) < INTEL_INFO(dev)->num_sprites[(pipe)] + 1; (p)++) > > +#define for_each_plane(__dev_priv, __pipe, __p) \ > > + for ((__p) = 0; \ > > + (__p) < INTEL_INFO(__dev_priv)->num_sprites[(__pipe)] + 1; \ > > + (__p)++) > > #define for_each_sprite(p, s) for ((s) = 0; (s) < INTEL_INFO(dev)->num_sprites[(p)]; (s)++) > > Is num_sprites explained anywhere? In particular the plane = num_sprites+1? > -Chris There's a tentative explanation also talking about the cursor plane: /* * This is the maximum (across all platforms) number of planes (primary + * sprites) that can be active at the same time on one pipe. * * This value doesn't count the cursor plane. */ #define I915_MAX_PLANES 3 But agreed that this couple be improved.
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index e4d5f41..6717052 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2854,7 +2854,7 @@ static int i915_ddb_info(struct seq_file *m, void *unused) for_each_pipe(dev_priv, pipe) { seq_printf(m, "Pipe %c\n", pipe_name(pipe)); - for_each_plane(pipe, plane) { + for_each_plane(dev_priv, pipe, plane) { entry = &ddb->plane[pipe][plane]; seq_printf(m, " Plane%-8d%8u%8u%8u\n", plane + 1, entry->start, entry->end, diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index e07a1cb..c204e30 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -223,8 +223,10 @@ enum hpd_pin { #define for_each_pipe(__dev_priv, __p) \ for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++) -#define for_each_plane(pipe, p) \ - for ((p) = 0; (p) < INTEL_INFO(dev)->num_sprites[(pipe)] + 1; (p)++) +#define for_each_plane(__dev_priv, __pipe, __p) \ + for ((__p) = 0; \ + (__p) < INTEL_INFO(__dev_priv)->num_sprites[(__pipe)] + 1; \ + (__p)++) #define for_each_sprite(p, s) for ((s) = 0; (s) < INTEL_INFO(dev)->num_sprites[(p)]; (s)++) #define for_each_crtc(dev, crtc) \ diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3156d77..23c3a37 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -10806,7 +10806,7 @@ static void check_wm_state(struct drm_device *dev) continue; /* planes */ - for_each_plane(pipe, plane) { + for_each_plane(dev_priv, pipe, plane) { hw_entry = &hw_ddb.plane[pipe][plane]; sw_entry = &sw_ddb->plane[pipe][plane]; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 0bf6767..e710b43 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2463,13 +2463,12 @@ static void skl_ddb_entry_init_from_hw(struct skl_ddb_entry *entry, u32 reg) void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv, struct skl_ddb_allocation *ddb /* out */) { - struct drm_device *dev = dev_priv->dev; enum pipe pipe; int plane; u32 val; for_each_pipe(dev_priv, pipe) { - for_each_plane(pipe, plane) { + for_each_plane(dev_priv, pipe, plane) { val = I915_READ(PLANE_BUF_CFG(pipe, plane)); skl_ddb_entry_init_from_hw(&ddb->plane[pipe][plane], val); @@ -2518,6 +2517,7 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc, struct skl_ddb_allocation *ddb /* out */) { struct drm_device *dev = crtc->dev; + struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); enum pipe pipe = intel_crtc->pipe; struct skl_ddb_entry *alloc = &ddb->pipe[pipe]; @@ -2542,7 +2542,7 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc, alloc->end -= cursor_blocks; /* 1. Allocate the mininum required blocks for each active plane */ - for_each_plane(pipe, plane) { + for_each_plane(dev_priv, pipe, plane) { const struct intel_plane_wm_parameters *p; p = ¶ms->plane[plane]; @@ -2996,12 +2996,11 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv, static void skl_wm_flush_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, int pass) { - struct drm_device *dev = dev_priv->dev; int plane; DRM_DEBUG_KMS("flush pipe %c (pass %d)\n", pipe_name(pipe), pass); - for_each_plane(pipe, plane) { + for_each_plane(dev_priv, pipe, plane) { I915_WRITE(PLANE_SURF(pipe, plane), I915_READ(PLANE_SURF(pipe, plane))); }
Implicit usage of local variables in macros isn't exactly the greatest thing in the world, especially when that variable is the drm device and we want to move towards a broader use of the i915 device structure. Let's make for_each_plane() take dev_priv as its first argument then. Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 2 +- drivers/gpu/drm/i915/i915_drv.h | 6 ++++-- drivers/gpu/drm/i915/intel_display.c | 2 +- drivers/gpu/drm/i915/intel_pm.c | 9 ++++----- 4 files changed, 10 insertions(+), 9 deletions(-)