Re: [PATCH 2/2] drm/i915: Render decompression support for Gen9 and above

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

 



> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx]
> Sent: Friday, March 18, 2016 10:42 PM
> To: Kannan, Vandana <vandana.kannan@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re:  [PATCH 2/2] drm/i915: Render decompression
> support for Gen9 and above

[...]
 
> > @@ -3573,7 +3576,7 @@ static void
> skylake_update_primary_plane(struct drm_plane *plane,
> >  	struct drm_framebuffer *fb = plane_state->base.fb;
> >  	int pipe = intel_crtc->pipe;
> >  	u32 plane_ctl;
> > -	unsigned int rotation = plane_state->base.rotation;
> > +	unsigned int rotation = plane_state->base.rotation, render_comp;
> >  	u32 stride = skl_plane_stride(fb, 0, rotation);
> >  	u32 surf_addr = plane_state->main.offset;
> >  	int scaler_id = plane_state->scaler_id; @@ -3585,6 +3588,9 @@
> static
> > void skylake_update_primary_plane(struct drm_plane *plane,
> >  	int dst_y = plane_state->dst.y1;
> >  	int dst_w = drm_rect_width(&plane_state->dst);
> >  	int dst_h = drm_rect_height(&plane_state->dst);
> > +	unsigned long aux_dist = 0;
> > +	u32 aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
> > +	u32 tile_row_adjustment = 0;
> >
> >  	plane_ctl = PLANE_CTL_ENABLE |
> >  		    PLANE_CTL_PIPE_GAMMA_ENABLE |
> > @@ -3604,10 +3610,43 @@ static void
> skylake_update_primary_plane(struct drm_plane *plane,
> >  	intel_crtc->adjusted_x = src_x;
> >  	intel_crtc->adjusted_y = src_y;
> >
> > +	render_comp = plane_state->render_comp_enable;
> > +	if (render_comp) {
> > +		u32 tile_height = PAGE_SIZE /
> > +			intel_fb_stride_alignment(dev_priv, fb->modifier[0],
> > +					fb->pixel_format);
> > +
> > +		u32 height_in_mem = (fb->offsets[1]/fb->pitches[0]);
> > +		tile_row_adjustment = height_in_mem % tile_height;
> > +		aux_dist = (fb->pitches[0] *
> > +				(height_in_mem - tile_row_adjustment));
> > +		aux_stride = skl_plane_stride(fb, 1, rotation);
> 
> Could you go read my fb offsets series [1] and see if there's something
> there that doesn't agree with render compression?
> 
> [1] https://lists.freedesktop.org/archives/intel-gfx/2016-
> February/087660.html
> 
[Vandana] 
I went through that..
intel_fb_pitch() return the fb->pitches[plane]. So for aux plane in the case of RC and UV plane in the case of NV12,
plane value should be 1.
I dint see any other dependency on first read.

> > +		plane_ctl |= PLANE_CTL_DECOMPRESSION_ENABLE;
> > +	} else {
> > +		plane_ctl &= ~PLANE_CTL_DECOMPRESSION_ENABLE;
> > +	}
> > +
> >  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
> >  	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
> >  	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> >  	I915_WRITE(PLANE_SIZE(pipe, 0), (src_h << 16) | src_w);
> > +	I915_WRITE(PLANE_AUX_DIST(pipe, 0), aux_dist | aux_stride);
> > +	I915_WRITE(PLANE_AUX_OFFSET(pipe, 0), aux_y_offset<<16 |
> > +aux_x_offset);
> > +
> > +	/*
> > +	 * Per bspec, for SKL C and BXT A steppings, when the plane source
> pixel
> > +	 * format is NV12, the CHICKEN_PIPESL register bit 22 must be set
> to 1.
> > +	 * When render compression is enabled, bit 22 must be set to 0.
> > +	 */
> > +	if (IS_SKL_REVID(dev, 0, SKL_REVID_C0) ||
> > +			IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
> 
> Does anyone really care about these anymore?
> 
[Vandana] It was required internally.. If all users will have the latest stepping, then this can be removed.

> Also you really should fix your editor to wrap lines correctly.
> 
> > +		u32 temp = I915_READ(CHICKEN_PIPESL_1(pipe));
> > +		if (render_comp) {
> > +			if ((temp & HSW_FBCQ_DIS) == HSW_FBCQ_DIS)
> > +				I915_WRITE(CHICKEN_PIPESL_1(pipe),
> > +						temp & ~HSW_FBCQ_DIS);
> > +		}
> > +	}
> >
> >  	if (scaler_id >= 0) {
> >  		uint32_t ps_ctrl = 0;
> > @@ -12333,6 +12372,17 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
> >  			to_intel_plane_state(plane_state));
> >  		if (ret)
> >  			return ret;
> > +
> > +		if (fb && to_intel_plane_state(plane_state)->
> > +				render_comp_enable) {
> > +			ret = skl_check_compression(dev,
> > +					to_intel_plane_state(plane_state),
> > +					intel_crtc->pipe, crtc->x, crtc->y);
> > +			if (ret) {
> > +				DRM_DEBUG_KMS("Render compr checks
> failed\n");
> > +				return ret;
> > +			}
> > +		}
> >  	}
> >
> >  	was_visible = old_plane_state->visible; @@ -12388,7 +12438,6 @@
> int
> > intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> >  	case DRM_PLANE_TYPE_PRIMARY:
> >  		intel_crtc->atomic.post_enable_primary = turn_on;
> >  		intel_crtc->atomic.update_fbc = true;
> > -
> >  		break;
> >  	case DRM_PLANE_TYPE_CURSOR:
> >  		break;
> > @@ -12516,6 +12565,41 @@ static int intel_crtc_atomic_check(struct
> drm_crtc *crtc,
> >  		}
> >  	}
> >
> > +	/*
> > +	 * On SKL:*:C and BXT:*:A, there is a possible hang with NV12
> format.
> > +	 * WA: render decompression must not be enabled on any of the
> planes in
> > +	 * that pipe.
> > +	 */
> > +	if (IS_SKL_REVID(dev, 0, SKL_REVID_C0) ||
> > +			IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
> 
> More extinct animals
> 
> > +		struct drm_plane *p;
> > +		bool rc_enabled = false, nv12_enabled = false;
> > +
> > +		drm_for_each_plane_mask(p, dev, crtc_state->plane_mask) {
> > +			struct drm_plane_state *plane_state =
> > +				drm_atomic_get_plane_state(state, p);
> > +
> > +			if (plane_state) {
> > +				if (to_intel_plane_state(plane_state)->
> > +						render_comp_enable)
> > +					rc_enabled = true;
> > +
> > +				if (plane_state->fb &&
> > +						plane_state->fb->pixel_format
> ==
> > +						DRM_FORMAT_NV12)
> > +					nv12_enabled = true;
> > +			}
> > +
> > +		}
> > +		if (rc_enabled && nv12_enabled) {
> > +			DRM_DEBUG_KMS("RC should not be enabled "
> > +					"on any plane of the "
> > +					"pipe on which NV12 is "
> > +					"enabled\n");
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> >  	if (INTEL_INFO(dev)->gen >= 9) {
> >  		if (mode_changed)
> >  			ret = skl_update_scaler_crtc(pipe_config);
> > @@ -14517,6 +14601,91 @@ skl_max_scale(struct intel_crtc *intel_crtc,
> struct intel_crtc_state *crtc_state
> >  	return max_scale;
> >  }
> >
> > +static int skl_check_compression(struct drm_device *dev,
> > +		struct intel_plane_state *plane_state,
> > +		enum pipe pipe, int x, int y)
> > +{
> > +	struct drm_framebuffer *fb = plane_state->base.fb;
> > +	struct drm_plane *plane = plane_state->base.plane;
> > +	int x_offset;
> > +	int src_w = drm_rect_width(&plane_state->src) >> 16;
> > +
> > +	if (!IS_SKYLAKE(dev) && !IS_BROXTON(dev)) {
> > +		DRM_DEBUG_KMS("RC support on CNL+ needs to be
> revisited\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * TODO:
> > +	 * 1. Disable stereo 3D when render decomp is enabled (bit 7:6)
> > +	 * 2. Render decompression must not be used in VTd pass-through
> mode
> > +	 * 3. Program hashing select CHICKEN_MISC1 bit 15
> > +	 */
> > +
> > +	/*
> > +	 * On SKL A and SKL B,
> > +	 * Do not enable render decompression when the plane
> > +	 * width is smaller than 32 pixels or greater than
> > +	 * 2048 pixels
> > +	 */
> > +	if (IS_SKL_REVID(dev, 0, SKL_REVID_C0) && ((src_w < 32) || (src_w >
> > +2048))) {a
> 
> And more
> 
> > +		DRM_DEBUG_KMS("SKL-A, SKL-B RC: width > 2048 or <
> 32\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * Conditions to satisfy before enabling render decomp.
> > +	 * SKL+
> > +	 * Pipe A & B, Planes 1 & 2
> > +	 * RGB8888 Tile-Y format
> > +	 * 0/180 rotation
> > +	 */
> > +	if (pipe == PIPE_C) {
> > +		DRM_DEBUG_KMS("RC supported only on pipe A & B\n");
> > +		return -EINVAL;
> > +	}
> 
> Shouldn't have added the prop on pipe C at all then. Or the prop should
> advertise only uncompressed.
> 
[Vandana] I can make the change to set supported values based on plane/pipe and accordingly create the property.
> > +
> > +	if (!(plane->type == DRM_PLANE_TYPE_PRIMARY ||
> > +				(plane->type == DRM_PLANE_TYPE_OVERLAY
> &&
> > +				 to_intel_plane(plane)->plane == PLANE_A)))
> {
> 
> We should really have a better way to refer to specific planes. Not sure of
> anything happened on that front.
> 
> > +		DRM_DEBUG_KMS("RC supported only on planes 1 & 2\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (intel_rotation_90_or_270(plane_state->base.rotation)) {
> > +		DRM_DEBUG_KMS("RC support only with 0/180 degree
> rotation\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if ((fb->modifier[0] == DRM_FORMAT_MOD_NONE) ||
> > +			(fb->modifier[0] == I915_FORMAT_MOD_X_TILED)) {
> 
> Better check for the specific things you do support I think.
> 
> > +		DRM_DEBUG_KMS("RC supported only with Y-tile\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if ((fb->pixel_format != DRM_FORMAT_XBGR8888) &&
> > +			(fb->pixel_format != DRM_FORMAT_ABGR8888)) {
> 
> Presumably we shouldn't have allowed such an fb to be even created.
> 
> > +		DRM_DEBUG_KMS("RC supported only with RGB8888
> formats\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * For SKL & BXT,
> > +	 * When the render compression is enabled with plane
> > +	 * width greater than 3840 and horizontal panning,
> > +	 * the stride programmed in the PLANE_STRIDE register
> > +	 * must be multiple of 4.
> > +	 */
> > +	x_offset = x;
> > +
> > +	if (src_w > 3840 && x_offset != 0) {
> > +		DRM_DEBUG_KMS("RC: width > 3840, horizontal
> panning\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int
> >  intel_check_primary_plane(struct drm_plane *plane,
> >  			  struct intel_crtc_state *crtc_state, @@ -14679,6
> +14848,9 @@
> > static struct drm_plane *intel_primary_plane_create(struct drm_device
> *dev,
> >  	if (INTEL_INFO(dev)->gen >= 4)
> >  		intel_create_rotation_property(dev, primary);
> >
> > +	if (INTEL_INFO(dev)->gen >= 9)
> > +		intel_create_render_comp_property(dev, primary);
> > +
> >  	drm_plane_helper_add(&primary->base,
> &intel_plane_helper_funcs);
> >
> >  	return &primary->base;
> > @@ -14702,6 +14874,36 @@ void intel_create_rotation_property(struct
> drm_device *dev, struct intel_plane *
> >  				plane->base.state->rotation);
> >  }
> >
> > +void intel_create_render_comp_property(struct drm_device *dev,
> > +		struct intel_plane *plane)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	static const struct drm_prop_enum_list rc_status[] = {
> > +		{ COMP_UNCOMPRESSED,   "Uncompressed/not capable" },
> > +		{ COMP_RENDER,  "Only render decompression" },
> > +	};
> > +
> > +	if (!dev_priv->render_comp_property) {
> > +		dev_priv->render_comp_property =
> > +			drm_property_create_bitmask(dev, 0,
> > +					"render compression",
> > +					rc_status, ARRAY_SIZE(rc_status),
> > +					BIT(COMP_UNCOMPRESSED) |
> > +					BIT(COMP_RENDER));
> 
> Why is it a bitmask? Are you expecting the user to set multiple bits?
> 
> So far it looks more like a yes/no type of thing than an enum even. Are we
> expecting more possible values here?
> 
[Vandana] Yes, there are 2 other values that will come up in future.
"
	{ DRM_RC_RENDER_AND_CLEAR, "Render & clear decompression" },
	{ DRM_RC_PLANAR, "Planar compression capable (for future)" },
"
> Also seems like this really should have different set of supported values
> per plane.
> 
[Vandana] Ok, will make this change.
> > +		if (!dev_priv->render_comp_property) {
> > +			DRM_ERROR("RC: Failed to create property\n");
> > +			return;
> > +		}
> > +	}

[...]

> >  	/* program plane scaler */
> >  	if (plane_state->scaler_id >= 0) {
> > @@ -1092,6 +1129,9 @@ intel_plane_init(struct drm_device *dev, enum
> > pipe pipe, int plane)
> >
> >  	intel_create_rotation_property(dev, intel_plane);
> >
> > +	if (INTEL_INFO(dev)->gen >= 9)
> > +		intel_create_render_comp_property(dev, intel_plane);
> > +
> >  	drm_plane_helper_add(&intel_plane->base,
> &intel_plane_helper_funcs);
> >
> >  out:
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://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