Re: [PATCH] drm/i915: Clean up intel_plane->plane usage

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Sep 18, 2015 at 11:48:30AM +0300, Ville Syrjälä wrote:
> On Thu, Sep 17, 2015 at 06:55:21PM -0700, Matt Roper wrote:
> > intel_plane->plane has inconsistent meanings across the driver:
> >  * For primary planes, intel_plane->plane is usually the pipe number
> >    (except for old platforms where it had to be swapped for FBC
> >    reasons).
> >  * For sprite planes, intel_plane->plane represents the sprite index for
> >    the specific CRTC the sprite belongs to (0, 1, 2, ...)
> >  * For cursor planes, intel_plane->plane is again the pipe number,
> >    but without the FBC swapping on old platforms.
> > 
> > This overloading of the field can be quite confusing and easily leads to
> > bugs due to differences in how the hardware actually gets programmed
> > (e.g., SKL code needs to use p->plane + 1 because the hardware expects
> > universal plane ID's that include the primary, whereas VLV code needs to
> > use just p->plane because hardware expects a sprite index that does not
> > include the primary).
> > 
> > Let's try to clean up this situation by giving intel_plane->plane a
> > consistent meaning across all plane types, and then update all uses
> > across driver code to use macros to interpret it properly.  The
> > following macros should be used in the code where we previously accessed
> > p->plane and will do additional plane type checking to help prevent
> > misuse:
> >  * I915_UNIVERSAL_NUM(p) --- Universal plane ID (primary = 0, sprites
> >    start from 1); intended for use in gen9+ code.
> >  * I915_I915_PRIMARY_NUM(p) --- Primary plane ID for pre-gen9 platforms;
> >    determines whether FBC-related swapping is needed or not.
> >  * I915_SPRITE_NUM(p) --- Sprite plane ID (first sprite = 0); intended
> >    for use in pre-gen9 code.
> > 
> > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      | 24 ++++++++++++++++++------
> >  drivers/gpu/drm/i915/intel_display.c | 12 ++++--------
> >  drivers/gpu/drm/i915/intel_pm.c      | 10 +++++-----
> >  drivers/gpu/drm/i915/intel_sprite.c  | 13 +++++++------
> >  4 files changed, 34 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 3bf8a9b..132fe53 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -131,22 +131,34 @@ enum transcoder {
> >  #define transcoder_name(t) ((t) + 'A')
> >  
> >  /*
> > - * 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.
> > + * I915_MAX_PLANES in the enum below is the maximum (across all platforms)
> > + * number of planes per CRTC.  Not all platforms really have this many planes,
> > + * which means some arrays of size I915_MAX_PLANES may have unused entries
> > + * between the topmost sprite plane and the cursor plane.
> >   */
> > -#define I915_MAX_PLANES	4
> > -
> >  enum plane {
> >  	PLANE_A = 0,
> > +	PLANE_PRIMARY = PLANE_A,
> >  	PLANE_B,
> >  	PLANE_C,
> > +	PLANE_CURSOR,
> > +	I915_MAX_PLANES,
> >  };
> 
> If we're really going to redefine this as a per-pipe thing, then I
> think we'd need to rename it. A,B,C have a definite meaning on most of
> our hardware, so using it for something else is confusing.
> 
> I know that looking for enum plane in the code probably won't give you
> a lot of hits, but that's just because we suck and usually use int or
> something. I have a patch series in a branch somewhere to clean that
> stuff up, but I don't think I posted it to the list so far.
> 
> I think it would be better to have a separate per-pipe plane enum.
> enum plane_id or something? And maybe store it as plane->id.

Yeah, enum plane is for the legacy per-pipe plane, so converting that over
to a per-pipe plane enum for universal planes would result in tons of
confusion.

I think short term Ville's idea of adding a plane->id might be best, or
perhaps adding an enum universal_plane.

Also note that in the future we won't use the backwards compat cursor
plane on gen9+ since it just aliases with the last universal plane.
Insteaad that code should die and we'll just mark the last universal plane
with type = TYPE_CURSOR. Atm we can't expose that last plane due to
this aliasing.

> >  #define plane_name(p) ((p) + 'A')
> >  
> >  #define sprite_name(p, s) ((p) * INTEL_INFO(dev)->num_sprites[(p)] + (s) + 'A')
> >  
> > +#define I915_UNIVERSAL_NUM(p) ( \
> > +	WARN_ON(p->base.type == DRM_PLANE_TYPE_CURSOR), \
> 
> I don't think that'll fly with SKL. The cursor is just another universal
> plane, or well, will be. I think the patch to do that change got stuck
> in review or something.

Yup that patch is still lost somewhere :(

> I guess that should be easily fixable by tossing in a !IS_SKL&& once
> that patch lands.
> 
> > +	p->plane)
> > +#define I915_PRIMARY_NUM(p) ( \
> > +	WARN_ON(p->base.type != DRM_PLANE_TYPE_PRIMARY), \
> > +	(HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) ? \
> > +			      !p->pipe : p->pipe)
> 
> I'm not sure we want to do this swapping here.
> 
> Maybe we'll just want a some kind of
> enum plane plane_id_to_plane(crtc, enum plane_id) function.
> Or we could just store the enum plane alongside the plane_id in the
> plane, and make it clear that it should only be used by pre-SKL
> platforms. In theory we could limit it to pre-gen4, or just gmch, but
> I think we probably want to share a bunch of the primary plane code
> all the way up to BDW, so allowing it's use there seems sane to me.

