[Intel-gfx] [PATCH v3 6/8] drm/i915: Overcome display engine stride limits via GTT remapping

[Ville Syrjälä]

On Tue, Oct 23, 2018 at 08:16:56PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjala (2018-09-25 20:37:12)
> > +static bool intel_plane_needs_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;
> > +       unsigned int rotation = plane_state->base.rotation;
> > +       u32 stride, max_stride;
> > +
> > +       /* We don't want to deal with remapping with cursors */
> > +       if (plane->id == PLANE_CURSOR)
> > +               return false;
> > +
> > +       /* No fence for the remapped vma */
> 
> Should work now (with a minor tweak to plane_can_fence), right?

Gen2 would probably need a bit of extra considerationa due to
the tile size vs. page size thing. I guess gen3 might work as is.

But as long as we're limited by the current max fb size I
suppose there isn't much point in remapping.

> 
> > +       if (INTEL_GEN(dev_priv) < 4)
> > +               return false;
> > +
> > +       /* New CCS hash mode makes remapping impossible */
> > +       if (is_ccs_modifier(fb->modifier))
> > +               return false;
> > +
> > +       /* FIXME other color planes? */
> > +       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)
> > @@ -2676,6 +2741,182 @@ 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 tile_width, tile_height;
> > +       unsigned int aligned_x, aligned_y;
> > +       unsigned int aligned_w, aligned_h;
> > +       unsigned int src_x, src_y;
> > +       unsigned int src_w, src_h;
> > +       unsigned int x, y;
> > +       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));
> > +
> > +       /* Align our viewport start to tile boundary */
> > +       intel_tile_dims(fb, 0, &tile_width, &tile_height);
> > +
> > +       x = src_x & (tile_width - 1);
> > +       y = src_y & (tile_height - 1);
> > +
> > +       aligned_x = src_x - x;
> > +       aligned_y = src_y - y;
> > +
> > +       aligned_w = x + src_w;
> > +       aligned_h = y + src_h;
> > +
> > +       /* Make src coordinates relative to the aligned viewport */
> > +       drm_rect_translate(&plane_state->base.src,
> > +                          -(aligned_x << 16), -(aligned_y << 16));
> > +
> > +       /* Rotate src coordinates to match rotated GTT view */
> > +       if (drm_rotation_90_or_270(rotation))
> > +               drm_rect_rotate(&plane_state->base.src,
> > +                               aligned_w << 16, aligned_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 width, height;
> > +               unsigned int pitch_tiles;
> > +               unsigned int x, y;
> > +               u32 offset;
> > +
> > +               intel_tile_dims(fb, i, &tile_width, &tile_height);
> > +
> > +               x = aligned_x / hsub;
> > +               y = aligned_y / vsub;
> > +               width = aligned_w / hsub;
> 
> aligned_w here seems to be just x2.
> 
> Did I miss aligned_w = ALIGN(aligned_w, tile_width) - aligned_x?

No. I guess it should be called width_from_aligned_x or something along
those lines.

> 
> > +               height = aligned_h / vsub;
> > +
> > +               /*
> > +                * First pixel of the aligned 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);
> 
> I thought .width here would be in pages? Ok tile_width != fence_stride
> and does produce pages just fine.
> 
> And shouldn't width here be adjusted for x? Similarly for height?

Hmm. The surf register will point to offset (well, the equivalent place
in the new vma), and the plane starts to scan out x pixels from that,
so the vma must be at least x+width pixels wide. Still seems to make
sense to me at least :)

Actually thinking about this some more, I should be able to just throw
out the aligned_x/y stuff. intel_compute_aligned_offset() will give us
what we need anyway. I originally added that extra alignement step for
CCS (to align to a whole CCS tile), but since remapping with CCS
doesn't work anyway there's no point in doing this extra work.

-- 
Ville Syrjälä
Intel