Re: [PATCH v4 4/8] drm/i915: Overcome display engine stride limits via GTT remapping

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jan 30, 2019 at 10:59:04PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 30, 2019 at 08:01:21PM +0100, Daniel Vetter wrote:
> > On Thu, Jan 24, 2019 at 08:58:49PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > The display engine stride limits are getting in our way. On SKL+
> > > we are limited to 8k pixels, which is easily exceeded with three
> > > 4k displays. To overcome this limitation we can remap the pages
> > > in the GTT to provide the display engine with a view of memory
> > > with a smaller stride.
> > > 
> > > The code is mostly already there as We already play tricks with
> > > the plane surface address and x/y offsets.
> > > 
> > > A few caveats apply:
> > > * linear buffers need the fb stride to be page aligned, as
> > >   otherwise the remapped lines wouldn't start at the same
> > >   spot
> > > * compressed buffers can't be remapped due to the new
> > >   ccs hash mode causing the virtual address of the pages
> > >   to affect the interpretation of the compressed data. IIRC
> > >   the old hash was limited to the low 12 bits so if we were
> > >   using that mode we could remap. As it stands we just refuse
> > >   to remapp with compressed fbs.
> > > * no remapping gen2/3 as we'd need a fence for the remapped
> > >   vma, which we currently don't have. Need to deal with the
> > >   fence POT requirements, and do something about the gen2
> > >   gtt page size vs tile size difference
> > > 
> > > v2: Rebase due to is_ccs_modifier()
> > >     Fix up the skl+ stride_mult mess
> > >     memset() the gtt_view because otherwise we could leave
> > >     junk in plane[1] when going from 2 plane to 1 plane format
> > > v3: intel_check_plane_stride() was split out
> > > v4: Drop the aligned viewport stuff, it was meant for ccs which
> > >     can't be remapped anyway
> > > v5: Introduce intel_plane_can_remap()
> > >     Reorder the code so that plane_state->view gets filled
> > >     even for invisible planes, otherwise we'd keep using
> > >     stale values and could explode during remapping. The new
> > >     logic never remaps invisible planes since we don't have
> > >     a viewport, and instead pins the full fb instead
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 393 +++++++++++++++++++++------
> > >  drivers/gpu/drm/i915/intel_drv.h     |   1 +
> > >  drivers/gpu/drm/i915/intel_sprite.c  |  34 ++-
> > >  3 files changed, 334 insertions(+), 94 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 17c7edee9584..3713b6f1796e 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -1865,7 +1865,7 @@ intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane)
> > >  
> > >  	switch (fb->modifier) {
> > >  	case DRM_FORMAT_MOD_LINEAR:
> > > -		return cpp;
> > > +		return intel_tile_size(dev_priv);
> > >  	case I915_FORMAT_MOD_X_TILED:
> > >  		if (IS_GEN(dev_priv, 2))
> > >  			return 128;
> > > @@ -1908,11 +1908,8 @@ intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane)
> > >  static unsigned int
> > >  intel_tile_height(const struct drm_framebuffer *fb, int color_plane)
> > >  {
> > > -	if (fb->modifier == DRM_FORMAT_MOD_LINEAR)
> > > -		return 1;
> > > -	else
> > > -		return intel_tile_size(to_i915(fb->dev)) /
> > > -			intel_tile_width_bytes(fb, color_plane);
> > > +	return intel_tile_size(to_i915(fb->dev)) /
> > > +		intel_tile_width_bytes(fb, color_plane);
> > >  }
> > >  
> > >  /* Return the tile dimensions in pixel units */
> > > @@ -2170,16 +2167,8 @@ void intel_add_fb_offsets(int *x, int *y,
> > >  			  int color_plane)
> > >  
> > >  {
> > > -	const struct intel_framebuffer *intel_fb = to_intel_framebuffer(state->base.fb);
> > > -	unsigned int rotation = state->base.rotation;
> > > -
> > > -	if (drm_rotation_90_or_270(rotation)) {
> > > -		*x += intel_fb->rotated[color_plane].x;
> > > -		*y += intel_fb->rotated[color_plane].y;
> > > -	} else {
> > > -		*x += intel_fb->normal[color_plane].x;
> > > -		*y += intel_fb->normal[color_plane].y;
> > > -	}
> > > +	*x += state->color_plane[color_plane].x;
> > > +	*y += state->color_plane[color_plane].y;
> > >  }
> > >  
> > >  static u32 intel_adjust_tile_offset(int *x, int *y,
> > > @@ -2459,6 +2448,119 @@ bool is_ccs_modifier(u64 modifier)
> > >  	       modifier == I915_FORMAT_MOD_Yf_TILED_CCS;
> > >  }
> > >  
> > > +static
> > > +u32 intel_plane_fb_max_stride(struct drm_i915_private *dev_priv,
> > > +			      u32 pixel_format, u64 modifier)
> > > +{
> > > +	struct intel_crtc *crtc;
> > > +	struct intel_plane *plane;
> > > +
> > > +	/*
> > > +	 * We assume the primary plane for pipe A has
> > > +	 * the highest stride limits of them all.
> > > +	 */
> > > +	crtc = intel_get_crtc_for_pipe(dev_priv, PIPE_A);
> > > +	plane = to_intel_plane(crtc->base.primary);
> > > +
> > > +	return plane->max_stride(plane, pixel_format, modifier,
> > > +				 DRM_MODE_ROTATE_0);
> > > +}
> > > +
> > > +static
> > > +u32 intel_fb_max_stride(struct drm_i915_private *dev_priv,
> > > +			u32 pixel_format, u64 modifier)
> > > +{
> > > +	return intel_plane_fb_max_stride(dev_priv, pixel_format, modifier);
> > > +}
> > > +
> > > +static u32
> > > +intel_fb_stride_alignment(const struct drm_framebuffer *fb, int color_plane)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(fb->dev);
> > > +
> > > +	if (fb->modifier == DRM_FORMAT_MOD_LINEAR) {
> > > +		u32 max_stride = intel_plane_fb_max_stride(dev_priv,
> > > +							   fb->format->format,
> > > +							   fb->modifier);
> > > +
> > > +		/*
> > > +		 * To make remapping with linear generally feasible
> > > +		 * we need the stride to be page aligned.
> > > +		 */
> > > +		if (fb->pitches[color_plane] > max_stride)
> > > +			return intel_tile_size(dev_priv);
> > > +		else
> > > +			return 64;
> > > +	} else {
> > > +		return intel_tile_width_bytes(fb, color_plane);
> > > +	}
> > > +}
> > > +
> > > +bool intel_plane_can_remap(const struct intel_plane_state *plane_state)
> > > +{
> > > +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > +	const struct drm_framebuffer *fb = plane_state->base.fb;
> > > +	int i;
> > > +
> > > +	/* We don't want to deal with remapping with cursors */
> > > +	if (plane->id == PLANE_CURSOR)
> > > +		return false;
> > 
> > They should be caught in the stride check below anyway ... why special
> > case?
> 
> I guess we could just drop this. Hmm, yeah even i845/i865 with
> their max cursor stride of 2k should get rejected later.
> 
> > 
> > > +
> > > +	/*
> > > +	 * The dsplay engine limits already match the render
> > > +	 * engine limits, so not much point in remapping.
> > > +	 * Would also need to deal with the fence POT alignment
> > > +	 * and gen2 2KiB GTT tile size.
> > > +	 */
> > > +	if (INTEL_GEN(dev_priv) < 4)
> > > +		return false;
> > > +
> > > +	/*
> > > +	 * The new CCS hash mode isn't compatible with remapping as
> > > +	 * the virtual address of the pages affects the compressed data.
> > > +	 */
> > > +	if (is_ccs_modifier(fb->modifier))
> > > +		return false;
> > > +
> > > +	/* Linear needs a page aligned stride for remapping */
> > > +	if (fb->modifier == DRM_FORMAT_MOD_LINEAR) {
> > 
> > Not sure whether cramming linear formats into the same macheniry is really
> > clever in a good way or bad way (because too tricky). I guess it works,
> > and this is not something that's well explaing in some comments sprinkled
> > all over.
> > 
> > *shrug*
> > 
> > > +		unsigned int alignment = intel_tile_size(dev_priv) - 1;
> > > +
> > > +		for (i = 0; i < fb->format->num_planes; i++) {
> > > +			if (fb->pitches[i] & alignment)
> > > +				return false;
> > > +		}
> > > +	}
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +static bool intel_plane_needs_remap(const struct intel_plane_state *plane_state)
> > > +{
> > > +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> > > +	const struct drm_framebuffer *fb = plane_state->base.fb;
> > > +	unsigned int rotation = plane_state->base.rotation;
> > > +	u32 stride, max_stride;
> > > +
> > > +	/*
> > > +	 * No remapping for invisible planes since we don't have
> > > +	 * an actual source viewport to remap.
> > > +	 */
> > > +	if (!plane_state->base.visible)
> > > +		return false;
> > > +
> > > +	if (!intel_plane_can_remap(plane_state))
> > > +		return false;
> > > +
> > > +	/* FIXME other color planes? */
> > 
> > Should be simple to fix if we do a similar loop like in can_remap above.
> > Just true if any of them are bigger than max stride.
> 
> That's assuming the max stride for each plane is the same. Which
> it might not be. IIRC the skl+ docs didn't really say what the max
> aux stride is.

Ugh. In that case probably better to make that clear in the comment, e.g.

	/* FIXME: aux plane limits on gen9+ are unclear in Bspec, for now
	 * no checking. */

Otoh there's no reasonable scenario where they're wider, so might still be
better to at least check for that.

> > > +	stride = intel_fb_pitch(fb, 0, rotation);
> > > +	max_stride = plane->max_stride(plane, fb->format->format,
> > > +				       fb->modifier, rotation);
> > > +
> > > +	return stride > max_stride;
> > > +}
> > > +
> > >  static int
> > >  intel_fill_fb_info(struct drm_i915_private *dev_priv,
> > >  		   struct drm_framebuffer *fb)
> > > @@ -2624,6 +2726,172 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
> > >  	return 0;
> > >  }
> > >  
> > > +static void
> > > +intel_plane_remap_gtt(struct intel_plane_state *plane_state)
> > > +{
> > > +	struct drm_i915_private *dev_priv =
> > > +		to_i915(plane_state->base.plane->dev);
> > > +	struct drm_framebuffer *fb = plane_state->base.fb;
> > > +	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > > +	struct intel_rotation_info *info = &plane_state->view.rotated;
> > > +	unsigned int rotation = plane_state->base.rotation;
> > > +	int i, num_planes = fb->format->num_planes;
> > > +	unsigned int tile_size = intel_tile_size(dev_priv);
> > > +	unsigned int src_x, src_y;
> > > +	unsigned int src_w, src_h;
> > > +	u32 gtt_offset = 0;
> > > +
> > > +	memset(&plane_state->view, 0, sizeof(plane_state->view));
> > > +	plane_state->view.type = drm_rotation_90_or_270(rotation) ?
> > > +		I915_GGTT_VIEW_ROTATED : I915_GGTT_VIEW_REMAPPED;
> > > +
> > > +	src_x = plane_state->base.src.x1 >> 16;
> > > +	src_y = plane_state->base.src.y1 >> 16;
> > > +	src_w = drm_rect_width(&plane_state->base.src) >> 16;
> > > +	src_h = drm_rect_height(&plane_state->base.src) >> 16;
> > > +
> > > +	WARN_ON(is_ccs_modifier(fb->modifier));
> > > +
> > > +	/* Make src coordinates relative to the viewport */
> > > +	drm_rect_translate(&plane_state->base.src,
> > > +			   -(src_x << 16), -(src_y << 16));
> > > +
> > > +	/* Rotate src coordinates to match rotated GTT view */
> > > +	if (drm_rotation_90_or_270(rotation))
> > > +		drm_rect_rotate(&plane_state->base.src,
> > > +				src_w << 16, src_h << 16,
> > > +				DRM_MODE_ROTATE_270);
> > > +
> > > +	for (i = 0; i < num_planes; i++) {
> > > +		unsigned int hsub = i ? fb->format->hsub : 1;
> > > +		unsigned int vsub = i ? fb->format->vsub : 1;
> > > +		unsigned int cpp = fb->format->cpp[i];
> > > +		unsigned int tile_width, tile_height;
> > > +		unsigned int width, height;
> > > +		unsigned int pitch_tiles;
> > > +		unsigned int x, y;
> > > +		u32 offset;
> > > +
> > > +		intel_tile_dims(fb, i, &tile_width, &tile_height);
> > > +
> > > +		x = src_x / hsub;
> > > +		y = src_y / vsub;
> > > +		width = src_w / hsub;
> > > +		height = src_h / vsub;
> > > +
> > > +		/*
> > > +		 * First pixel of the src viewport from the
> > > +		 * start of the normal gtt mapping.
> > > +		 */
> > > +		x += intel_fb->normal[i].x;
> > > +		y += intel_fb->normal[i].y;
> > > +
> > > +		offset = intel_compute_aligned_offset(dev_priv, &x, &y,
> > > +						      fb, i, fb->pitches[i],
> > > +						      DRM_MODE_ROTATE_0, tile_size);
> > > +		offset /= tile_size;
> > > +
> > > +		info->plane[i].offset = offset;
> > > +		info->plane[i].stride = DIV_ROUND_UP(fb->pitches[i],
> > > +						     tile_width * cpp);
> > > +		info->plane[i].width = DIV_ROUND_UP(x + width, tile_width);
> > > +		info->plane[i].height = DIV_ROUND_UP(y + height, tile_height);
> > > +
> > > +		if (drm_rotation_90_or_270(rotation)) {
> > > +			struct drm_rect r;
> > > +
> > > +			/* rotate the x/y offsets to match the GTT view */
> > > +			r.x1 = x;
> > > +			r.y1 = y;
> > > +			r.x2 = x + width;
> > > +			r.y2 = y + height;
> > > +			drm_rect_rotate(&r,
> > > +					info->plane[i].width * tile_width,
> > > +					info->plane[i].height * tile_height,
> > > +					DRM_MODE_ROTATE_270);
> > > +			x = r.x1;
> > > +			y = r.y1;
> > > +
> > > +			pitch_tiles = info->plane[i].height;
> > > +			plane_state->color_plane[i].stride = pitch_tiles * tile_height;
> > > +
> > > +			/* rotate the tile dimensions to match the GTT view */
> > > +			swap(tile_width, tile_height);
> > > +		} else {
> > > +			pitch_tiles = info->plane[i].width;
> > > +			plane_state->color_plane[i].stride = pitch_tiles * tile_width * cpp;
> > > +		}
> > > +
> > > +		/*
> > > +		 * We only keep the x/y offsets, so push all of the
> > > +		 * gtt offset into the x/y offsets.
> > > +		 */
> > > +		intel_adjust_tile_offset(&x, &y,
> > > +					 tile_width, tile_height,
> > > +					 tile_size, pitch_tiles,
> > > +					 gtt_offset * tile_size, 0);
> > > +
> > > +		gtt_offset += info->plane[i].width * info->plane[i].height;
> > > +
> > > +		plane_state->color_plane[i].offset = 0;
> > > +		plane_state->color_plane[i].x = x;
> > > +		plane_state->color_plane[i].y = y;
> > > +	}
> > > +}
> > 
> > Validating this freaks me out. Keeping it working freaks me out even more,
> > there's pretty much a guarantee that we don't.
> > 
> > Also, I'm not sure CI exercises this much even with your hacks. Most of
> > our kms tests use single-crtc buffers, so no massive overallocation, so no
> > real need for views. The selftests are good, but they don't cover the
> > massive pile of pixel/coordination frobbing above.
> > 
> > I think only thing that can validate this is:
> > - a pile of igts that make sure we overallocate plentiful, while still
> >   exercising all the major fb layouts (rotated, all the different cpp,
> >   tiling formats, and throw in a ccs for lols to make sure nothing blows
> >   up).
> > - kernel patch to prefer remmaped over normal. Don't move your buffers
> >   around too much though :-)
> > 
> > Now the second part isn't really a real world thing, since even with
> > y-tiled that means remapping the view every 32 pixels (on average, worse
> > if your edges aren't aligned to 32 pixels, then it's twice as much).
> > 
> > So I think the only thing that works for validating this beast is a pile
> > of new igts that allocate dumb&tiled buffers at the size limit, and do all
> > the rotations/scaling/moving tests we already have.
> > 
> > Yes this is painful.
> 
> One test that I did write was kms_big_fb, which allocates a huge fb and
> pans around it with all rotations and modifiers. But the version I
> posted was pretty old and I need to look into cleaning up any later
> changes I had. The other annoying thing is that generating that huge
> fb is pretty slow. Should maybe look into minimizing the stuff we render
> to it to speed it up.