Yeah, maybe rename enum plane to enum i9xx_plane (and same for all the
variables holding it). Same should be done probably for intel_crtc->plane.

The problem with that one really is that we can't just derive the plane
from the pipe because of fbc and fun stuff like that on gen2/3.

Then I think we should add a new intel_plane->id like Ville suggested with
the semantics you're using in your patch here.
-Daniel
> 
> > +#define I915_SPRITE_NUM(p) ( \
> > +	WARN_ON(p->base.type != DRM_PLANE_TYPE_OVERLAY), \
> > +	p->plane - 1) I think that should untangle this mess sufficiently.
> > +
> >  enum port {
> >  	PORT_A = 0,
> >  	PORT_B,
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index fc00867..dab0b71 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11976,16 +11976,14 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
> >  			DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d "
> >  				"disabled, scaler_id = %d\n",
> >  				plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD",
> > -				plane->base.id, intel_plane->pipe,
> > -				(crtc->base.primary == plane) ? 0 : intel_plane->plane + 1,
> > +				plane->base.id, intel_plane->pipe, intel_plane->plane,
> >  				drm_plane_index(plane), state->scaler_id);
> >  			continue;
> >  		}
> >  
> >  		DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d enabled",
> >  			plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD",
> > -			plane->base.id, intel_plane->pipe,
> > -			crtc->base.primary == plane ? 0 : intel_plane->plane + 1,
> > +			plane->base.id, intel_plane->pipe, intel_plane->plane,
> >  			drm_plane_index(plane));
> >  		DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = 0x%x",
> >  			fb->base.id, fb->width, fb->height, fb->pixel_format);
> > @@ -13547,13 +13545,11 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> >  		state->scaler_id = -1;
> >  	}
> >  	primary->pipe = pipe;
> > -	primary->plane = pipe;
> > +	primary->plane = 0;
> >  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
> >  	primary->check_plane = intel_check_primary_plane;
> >  	primary->commit_plane = intel_commit_primary_plane;
> >  	primary->disable_plane = intel_disable_primary_plane;
> > -	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> > -		primary->plane = !pipe;
> >  
> >  	if (INTEL_INFO(dev)->gen >= 9) {
> >  		intel_primary_formats = skl_primary_formats;
> > @@ -13699,7 +13695,7 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> >  	cursor->can_scale = false;
> >  	cursor->max_downscale = 1;
> >  	cursor->pipe = pipe;
> > -	cursor->plane = pipe;
> > +	cursor->plane = PLANE_CURSOR;
> >  	cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
> >  	cursor->check_plane = intel_check_cursor_plane;
> >  	cursor->commit_plane = intel_commit_cursor_plane;
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 62de97e..9db19f2 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -1131,7 +1131,7 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
> >  					wm_state->wm[level].primary;
> >  				break;
> >  			case DRM_PLANE_TYPE_OVERLAY:
> > -				sprite = plane->plane;
> > +				sprite = I915_SPRITE_NUM(plane);
> >  				wm_state->wm[level].sprite[sprite] = plane->wm.fifo_size -
> >  					wm_state->wm[level].sprite[sprite];
> >  				break;
> > @@ -1195,7 +1195,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> >  				wm_state->wm[level].primary = wm;
> >  				break;
> >  			case DRM_PLANE_TYPE_OVERLAY:
> > -				sprite = plane->plane;
> > +				sprite = I915_SPRITE_NUM(plane);
> >  				wm_state->wm[level].sprite[sprite] = wm;
> >  				break;
> >  			}
> > @@ -1221,7 +1221,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> >  					    wm_state->wm[level].primary);
> >  			break;
> >  		case DRM_PLANE_TYPE_OVERLAY:
> > -			sprite = plane->plane;
> > +			sprite = I915_SPRITE_NUM(plane);
> >  			for (level = 0; level < wm_state->num_levels; level++)
> >  				wm_state->sr[level].plane =
> >  					min(wm_state->sr[level].plane,
> > @@ -1257,7 +1257,7 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
> >  
> >  		if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> >  			sprite0_start = plane->wm.fifo_size;
> > -		else if (plane->plane == 0)
> > +		else if (I915_SPRITE_NUM(plane) == 0)
> >  			sprite1_start = sprite0_start + plane->wm.fifo_size;
> >  		else
> >  			fifo_size = sprite1_start + plane->wm.fifo_size;
> > @@ -4076,7 +4076,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
> >  			plane->wm.fifo_size = vlv_get_fifo_size(dev, plane->pipe, 0);
> >  			break;
> >  		case DRM_PLANE_TYPE_OVERLAY:
> > -			sprite = plane->plane;
> > +			sprite = I915_SPRITE_NUM(plane);
> >  			plane->wm.fifo_size = vlv_get_fifo_size(dev, plane->pipe, sprite + 1);
> >  			break;
> >  		}
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 4d27243..11ca40a 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -180,7 +180,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >  	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
> >  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> >  	const int pipe = intel_plane->pipe;
> > -	const int plane = intel_plane->plane + 1;
> > +	const int plane = I915_UNIVERSAL_NUM(intel_plane);
> >  	u32 plane_ctl, stride_div, stride;
> >  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> >  	const struct drm_intel_sprite_colorkey *key =
> > @@ -280,7 +280,7 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> >  	const int pipe = intel_plane->pipe;
> > -	const int plane = intel_plane->plane + 1;
> > +	const int plane = I915_UNIVERSAL_NUM(intel_plane);
> >  
> >  	I915_WRITE(PLANE_CTL(pipe, plane), 0);
> >  
> > @@ -294,7 +294,7 @@ static void
> >  chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
> >  {
> >  	struct drm_i915_private *dev_priv = intel_plane->base.dev->dev_private;
> > -	int plane = intel_plane->plane;
> > +	int plane = I915_SPRITE_NUM(intel_plane);
> >  
> >  	/* Seems RGB data bypasses the CSC always */
> >  	if (!format_is_yuv(format))
> > @@ -342,7 +342,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> >  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> >  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> >  	int pipe = intel_plane->pipe;
> > -	int plane = intel_plane->plane;
> > +	int plane = I915_SPRITE_NUM(intel_plane);
> >  	u32 sprctl;
> >  	unsigned long sprsurf_offset, linear_offset;
> >  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> > @@ -461,7 +461,7 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> >  	int pipe = intel_plane->pipe;
> > -	int plane = intel_plane->plane;
> > +	int plane = I915_SPRITE_NUM(intel_plane);
> >  
> >  	I915_WRITE(SPCNTR(pipe, plane), 0);
> >  
> > @@ -1121,8 +1121,9 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> >  		return -ENODEV;
> >  	}
> >  
> > +	/* primary = 0, sprites start from 1 */
> > +	intel_plane->plane = plane + 1;
> >  	intel_plane->pipe = pipe;
> > -	intel_plane->plane = plane;
> >  	intel_plane->frontbuffer_bit = INTEL_FRONTBUFFER_SPRITE(pipe, plane);
> >  	intel_plane->check_plane = intel_check_sprite_plane;
> >  	intel_plane->commit_plane = intel_commit_sprite_plane;
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux