[Intel-gfx] [PATCH 2/2] drm/i915: Render decompression support for Gen9 and above
Kannan, Vandana
vandana.kannan at intel.com
Mon Apr 25 05:00:06 UTC 2016
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> Sent: Friday, March 18, 2016 10:42 PM
> To: Kannan, Vandana <vandana.kannan at intel.com>
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
More information about the Intel-gfx
mailing list