| Message ID | 1444840154-7804-9-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 14, 2015 at 07:29:00PM +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > 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 > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 62 ++++++++++++++++++++++++++++++------ > drivers/gpu/drm/i915/intel_drv.h | 3 +- > drivers/gpu/drm/i915/intel_sprite.c | 15 +++++---- > 3 files changed, 64 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 75be66b..bd55d06 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2451,13 +2451,50 @@ 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. */ > +/* > + * Return the tile dimensions in pixel units matching > + * the specified rotation angle. > + */ > +static void intel_rotate_tile_dims(unsigned int *tile_width, > + unsigned int *tile_height, > + unsigned int *pitch, > + unsigned int cpp, > + unsigned int rotation) A rotation enum would be nice, so that we can employ sparse to check it. That'd work since sparse treats enums as bitfields, but we'd need to add names for the BIT(DRM_ROTATION_*) variants. Just an aside. > +{ > + if (intel_rotation_90_or_270(rotation)) { > + WARN_ON(*pitch % *tile_height); > + > + /* pixel units please */ > + *tile_width /= cpp; Ok, something dawns on me now about tile_width ... it's in bytes, I somehow thought it's pixels. And this function doing a behind-the-scenes conversions from bytes to pixels is a bit tricky. Should we have separate tile_pitch and tile_width to unconfuse this? Generally foo_width in the modeset code is mostly pixels ... Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> on this one (and I retract my earlier review on tile_width), but I'd like something clearer here in the end ... Cheers, Daniel > + > + /* > + * Coordinate space is rotated, orient > + * our tile dimensions the same way > + */ > + swap(*tile_width, *tile_height); > + } else { > + WARN_ON(*pitch % *tile_width); > + > + /* pixel units please */ > + *tile_width /= cpp; > + *pitch /= cpp; > + } > +} > + > +/* > + * 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. > + */ > unsigned long intel_compute_page_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; > @@ -2467,13 +2504,16 @@ unsigned long intel_compute_page_offset(struct drm_i915_private *dev_priv, > tile_width = intel_tile_width(dev_priv, fb_modifier, cpp); > tile_height = tile_size / tile_width; > > + intel_rotate_tile_dims(&tile_width, &tile_height, > + &pitch, cpp, rotation); > + > 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 / tile_width) + tiles) * tile_size; > } else { > unsigned int alignment = intel_linear_alignment(dev_priv) - 1; > unsigned int offset; > @@ -2685,6 +2725,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, > bool visible = to_intel_plane_state(primary->state)->visible; > struct drm_i915_gem_object *obj; > int plane = intel_crtc->plane; > + unsigned int rotation; > unsigned long linear_offset; > u32 dspcntr; > u32 reg = DSPCNTR(plane); > @@ -2704,6 +2745,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, > if (WARN_ON(obj == NULL)) > return; > > + rotation = crtc->primary->state->rotation; > pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); > > dspcntr = DISPPLANE_GAMMA_ENABLE; > @@ -2768,7 +2810,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, > intel_crtc->dspaddr_offset = > intel_compute_page_offset(dev_priv, &x, &y, > fb->modifier[0], pixel_size, > - fb->pitches[0]); > + fb->pitches[0], rotation); > linear_offset -= intel_crtc->dspaddr_offset; > } else { > intel_crtc->dspaddr_offset = linear_offset; > @@ -2814,6 +2856,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > bool visible = to_intel_plane_state(primary->state)->visible; > struct drm_i915_gem_object *obj; > int plane = intel_crtc->plane; > + unsigned int rotation; > unsigned long linear_offset; > u32 dspcntr; > u32 reg = DSPCNTR(plane); > @@ -2830,6 +2873,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > if (WARN_ON(obj == NULL)) > return; > > + rotation = crtc->primary->state->rotation; > pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); > > dspcntr = DISPPLANE_GAMMA_ENABLE; > @@ -2872,9 +2916,9 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > intel_crtc->dspaddr_offset = > intel_compute_page_offset(dev_priv, &x, &y, > fb->modifier[0], pixel_size, > - fb->pitches[0]); > + fb->pitches[0], rotation); > linear_offset -= intel_crtc->dspaddr_offset; > - if (crtc->primary->state->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 a12ac95..ed47ca3 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1139,7 +1139,8 @@ unsigned long intel_compute_page_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 6614adb..8eaebce 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -354,6 +354,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc, > int pipe = intel_plane->pipe; > int plane = intel_plane->plane; > u32 sprctl; > + unsigned int rotation = dplane->state->rotation; > unsigned long sprsurf_offset, linear_offset; > int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); > const struct drm_intel_sprite_colorkey *key = > @@ -422,10 +423,10 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc, > linear_offset = y * fb->pitches[0] + x * pixel_size; > sprsurf_offset = intel_compute_page_offset(dev_priv, &x, &y, > fb->modifier[0], pixel_size, > - fb->pitches[0]); > + fb->pitches[0], rotation); > linear_offset -= sprsurf_offset; > > - if (dplane->state->rotation == BIT(DRM_ROTATE_180)) { > + if (rotation == BIT(DRM_ROTATE_180)) { > sprctl |= SP_ROTATE_180; > > x += src_w; > @@ -491,6 +492,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > enum pipe pipe = intel_plane->pipe; > u32 sprctl, sprscale = 0; > + unsigned int rotation = plane->state->rotation; > unsigned long sprsurf_offset, linear_offset; > int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); > const struct drm_intel_sprite_colorkey *key = > @@ -554,10 +556,10 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > linear_offset = y * fb->pitches[0] + x * pixel_size; > sprsurf_offset = intel_compute_page_offset(dev_priv, &x, &y, > fb->modifier[0], pixel_size, > - fb->pitches[0]); > + fb->pitches[0], rotation); > linear_offset -= sprsurf_offset; > > - if (plane->state->rotation == BIT(DRM_ROTATE_180)) { > + if (rotation == BIT(DRM_ROTATE_180)) { > sprctl |= SPRITE_ROTATE_180; > > /* HSW and BDW does this automagically in hardware */ > @@ -631,6 +633,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > struct intel_plane *intel_plane = to_intel_plane(plane); > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > int pipe = intel_plane->pipe; > + unsigned int rotation = plane->state->rotation; > unsigned long dvssurf_offset, linear_offset; > u32 dvscntr, dvsscale; > int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); > @@ -691,10 +694,10 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > linear_offset = y * fb->pitches[0] + x * pixel_size; > dvssurf_offset = intel_compute_page_offset(dev_priv, &x, &y, > fb->modifier[0], pixel_size, > - fb->pitches[0]); > + fb->pitches[0], rotation); > linear_offset -= dvssurf_offset; > > - if (plane->state->rotation == BIT(DRM_ROTATE_180)) { > + if (rotation == BIT(DRM_ROTATE_180)) { > dvscntr |= DVS_ROTATE_180; > > x += src_w; > -- > 2.4.9 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Oct 21, 2015 at 12:53:34PM +0200, Daniel Vetter wrote: > On Wed, Oct 14, 2015 at 07:29:00PM +0300, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > 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 > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 62 ++++++++++++++++++++++++++++++------ > > drivers/gpu/drm/i915/intel_drv.h | 3 +- > > drivers/gpu/drm/i915/intel_sprite.c | 15 +++++---- > > 3 files changed, 64 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 75be66b..bd55d06 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -2451,13 +2451,50 @@ 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. */ > > +/* > > + * Return the tile dimensions in pixel units matching > > + * the specified rotation angle. > > + */ > > +static void intel_rotate_tile_dims(unsigned int *tile_width, > > + unsigned int *tile_height, > > + unsigned int *pitch, > > + unsigned int cpp, > > + unsigned int rotation) > > A rotation enum would be nice, so that we can employ sparse to check it. > That'd work since sparse treats enums as bitfields, but we'd need to > add names for the BIT(DRM_ROTATION_*) variants. Just an aside. I was thinking of including the '1<<x' in the define already since the current thing just invites bugs all over. I guess making it an enum at the same time should be easy. > > > +{ > > + if (intel_rotation_90_or_270(rotation)) { > > + WARN_ON(*pitch % *tile_height); > > + > > + /* pixel units please */ > > + *tile_width /= cpp; > > Ok, something dawns on me now about tile_width ... it's in bytes, I > somehow thought it's pixels. And this function doing a behind-the-scenes > conversions from bytes to pixels is a bit tricky. > > Should we have separate tile_pitch and tile_width to unconfuse this? > Generally foo_width in the modeset code is mostly pixels ... Yeah, things are a bit unclear with bytes vs. pixels sometimes. I usually think of tile width as bytes because then you don't have to consider the pixel format (well, except for Yf/Ys tiling), but here we want it in pixels instead. I'm not sure tile_pitch is a good name becuse then it might get confused with the pitch for the fb or fence. But I don't really have a better name in mind either. > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> on this one (and I > retract my earlier review on tile_width), but I'd like something clearer > here in the end ... > > Cheers, Daniel > > > + > > + /* > > + * Coordinate space is rotated, orient > > + * our tile dimensions the same way > > + */ > > + swap(*tile_width, *tile_height); > > + } else { > > + WARN_ON(*pitch % *tile_width); > > + > > + /* pixel units please */ > > + *tile_width /= cpp; > > + *pitch /= cpp; > > + } > > +} > > + > > +/* > > + * 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. > > + */ > > unsigned long intel_compute_page_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; > > @@ -2467,13 +2504,16 @@ unsigned long intel_compute_page_offset(struct drm_i915_private *dev_priv, > > tile_width = intel_tile_width(dev_priv, fb_modifier, cpp); > > tile_height = tile_size / tile_width; > > > > + intel_rotate_tile_dims(&tile_width, &tile_height, > > + &pitch, cpp, rotation); > > + > > 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 / tile_width) + tiles) * tile_size; > > } else { > > unsigned int alignment = intel_linear_alignment(dev_priv) - 1; > > unsigned int offset; > > @@ -2685,6 +2725,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, > > bool visible = to_intel_plane_state(primary->state)->visible; > > struct drm_i915_gem_object *obj; > > int plane = intel_crtc->plane; > > + unsigned int rotation; > > unsigned long linear_offset; > > u32 dspcntr; > > u32 reg = DSPCNTR(plane); > > @@ -2704,6 +2745,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, > > if (WARN_ON(obj == NULL)) > > return; > > > > + rotation = crtc->primary->state->rotation; > > pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); > > > > dspcntr = DISPPLANE_GAMMA_ENABLE; > > @@ -2768,7 +2810,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, > > intel_crtc->dspaddr_offset = > > intel_compute_page_offset(dev_priv, &x, &y, > > fb->modifier[0], pixel_size, > > - fb->pitches[0]); > > + fb->pitches[0], rotation); > > linear_offset -= intel_crtc->dspaddr_offset; > > } else { > > intel_crtc->dspaddr_offset = linear_offset; > > @@ -2814,6 +2856,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > > bool visible = to_intel_plane_state(primary->state)->visible; > > struct drm_i915_gem_object *obj; > > int plane = intel_crtc->plane; > > + unsigned int rotation; > > unsigned long linear_offset; > > u32 dspcntr; > > u32 reg = DSPCNTR(plane); > > @@ -2830,6 +2873,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > > if (WARN_ON(obj == NULL)) > > return; > > > > + rotation = crtc->primary->state->rotation; > > pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); > > > > dspcntr = DISPPLANE_GAMMA_ENABLE; > > @@ -2872,9 +2916,9 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > > intel_crtc->dspaddr_offset = > > intel_compute_page_offset(dev_priv, &x, &y, > > fb->modifier[0], pixel_size, > > - fb->pitches[0]); > > + fb->pitches[0], rotation); > > linear_offset -= intel_crtc->dspaddr_offset; > > - if (crtc->primary->state->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 a12ac95..ed47ca3 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1139,7 +1139,8 @@ unsigned long intel_compute_page_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 6614adb..8eaebce 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -354,6 +354,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc, > > int pipe = intel_plane->pipe; > > int plane = intel_plane->plane; > > u32 sprctl; > > + unsigned int rotation = dplane->state->rotation; > > unsigned long sprsurf_offset, linear_offset; > > int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); > > const struct drm_intel_sprite_colorkey *key = > > @@ -422,10 +423,10 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc, > > linear_offset = y * fb->pitches[0] + x * pixel_size; > > sprsurf_offset = intel_compute_page_offset(dev_priv, &x, &y, > > fb->modifier[0], pixel_size, > > - fb->pitches[0]); > > + fb->pitches[0], rotation); > > linear_offset -= sprsurf_offset; > > > > - if (dplane->state->rotation == BIT(DRM_ROTATE_180)) { > > + if (rotation == BIT(DRM_ROTATE_180)) { > > sprctl |= SP_ROTATE_180; > > > > x += src_w; > > @@ -491,6 +492,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > > enum pipe pipe = intel_plane->pipe; > > u32 sprctl, sprscale = 0; > > + unsigned int rotation = plane->state->rotation; > > unsigned long sprsurf_offset, linear_offset; > > int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); > > const struct drm_intel_sprite_colorkey *key = > > @@ -554,10 +556,10 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > linear_offset = y * fb->pitches[0] + x * pixel_size; > > sprsurf_offset = intel_compute_page_offset(dev_priv, &x, &y, > > fb->modifier[0], pixel_size, > > - fb->pitches[0]); > > + fb->pitches[0], rotation); > > linear_offset -= sprsurf_offset; > > > > - if (plane->state->rotation == BIT(DRM_ROTATE_180)) { > > + if (rotation == BIT(DRM_ROTATE_180)) { > > sprctl |= SPRITE_ROTATE_180; > > > > /* HSW and BDW does this automagically in hardware */ > > @@ -631,6 +633,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > struct intel_plane *intel_plane = to_intel_plane(plane); > > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > > int pipe = intel_plane->pipe; > > + unsigned int rotation = plane->state->rotation; > > unsigned long dvssurf_offset, linear_offset; > > u32 dvscntr, dvsscale; > > int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); > > @@ -691,10 +694,10 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > linear_offset = y * fb->pitches[0] + x * pixel_size; > > dvssurf_offset = intel_compute_page_offset(dev_priv, &x, &y, > > fb->modifier[0], pixel_size, > > - fb->pitches[0]); > > + fb->pitches[0], rotation); > > linear_offset -= dvssurf_offset; > > > > - if (plane->state->rotation == BIT(DRM_ROTATE_180)) { > > + if (rotation == BIT(DRM_ROTATE_180)) { > > dvscntr |= DVS_ROTATE_180; > > > > x += src_w; > > -- > > 2.4.9 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Wed, Oct 21, 2015 at 02:36:34PM +0300, Ville Syrjälä wrote: > On Wed, Oct 21, 2015 at 12:53:34PM +0200, Daniel Vetter wrote: > > On Wed, Oct 14, 2015 at 07:29:00PM +0300, ville.syrjala@linux.intel.com wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > 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 > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 62 ++++++++++++++++++++++++++++++------ > > > drivers/gpu/drm/i915/intel_drv.h | 3 +- > > > drivers/gpu/drm/i915/intel_sprite.c | 15 +++++---- > > > 3 files changed, 64 insertions(+), 16 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 75be66b..bd55d06 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -2451,13 +2451,50 @@ 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. */ > > > +/* > > > + * Return the tile dimensions in pixel units matching > > > + * the specified rotation angle. > > > + */ > > > +static void intel_rotate_tile_dims(unsigned int *tile_width, > > > + unsigned int *tile_height, > > > + unsigned int *pitch, > > > + unsigned int cpp, > > > + unsigned int rotation) > > > > A rotation enum would be nice, so that we can employ sparse to check it. > > That'd work since sparse treats enums as bitfields, but we'd need to > > add names for the BIT(DRM_ROTATION_*) variants. Just an aside. > > I was thinking of including the '1<<x' in the define already since the > current thing just invites bugs all over. I guess making it an enum at > the same time should be easy. > > > > > > +{ > > > + if (intel_rotation_90_or_270(rotation)) { > > > + WARN_ON(*pitch % *tile_height); > > > + > > > + /* pixel units please */ > > > + *tile_width /= cpp; > > > > Ok, something dawns on me now about tile_width ... it's in bytes, I > > somehow thought it's pixels. And this function doing a behind-the-scenes > > conversions from bytes to pixels is a bit tricky. > > > > Should we have separate tile_pitch and tile_width to unconfuse this? > > Generally foo_width in the modeset code is mostly pixels ... > > Yeah, things are a bit unclear with bytes vs. pixels sometimes. I > usually think of tile width as bytes because then you don't have to > consider the pixel format (well, except for Yf/Ys tiling), but here > we want it in pixels instead. > > I'm not sure tile_pitch is a good name becuse then it might get > confused with the pitch for the fb or fence. But I don't really have > a better name in mind either. Yeah, redefining tile_width to be in pixel would break with existing code. But silently converting from bytes to pixels is worse, and I don't have any better idea than tile_width and tile_pitch for these two. We'd need to update i915_gem_tiling too though for consistency. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 75be66b..bd55d06 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2451,13 +2451,50 @@ 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. */ +/* + * Return the tile dimensions in pixel units matching + * the specified rotation angle. + */ +static void intel_rotate_tile_dims(unsigned int *tile_width, + unsigned int *tile_height, + unsigned int *pitch, + unsigned int cpp, + unsigned int rotation) +{ + if (intel_rotation_90_or_270(rotation)) { + WARN_ON(*pitch % *tile_height); + + /* pixel units please */ + *tile_width /= cpp; + + /* + * Coordinate space is rotated, orient + * our tile dimensions the same way + */ + swap(*tile_width, *tile_height); + } else { + WARN_ON(*pitch % *tile_width); + + /* pixel units please */ + *tile_width /= cpp; + *pitch /= cpp; + } +} + +/* + * 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. + */ unsigned long intel_compute_page_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; @@ -2467,13 +2504,16 @@ unsigned long intel_compute_page_offset(struct drm_i915_private *dev_priv, tile_width = intel_tile_width(dev_priv, fb_modifier, cpp); tile_height = tile_size / tile_width; + intel_rotate_tile_dims(&tile_width, &tile_height, + &pitch, cpp, rotation); + 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 / tile_width) + tiles) * tile_size; } else { unsigned int alignment = intel_linear_alignment(dev_priv) - 1; unsigned int offset; @@ -2685,6 +2725,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, bool visible = to_intel_plane_state(primary->state)->visible; struct drm_i915_gem_object *obj; int plane = intel_crtc->plane; + unsigned int rotation; unsigned long linear_offset; u32 dspcntr; u32 reg = DSPCNTR(plane); @@ -2704,6 +2745,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, if (WARN_ON(obj == NULL)) return; + rotation = crtc->primary->state->rotation; pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); dspcntr = DISPPLANE_GAMMA_ENABLE; @@ -2768,7 +2810,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, intel_crtc->dspaddr_offset = intel_compute_page_offset(dev_priv, &x, &y, fb->modifier[0], pixel_size, - fb->pitches[0]); + fb->pitches[0], rotation); linear_offset -= intel_crtc->dspaddr_offset; } else { intel_crtc->dspaddr_offset = linear_offset; @@ -2814,6 +2856,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, bool visible = to_intel_plane_state(primary->state)->visible; struct drm_i915_gem_object *obj; int plane = intel_crtc->plane; + unsigned int rotation; unsigned long linear_offset; u32 dspcntr; u32 reg = DSPCNTR(plane); @@ -2830,6 +2873,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, if (WARN_ON(obj == NULL)) return; + rotation = crtc->primary->state->rotation; pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); dspcntr = DISPPLANE_GAMMA_ENABLE; @@ -2872,9 +2916,9 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, intel_crtc->dspaddr_offset = intel_compute_page_offset(dev_priv, &x, &y, fb->modifier[0], pixel_size, - fb->pitches[0]); + fb->pitches[0], rotation); linear_offset -= intel_crtc->dspaddr_offset; - if (crtc->primary->state->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 a12ac95..ed47ca3 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1139,7 +1139,8 @@ unsigned long intel_compute_page_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 6614adb..8eaebce 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -354,6 +354,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc, int pipe = intel_plane->pipe; int plane = intel_plane->plane; u32 sprctl; + unsigned int rotation = dplane->state->rotation; unsigned long sprsurf_offset, linear_offset; int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); const struct drm_intel_sprite_colorkey *key = @@ -422,10 +423,10 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc, linear_offset = y * fb->pitches[0] + x * pixel_size; sprsurf_offset = intel_compute_page_offset(dev_priv, &x, &y, fb->modifier[0], pixel_size, - fb->pitches[0]); + fb->pitches[0], rotation); linear_offset -= sprsurf_offset; - if (dplane->state->rotation == BIT(DRM_ROTATE_180)) { + if (rotation == BIT(DRM_ROTATE_180)) { sprctl |= SP_ROTATE_180; x += src_w; @@ -491,6 +492,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_i915_gem_object *obj = intel_fb_obj(fb); enum pipe pipe = intel_plane->pipe; u32 sprctl, sprscale = 0; + unsigned int rotation = plane->state->rotation; unsigned long sprsurf_offset, linear_offset; int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); const struct drm_intel_sprite_colorkey *key = @@ -554,10 +556,10 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, linear_offset = y * fb->pitches[0] + x * pixel_size; sprsurf_offset = intel_compute_page_offset(dev_priv, &x, &y, fb->modifier[0], pixel_size, - fb->pitches[0]); + fb->pitches[0], rotation); linear_offset -= sprsurf_offset; - if (plane->state->rotation == BIT(DRM_ROTATE_180)) { + if (rotation == BIT(DRM_ROTATE_180)) { sprctl |= SPRITE_ROTATE_180; /* HSW and BDW does this automagically in hardware */ @@ -631,6 +633,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, struct intel_plane *intel_plane = to_intel_plane(plane); struct drm_i915_gem_object *obj = intel_fb_obj(fb); int pipe = intel_plane->pipe; + unsigned int rotation = plane->state->rotation; unsigned long dvssurf_offset, linear_offset; u32 dvscntr, dvsscale; int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); @@ -691,10 +694,10 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, linear_offset = y * fb->pitches[0] + x * pixel_size; dvssurf_offset = intel_compute_page_offset(dev_priv, &x, &y, fb->modifier[0], pixel_size, - fb->pitches[0]); + fb->pitches[0], rotation); linear_offset -= dvssurf_offset; - if (plane->state->rotation == BIT(DRM_ROTATE_180)) { + if (rotation == BIT(DRM_ROTATE_180)) { dvscntr |= DVS_ROTATE_180; x += src_w;