16*16 is just 1GB, that should still be ok to clflush and all that. As
long as we don't use pixman to render to it.

What I had in mind is just kms_rotation, but added big_fb subtests for at
least some of them, with the only difference that we allocate a huge fb.
But still render only into the fairly tiny visible portion. Iirc we even
have a few of those "bigger than necessary fb" tests in kms_rotation, to
make sure the remapping isn't broken in these corner-cases.

Since remapped view/fb is exactly the same problem as rotated, except not
rotated, I think reusing that test as much as possible simplifies the
reviewing.

Another thing: If we do the dumb_create change I recommend then it should
Just Work: MODIFIER_NONE is allocated through dumb, so should pick up the
stride restrictions automatically, and other modifiers work automatically
because tiles. I think that should be the quickest way to get good enough
test coverage without having to review an entire new igt.

Only gap would be a test for dumb_create to check it does the right thing,
but I'm ok with shrugging that one off.
-Daniel

> 
> > 
> > > +
> > > +static int
> > > +intel_plane_compute_gtt(struct intel_plane_state *plane_state)
> > > +{
> > > +	const struct intel_framebuffer *fb =
> > > +		to_intel_framebuffer(plane_state->base.fb);
> > > +	unsigned int rotation = plane_state->base.rotation;
> > > +	int i, num_planes;
> > > +	int ret;
> > > +
> > > +	if (!fb)
> > > +		return 0;
> > > +
> > > +	num_planes = fb->base.format->num_planes;
> > > +
> > > +	if (intel_plane_needs_remap(plane_state)) {
> > > +		intel_plane_remap_gtt(plane_state);
> > > +
> > > +		/* Remapping should take care of this always */
> > > +		ret = intel_plane_check_stride(plane_state);
> > > +		if (WARN_ON(ret))
> > > +			return ret;
> > > +
> > > +		return 0;
> > > +	}
> > > +
> > > +	intel_fill_fb_ggtt_view(&plane_state->view, &fb->base, rotation);
> > > +
> > > +	for (i = 0; i < num_planes; i++) {
> > > +		plane_state->color_plane[i].stride = intel_fb_pitch(&fb->base, i, rotation);
> > > +		plane_state->color_plane[i].offset = 0;
> > > +
> > > +		if (drm_rotation_90_or_270(rotation)) {
> > > +			plane_state->color_plane[i].x = fb->rotated[i].x;
> > > +			plane_state->color_plane[i].y = fb->rotated[i].y;
> > > +		} else {
> > > +			plane_state->color_plane[i].x = fb->normal[i].x;
> > > +			plane_state->color_plane[i].y = fb->normal[i].y;
> > > +		}
> > > +	}
> > > +
> > > +	/* Rotate src coordinates to match rotated GTT view */
> > > +	if (drm_rotation_90_or_270(rotation))
> > > +		drm_rect_rotate(&plane_state->base.src,
> > > +				fb->base.width << 16, fb->base.height << 16,
> > > +				DRM_MODE_ROTATE_270);
> > > +
> > > +	ret = intel_plane_check_stride(plane_state);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return 0;
> > > +}
> > 
> > Splitting the refactoring from the actual feature adding would be nice.
> 
> Aye. This patch did end up accumulating a bit too many changes.
> 
> > 
> > > +
> > >  static int i9xx_format_to_fourcc(int format)
> > >  {
> > >  	switch (format) {
> > > @@ -3127,26 +3395,15 @@ static int skl_check_ccs_aux_surface(struct intel_plane_state *plane_state)
> > >  int skl_check_plane_surface(struct intel_plane_state *plane_state)
> > >  {
> > >  	const struct drm_framebuffer *fb = plane_state->base.fb;
> > > -	unsigned int rotation = plane_state->base.rotation;
> > >  	int ret;
> > >  
> > > -	intel_fill_fb_ggtt_view(&plane_state->view, fb, rotation);
> > > -	plane_state->color_plane[0].stride = intel_fb_pitch(fb, 0, rotation);
> > > -	plane_state->color_plane[1].stride = intel_fb_pitch(fb, 1, rotation);
> > > -
> > > -	ret = intel_plane_check_stride(plane_state);
> > > +	ret = intel_plane_compute_gtt(plane_state);
> > >  	if (ret)
> > >  		return ret;
> > >  
> > >  	if (!plane_state->base.visible)
> > >  		return 0;
> > >  
> > > -	/* Rotate src coordinates to match rotated GTT view */
> > > -	if (drm_rotation_90_or_270(rotation))
> > > -		drm_rect_rotate(&plane_state->base.src,
> > > -				fb->width << 16, fb->height << 16,
> > > -				DRM_MODE_ROTATE_270);
> > > -
> > >  	/*
> > >  	 * Handle the AUX surface first since
> > >  	 * the main surface setup depends on it.
> > > @@ -3265,20 +3522,20 @@ int i9xx_check_plane_surface(struct intel_plane_state *plane_state)
> > >  {
> > >  	struct drm_i915_private *dev_priv =
> > >  		to_i915(plane_state->base.plane->dev);
> > > -	const struct drm_framebuffer *fb = plane_state->base.fb;
> > > -	unsigned int rotation = plane_state->base.rotation;
> > > -	int src_x = plane_state->base.src.x1 >> 16;
> > > -	int src_y = plane_state->base.src.y1 >> 16;
> > > +	int src_x, src_y;
> > >  	u32 offset;
> > >  	int ret;
> > >  
> > > -	intel_fill_fb_ggtt_view(&plane_state->view, fb, rotation);
> > > -	plane_state->color_plane[0].stride = intel_fb_pitch(fb, 0, rotation);
> > > -
> > > -	ret = intel_plane_check_stride(plane_state);
> > > +	ret = intel_plane_compute_gtt(plane_state);
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	if (!plane_state->base.visible)
> > > +		return 0;
> > > +
> > > +	src_x = plane_state->base.src.x1 >> 16;
> > > +	src_y = plane_state->base.src.y1 >> 16;
> > > +
> > >  	intel_add_fb_offsets(&src_x, &src_y, plane_state, 0);
> > >  
> > >  	if (INTEL_GEN(dev_priv) >= 4)
> > > @@ -3289,6 +3546,7 @@ int i9xx_check_plane_surface(struct intel_plane_state *plane_state)
> > >  
> > >  	/* HSW/BDW do this automagically in hardware */
> > >  	if (!IS_HASWELL(dev_priv) && !IS_BROADWELL(dev_priv)) {
> > > +		unsigned int rotation = plane_state->base.rotation;
> > >  		int src_w = drm_rect_width(&plane_state->base.src) >> 16;
> > >  		int src_h = drm_rect_height(&plane_state->base.src) >> 16;
> > >  
> > > @@ -3325,6 +3583,10 @@ i9xx_plane_check(struct intel_crtc_state *crtc_state,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	ret = i9xx_check_plane_surface(plane_state);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	if (!plane_state->base.visible)
> > >  		return 0;
> > >  
> > > @@ -3332,10 +3594,6 @@ i9xx_plane_check(struct intel_crtc_state *crtc_state,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	ret = i9xx_check_plane_surface(plane_state);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > >  	plane_state->ctl = i9xx_plane_ctl(crtc_state, plane_state);
> > >  
> > >  	return 0;
> > > @@ -3459,15 +3717,6 @@ static bool i9xx_plane_get_hw_state(struct intel_plane *plane,
> > >  	return ret;
> > >  }
> > >  
> > > -static u32
> > > -intel_fb_stride_alignment(const struct drm_framebuffer *fb, int color_plane)
> > > -{
> > > -	if (fb->modifier == DRM_FORMAT_MOD_LINEAR)
> > > -		return 64;
> > > -	else
> > > -		return intel_tile_width_bytes(fb, color_plane);
> > > -}
> > > -
> > >  static void skl_detach_scaler(struct intel_crtc *intel_crtc, int id)
> > >  {
> > >  	struct drm_device *dev = intel_crtc->base.dev;
> > > @@ -9830,19 +10079,17 @@ static bool intel_cursor_size_ok(const struct intel_plane_state *plane_state)
> > >  
> > >  static int intel_cursor_check_surface(struct intel_plane_state *plane_state)
> > >  {
> > > -	const struct drm_framebuffer *fb = plane_state->base.fb;
> > > -	unsigned int rotation = plane_state->base.rotation;
> > >  	int src_x, src_y;
> > >  	u32 offset;
> > >  	int ret;
> > >  
> > > -	intel_fill_fb_ggtt_view(&plane_state->view, fb, rotation);
> > > -	plane_state->color_plane[0].stride = intel_fb_pitch(fb, 0, rotation);
> > > -
> > > -	ret = intel_plane_check_stride(plane_state);
> > > +	ret = intel_plane_compute_gtt(plane_state);
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	if (!plane_state->base.visible)
> > > +		return 0;
> > > +
> > >  	src_x = plane_state->base.src_x >> 16;
> > >  	src_y = plane_state->base.src_y >> 16;
> > >  
> > > @@ -9879,6 +10126,10 @@ static int intel_check_cursor(struct intel_crtc_state *crtc_state,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	ret = intel_cursor_check_surface(plane_state);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	if (!plane_state->base.visible)
> > >  		return 0;
> > >  
> > > @@ -9886,10 +10137,6 @@ static int intel_check_cursor(struct intel_crtc_state *crtc_state,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	ret = intel_cursor_check_surface(plane_state);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > >  	return 0;
> > >  }
> > >  
> > > @@ -14591,31 +14838,13 @@ static const struct drm_framebuffer_funcs intel_fb_funcs = {
> > >  	.dirty = intel_user_framebuffer_dirty,
> > >  };
> > >  
> > > -static
> > > -u32 intel_fb_pitch_limit(struct drm_i915_private *dev_priv,
> > > -			 u32 pixel_format, u64 fb_modifier)
> > > -{
> > > -	struct intel_crtc *crtc;
> > > -	struct intel_plane *plane;
> > > -
> > > -	/*
> > > -	 * We assume the primary plane for pipe A has
> > > -	 * the highest stride limits of them all.
> > > -	 */
> > > -	crtc = intel_get_crtc_for_pipe(dev_priv, PIPE_A);
> > > -	plane = to_intel_plane(crtc->base.primary);
> > > -
> > > -	return plane->max_stride(plane, pixel_format, fb_modifier,
> > > -				 DRM_MODE_ROTATE_0);
> > > -}
> > > -
> > >  static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
> > >  				  struct drm_i915_gem_object *obj,
> > >  				  struct drm_mode_fb_cmd2 *mode_cmd)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> > >  	struct drm_framebuffer *fb = &intel_fb->base;
> > > -	u32 pitch_limit;
> > > +	u32 max_stride;
> > >  	unsigned int tiling, stride;
> > >  	int ret = -EINVAL;
> > >  	int i;
> > > @@ -14667,13 +14896,13 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
> > >  		goto err;
> > >  	}
> > >  
> > > -	pitch_limit = intel_fb_pitch_limit(dev_priv, mode_cmd->pixel_format,
> > > -					   mode_cmd->modifier[0]);
> > > -	if (mode_cmd->pitches[0] > pitch_limit) {
> > > +	max_stride = intel_fb_max_stride(dev_priv, mode_cmd->pixel_format,
> > > +					 mode_cmd->modifier[0]);
> > > +	if (mode_cmd->pitches[0] > max_stride) {
> > >  		DRM_DEBUG_KMS("%s pitch (%u) must be at most %d\n",
> > >  			      mode_cmd->modifier[0] != DRM_FORMAT_MOD_LINEAR ?
> > >  			      "tiled" : "linear",
> > > -			      mode_cmd->pitches[0], pitch_limit);
> > > +			      mode_cmd->pitches[0], max_stride);
> > >  		goto err;
> > >  	}
> > >  
> > 
> > We need an intel_framebuffer|plane.c. And a metric pile of other extracted
> > files, probably also per major platforms and stuff like that :-/
> 
> Yeah, I've been thinking about skl_universal_plane.c + i9xx_plane.c +
> maybe intel_plane.c for generic stuff for a while now. Also maybe
> intel_cursor.c. But I've been putting it off to avoid rebase pain.
> But I think we're probably in a good enough state now that it should
> be fine to take the plunge.
> 
> > 
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 813626322fa3..0ee73f9dea7c 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1586,6 +1586,7 @@ void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state,
> > >  			    const char *context);
> > >  
> > >  /* intel_display.c */
> > > +bool intel_plane_can_remap(const struct intel_plane_state *plane_state);
> > >  void i830_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe);
> > >  void i830_disable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe);
> > >  enum pipe intel_crtc_pch_transcoder(struct intel_crtc *crtc);
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > index b02d3d9809e3..517747d08962 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -237,6 +237,16 @@ int intel_plane_check_stride(const struct intel_plane_state *plane_state)
> > >  	unsigned int rotation = plane_state->base.rotation;
> > >  	u32 stride, max_stride;
> > >  
> > > +	/*
> > > +	 * We ignore stride for all invisible planes that
> > > +	 * can be remapped. Otherwise we could end up
> > > +	 * with a false positive when the remapping didn't
> > > +	 * kick in due the plane being invisible.
> > > +	 */
> > > +	if (intel_plane_can_remap(plane_state) &&
> > > +	    !plane_state->base.visible)
> > > +		return 0;
> > > +
> > >  	/* FIXME other color planes? */
> > >  	stride = plane_state->color_plane[0].stride;
> > >  	max_stride = plane->max_stride(plane, fb->format->format,
> > > @@ -1341,6 +1351,10 @@ g4x_sprite_check(struct intel_crtc_state *crtc_state,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	ret = i9xx_check_plane_surface(plane_state);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	if (!plane_state->base.visible)
> > >  		return 0;
> > >  
> > > @@ -1352,10 +1366,6 @@ g4x_sprite_check(struct intel_crtc_state *crtc_state,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	ret = i9xx_check_plane_surface(plane_state);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > >  	if (INTEL_GEN(dev_priv) >= 7)
> > >  		plane_state->ctl = ivb_sprite_ctl(crtc_state, plane_state);
> > >  	else
> > > @@ -1399,6 +1409,10 @@ vlv_sprite_check(struct intel_crtc_state *crtc_state,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	ret = i9xx_check_plane_surface(plane_state);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	if (!plane_state->base.visible)
> > >  		return 0;
> > >  
> > > @@ -1406,10 +1420,6 @@ vlv_sprite_check(struct intel_crtc_state *crtc_state,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	ret = i9xx_check_plane_surface(plane_state);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > >  	plane_state->ctl = vlv_sprite_ctl(crtc_state, plane_state);
> > >  
> > >  	return 0;
> > > @@ -1556,6 +1566,10 @@ static int skl_plane_check(struct intel_crtc_state *crtc_state,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	ret = skl_check_plane_surface(plane_state);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	if (!plane_state->base.visible)
> > >  		return 0;
> > >  
> > > @@ -1571,10 +1585,6 @@ static int skl_plane_check(struct intel_crtc_state *crtc_state,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	ret = skl_check_plane_surface(plane_state);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > >  	/* HW only has 8 bits pixel precision, disable plane if invisible */
> > >  	if (!(plane_state->base.alpha >> 8))
> > >  		plane_state->base.visible = false;
> > 
> > Code looks good, but the testing freaks me out. Needs lots of igt, I'd say
> > at least similar amounts to what we've all added for the original
> > kms_rotation tests.
> > -Daniel
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel

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