> -----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
