On Thu, Jan 28, 2016 at 08:51:14PM +0200, Ville Syrjälä wrote:
> On Mon, Jan 25, 2016 at 06:30:17PM +0100, Daniel Vetter wrote:
> > On Wed, Jan 20, 2016 at 09:05:29PM +0200, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > The page aligned surface address calculation needs to know which way
> > > things are rotated. The contract now says that the caller must pass the
> > > rotate x/y coordinates, as well as the tile_height aligned stride in
> > > the tile_height direction. This will make it fairly simple to deal with
> > > 90/270 degree rotation on SKL+ where we have to deal with the rotated
> > > view into the GTT.
> > >
> > > v2: Pass rotation instead of bool even thoughwe only care about 0/180 vs. 90/270
> > > v3: Introduce intel_tile_dims(), and don't mix up different units so much
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx> (v2)
> > > ---
> > > drivers/gpu/drm/i915/intel_display.c | 66 +++++++++++++++++++++++++-----------
> > > drivers/gpu/drm/i915/intel_drv.h | 3 +-
> > > drivers/gpu/drm/i915/intel_sprite.c | 18 +++++-----
> > > 3 files changed, 59 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index cfd52ea68e34..bda3224021b2 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2269,6 +2269,18 @@ unsigned int intel_tile_height(const struct drm_i915_private *dev_priv,
> > > intel_tile_width(dev_priv, fb_modifier, cpp);
> > > }
> > >
> > > +/* Return the tile dimensions in pixel units */
> > > +static void intel_tile_dims(const struct drm_i915_private *dev_priv,
> > > + unsigned int *tile_width,
> > > + unsigned int *tile_height,
> > > + uint64_t fb_modifier,
> > > + unsigned int cpp)
> > > +{
> > > + *tile_width = intel_tile_width(dev_priv, fb_modifier, cpp);
> > > + *tile_height = intel_tile_size(dev_priv) / *tile_width;
> > > + *tile_width /= cpp;
> >
> > Seems like my suggestion to use tile_width in cpp wasn't that awesome, at
> > least we still have a big confusion right here. Not sure what to do,
> > especially since these kind of changes are super-painful to review. Could
> > we perhaps go back to the old versions and untangle this mess afterwards?
> > Iirc I've follow-up with r-b tags to all patches in the old series for
> > that approach.
>
> Hmm. What did I do differently in the old approach again? Other than not
> having the confusion isolated to this one function. One step forward and
> two back doens't sound productive. Plus I have some more patches on top
> of this one already, so not pleasant at all.
Old patch reused the same variables for values both in bytes and pixels
(and the conversion happened deep down in some function call, without any
comments). This here looks better, but still suffers a bit from confusing
naming.
>
> Anyway, to avoid more confusion I suppose one thing that could be done
> is this:
> - intel_tile_width
> + intel_tile_width_bytes
>
> intel_tile_dims()
> {
> tile_width_bytes = intel_tile_width_bytes()
> *tile_height = tile_size / tile_width_bytes;
> *tile_width = tile_width_bytes / cpp;
> }
Yeah that sounds reasonable I think.
-Daniel
>
> >
> > No idea really what would be best here ...
> > -Daniel
> >
> > > +}
> > > +
> > > unsigned int
> > > intel_fb_align_height(struct drm_device *dev, unsigned int height,
> > > uint32_t pixel_format, uint64_t fb_modifier)
> > > @@ -2306,19 +2318,19 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
> > > tile_size = intel_tile_size(dev_priv);
> > >
> > > cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> > > - tile_width = intel_tile_width(dev_priv, fb->modifier[0], cpp);
> > > - tile_height = tile_size / tile_width;
> > > + intel_tile_dims(dev_priv, &tile_width, &tile_height,
> > > + fb->modifier[0], cpp);
> > >
> > > - info->width_pages = DIV_ROUND_UP(fb->pitches[0], tile_width);
> > > + info->width_pages = DIV_ROUND_UP(fb->pitches[0], tile_width * cpp);
> > > info->height_pages = DIV_ROUND_UP(fb->height, tile_height);
> > > info->size = info->width_pages * info->height_pages * tile_size;
> > >
> > > if (info->pixel_format == DRM_FORMAT_NV12) {
> > > cpp = drm_format_plane_cpp(fb->pixel_format, 1);
> > > - tile_width = intel_tile_width(dev_priv, fb->modifier[1], cpp);
> > > - tile_height = tile_size / tile_width;
> > > + intel_tile_dims(dev_priv, &tile_width, &tile_height,
> > > + fb->modifier[1], cpp);
> > >
> > > - info->width_pages_uv = DIV_ROUND_UP(fb->pitches[1], tile_width);
> > > + info->width_pages_uv = DIV_ROUND_UP(fb->pitches[1], tile_width * cpp);
> > > info->height_pages_uv = DIV_ROUND_UP(fb->height / 2, tile_height);
> > > info->size_uv = info->width_pages_uv * info->height_pages_uv * tile_size;
> > > }
> > > @@ -2446,29 +2458,43 @@ static void intel_unpin_fb_obj(struct drm_framebuffer *fb,
> > > i915_gem_object_unpin_from_display_plane(obj, &view);
> > > }
> > >
> > > -/* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel
> > > - * is assumed to be a power-of-two. */
> > > +/*
> > > + * Computes the linear offset to the base tile and adjusts
> > > + * x, y. bytes per pixel is assumed to be a power-of-two.
> > > + *
> > > + * In the 90/270 rotated case, x and y are assumed
> > > + * to be already rotated to match the rotated GTT view, and
> > > + * pitch is the tile_height aligned framebuffer height.
> > > + */
> > > u32 intel_compute_tile_offset(struct drm_i915_private *dev_priv,
> > > int *x, int *y,
> > > uint64_t fb_modifier,
> > > unsigned int cpp,
> > > - unsigned int pitch)
> > > + unsigned int pitch,
> > > + unsigned int rotation)
> > > {
> > > if (fb_modifier != DRM_FORMAT_MOD_NONE) {
> > > unsigned int tile_size, tile_width, tile_height;
> > > - unsigned int tile_rows, tiles;
> > > + unsigned int tile_rows, tiles, pitch_tiles;
> > >
> > > tile_size = intel_tile_size(dev_priv);
> > > - tile_width = intel_tile_width(dev_priv, fb_modifier, cpp);
> > > - tile_height = tile_size / tile_width;
> > > + intel_tile_dims(dev_priv, &tile_width, &tile_height,
> > > + fb_modifier, cpp);
> > > +
> > > + if (intel_rotation_90_or_270(rotation)) {
> > > + pitch_tiles = pitch / tile_height;
> > > + swap(tile_width, tile_height);
> > > + } else {
> > > + pitch_tiles = pitch / (tile_width * cpp);
> > > + }
> > >
> > > tile_rows = *y / tile_height;
> > > *y %= tile_height;
> > >
> > > - tiles = *x / (tile_width/cpp);
> > > - *x %= tile_width/cpp;
> > > + tiles = *x / tile_width;
> > > + *x %= tile_width;
> > >
> > > - return tile_rows * pitch * tile_height + tiles * tile_size;
> > > + return (tile_rows * pitch_tiles + tiles) * tile_size;
> > > } else {
> > > unsigned int alignment = intel_linear_alignment(dev_priv) - 1;
> > > unsigned int offset;
> > > @@ -2709,6 +2735,7 @@ static void i9xx_update_primary_plane(struct drm_plane *primary,
> > > u32 linear_offset;
> > > u32 dspcntr;
> > > i915_reg_t reg = DSPCNTR(plane);
> > > + unsigned int rotation = plane_state->base.rotation;
> > > int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> > > int x = plane_state->src.x1 >> 16;
> > > int y = plane_state->src.y1 >> 16;
> > > @@ -2775,13 +2802,13 @@ static void i9xx_update_primary_plane(struct drm_plane *primary,
> > > intel_crtc->dspaddr_offset =
> > > intel_compute_tile_offset(dev_priv, &x, &y,
> > > fb->modifier[0], cpp,
> > > - fb->pitches[0]);
> > > + fb->pitches[0], rotation);
> > > linear_offset -= intel_crtc->dspaddr_offset;
> > > } else {
> > > intel_crtc->dspaddr_offset = linear_offset;
> > > }
> > >
> > > - if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
> > > + if (rotation == BIT(DRM_ROTATE_180)) {
> > > dspcntr |= DISPPLANE_ROTATE_180;
> > >
> > > x += (crtc_state->pipe_src_w - 1);
> > > @@ -2839,6 +2866,7 @@ static void ironlake_update_primary_plane(struct drm_plane *primary,
> > > u32 linear_offset;
> > > u32 dspcntr;
> > > i915_reg_t reg = DSPCNTR(plane);
> > > + unsigned int rotation = plane_state->base.rotation;
> > > int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> > > int x = plane_state->src.x1 >> 16;
> > > int y = plane_state->src.y1 >> 16;
> > > @@ -2882,9 +2910,9 @@ static void ironlake_update_primary_plane(struct drm_plane *primary,
> > > intel_crtc->dspaddr_offset =
> > > intel_compute_tile_offset(dev_priv, &x, &y,
> > > fb->modifier[0], cpp,
> > > - fb->pitches[0]);
> > > + fb->pitches[0], rotation);
> > > linear_offset -= intel_crtc->dspaddr_offset;
> > > - if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
> > > + if (rotation == BIT(DRM_ROTATE_180)) {
> > > dspcntr |= DISPPLANE_ROTATE_180;
> > >
> > > if (!IS_HASWELL(dev) && !IS_BROADWELL(dev)) {
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index f620023ed134..223693dbfe7c 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1176,7 +1176,8 @@ u32 intel_compute_tile_offset(struct drm_i915_private *dev_priv,
> > > int *x, int *y,
> > > uint64_t fb_modifier,
> > > unsigned int cpp,
> > > - unsigned int pitch);
> > > + unsigned int pitch,
> > > + unsigned int rotation);
> > > void intel_prepare_reset(struct drm_device *dev);
> > > void intel_finish_reset(struct drm_device *dev);
> > > void hsw_enable_pc8(struct drm_i915_private *dev_priv);
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > index a2582c455b36..7dc2b8b2a4ac 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -193,7 +193,7 @@ skl_update_plane(struct drm_plane *drm_plane,
> > > const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> > > u32 surf_addr;
> > > u32 tile_height, plane_offset, plane_size;
> > > - unsigned int rotation;
> > > + unsigned int rotation = plane_state->base.rotation;
> > > int x_offset, y_offset;
> > > int crtc_x = plane_state->dst.x1;
> > > int crtc_y = plane_state->dst.y1;
> > > @@ -213,7 +213,6 @@ skl_update_plane(struct drm_plane *drm_plane,
> > > plane_ctl |= skl_plane_ctl_format(fb->pixel_format);
> > > plane_ctl |= skl_plane_ctl_tiling(fb->modifier[0]);
> > >
> > > - rotation = plane_state->base.rotation;
> > > plane_ctl |= skl_plane_ctl_rotation(rotation);
> > >
> > > stride_div = intel_fb_stride_alignment(dev_priv, fb->modifier[0],
> > > @@ -351,6 +350,7 @@ vlv_update_plane(struct drm_plane *dplane,
> > > int plane = intel_plane->plane;
> > > u32 sprctl;
> > > u32 sprsurf_offset, linear_offset;
> > > + unsigned int rotation = dplane->state->rotation;
> > > int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> > > const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> > > int crtc_x = plane_state->dst.x1;
> > > @@ -425,10 +425,10 @@ vlv_update_plane(struct drm_plane *dplane,
> > > linear_offset = y * fb->pitches[0] + x * cpp;
> > > sprsurf_offset = intel_compute_tile_offset(dev_priv, &x, &y,
> > > fb->modifier[0], cpp,
> > > - fb->pitches[0]);
> > > + fb->pitches[0], rotation);
> > > linear_offset -= sprsurf_offset;
> > >
> > > - if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
> > > + if (rotation == BIT(DRM_ROTATE_180)) {
> > > sprctl |= SP_ROTATE_180;
> > >
> > > x += src_w;
> > > @@ -493,6 +493,7 @@ ivb_update_plane(struct drm_plane *plane,
> > > enum pipe pipe = intel_plane->pipe;
> > > u32 sprctl, sprscale = 0;
> > > u32 sprsurf_offset, linear_offset;
> > > + unsigned int rotation = plane_state->base.rotation;
> > > int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> > > const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> > > int crtc_x = plane_state->dst.x1;
> > > @@ -558,10 +559,10 @@ ivb_update_plane(struct drm_plane *plane,
> > > linear_offset = y * fb->pitches[0] + x * cpp;
> > > sprsurf_offset = intel_compute_tile_offset(dev_priv, &x, &y,
> > > fb->modifier[0], cpp,
> > > - fb->pitches[0]);
> > > + fb->pitches[0], rotation);
> > > linear_offset -= sprsurf_offset;
> > >
> > > - if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
> > > + if (rotation == BIT(DRM_ROTATE_180)) {
> > > sprctl |= SPRITE_ROTATE_180;
> > >
> > > /* HSW and BDW does this automagically in hardware */
> > > @@ -634,6 +635,7 @@ ilk_update_plane(struct drm_plane *plane,
> > > int pipe = intel_plane->pipe;
> > > u32 dvscntr, dvsscale;
> > > u32 dvssurf_offset, linear_offset;
> > > + unsigned int rotation = plane_state->base.rotation;
> > > int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> > > const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> > > int crtc_x = plane_state->dst.x1;
> > > @@ -695,10 +697,10 @@ ilk_update_plane(struct drm_plane *plane,
> > > linear_offset = y * fb->pitches[0] + x * cpp;
> > > dvssurf_offset = intel_compute_tile_offset(dev_priv, &x, &y,
> > > fb->modifier[0], cpp,
> > > - fb->pitches[0]);
> > > + fb->pitches[0], rotation);
> > > linear_offset -= dvssurf_offset;
> > >
> > > - if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
> > > + if (rotation == BIT(DRM_ROTATE_180)) {
> > > dvscntr |= DVS_ROTATE_180;
> > >
> > > x += src_w;
> > > --
> > > 2.4.10
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> Ville Syrjälä
> Intel OTC
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
