On Thu, Aug 21, 2014 at 05:14:35PM +0530, Jindal, Sonika wrote:
>
>
> On 8/21/2014 2:03 PM, Ville Syrjälä wrote:
> > On Thu, Aug 21, 2014 at 11:45:41AM +0530, sonika.jindal@xxxxxxxxx wrote:
> >> From: Sonika Jindal <sonika.jindal@xxxxxxxxx>
> >>
> >> Primary planes support 180 degree rotation. Expose the feature
> >> through rotation drm property.
> >>
> >> v2: Calculating linear/tiled offsets based on pipe source width and
> >> height. Added 180 degree rotation support in ironlake_update_plane.
> >>
> >> v3: Checking if CRTC is active before issueing update_plane. Added
> >> wait for vblank to make sure we dont overtake page flips. Disabling
> >> FBC since it does not work with rotated planes.
> >>
> >> v4: Updated rotation checks for pending flips, fbc disable. Creating
> >> rotation property only for Gen4 onwards. Property resetting as part
> >> of lastclose.
> >>
> >> v5: Resetting property in i915_driver_lastclose properly for planes
> >> and crtcs. Fixed linear offset calculation that was off by 1 w.r.t
> >> width in i9xx_update_plane and ironlake_update_plane. Removed tab
> >> based indentation and unnecessary braces in intel_crtc_set_property
> >> and intel_update_fbc. FBC and flip related checks should be done only
> >> for valid crtcs.
> >>
> >> v6: Minor nits in FBC disable checks for comments in intel_crtc_set_property
> >> and positioning the disable code in intel_update_fbc.
> >>
> >> v7: In case rotation property on inactive crtc is updated, we return
> >> successfully printing debug log as crtc is inactive and only property change
> >> is preserved.
> >>
> >> v8: update_plane is changed to update_primary_plane, crtc->fb is changed to
> >> crtc->primary->fb and return value of update_primary_plane is ignored.
> >>
> >> v9: added rotation property to primary plane instead of crtc. Removing reset
> >> of rotation property from lastclose. rotation_property is moved to
> >> drm_mode_config, so drm layer will take care of resetting. Adding updation of
> >> fbc when rotation is set to 0. Allowing rotation only if value is
> >> different than old one.
> >>
> >> v10: Calling intel_primary_plane_setplane instead of update_primary_plane in
> >> set_property(Daniel).
> >>
> >> v11: Using same set_property function for both primary and sprite, Adding
> >> primary plane specific code in the same function (Matt).
> >>
> >> Testcase: igt/kms_rotation_crc
> >>
> >> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx>
> >> Signed-off-by: Sagar Kamble <sagar.a.kamble@xxxxxxxxx>
> >> Signed-off-by: Sonika Jindal <sonika.jindal@xxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/i915/i915_reg.h | 1 +
> >> drivers/gpu/drm/i915/intel_display.c | 57 +++++++++++++++++++++++++++++++---
> >> drivers/gpu/drm/i915/intel_drv.h | 3 ++
> >> drivers/gpu/drm/i915/intel_pm.c | 6 ++++
> >> drivers/gpu/drm/i915/intel_sprite.c | 33 ++++++++++++++++++--
> >> 5 files changed, 94 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> index 203062e..142ac52 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -4214,6 +4214,7 @@ enum punit_power_well {
> >> #define DISPPLANE_NO_LINE_DOUBLE 0
> >> #define DISPPLANE_STEREO_POLARITY_FIRST 0
> >> #define DISPPLANE_STEREO_POLARITY_SECOND (1<<18)
> >> +#define DISPPLANE_ROTATE_180 (1<<15)
> >> #define DISPPLANE_TRICKLE_FEED_DISABLE (1<<14) /* Ironlake */
> >> #define DISPPLANE_TILED (1<<10)
> >> #define _DSPAADDR 0x70184
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index f2a8797..22851a9 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -2384,6 +2384,9 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
> >> unsigned long linear_offset;
> >> u32 dspcntr;
> >> u32 reg = DSPCNTR(plane);
> >> + int pixel_size;
> >> +
> >> + pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> >>
> >> if (!intel_crtc->primary_enabled) {
> >> I915_WRITE(reg, 0);
> >> @@ -2411,6 +2414,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
> >> (intel_crtc->config.pipe_src_w - 1));
> >> I915_WRITE(DSPPOS(plane), 0);
> >> }
> >> + dspcntr &= ~DISPPLANE_ROTATE_180;
> >
> > No longer needed. We start building DSPCNTR from scratch.
> >
> >>
> >> switch (fb->pixel_format) {
> >> case DRM_FORMAT_C8:
> >> @@ -2450,8 +2454,6 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
> >> if (IS_G4X(dev))
> >> dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
> >>
> >> - I915_WRITE(reg, dspcntr);
> >> -
> >> linear_offset = y * fb->pitches[0] + x * (fb->bits_per_pixel / 8);
> >>
> >> if (INTEL_INFO(dev)->gen >= 4) {
> >> @@ -2464,6 +2466,21 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
> >> intel_crtc->dspaddr_offset = linear_offset;
> >> }
> >>
> >> + if (to_intel_plane(crtc->primary)->rotation == BIT(DRM_ROTATE_180)) {
> >> + dspcntr |= DISPPLANE_ROTATE_180;
> >> +
> >> + x += (intel_crtc->config.pipe_src_w - 1);
> >> + y += (intel_crtc->config.pipe_src_h - 1);
> >> +
> >> + /* Finding the last pixel of the last line of the display
> >> + data and adding to linear_offset*/
> >> + linear_offset += (intel_crtc->config.pipe_src_h - 1) *
> >> + fb->pitches[0] +
> >> + (intel_crtc->config.pipe_src_w - 1) * pixel_size;
> >> + }
> >> +
> >> + I915_WRITE(reg, dspcntr);
> >> +
> >> DRM_DEBUG_KMS("Writing base %08lX %08lX %d %d %d\n",
> >> i915_gem_obj_ggtt_offset(obj), linear_offset, x, y,
> >> fb->pitches[0]);
> >> @@ -2490,6 +2507,9 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
> >> unsigned long linear_offset;
> >> u32 dspcntr;
> >> u32 reg = DSPCNTR(plane);
> >> + int pixel_size;
> >> +
> >> + pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> >>
> >> if (!intel_crtc->primary_enabled) {
> >> I915_WRITE(reg, 0);
> >> @@ -2505,6 +2525,8 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
> >> if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >> dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
> >>
> >> + dspcntr &= ~DISPPLANE_ROTATE_180;
> >> +
> >
> > ditto
> >
> >> switch (fb->pixel_format) {
> >> case DRM_FORMAT_C8:
> >> dspcntr |= DISPPLANE_8BPP;
> >> @@ -2538,14 +2560,26 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
> >> if (!IS_HASWELL(dev) && !IS_BROADWELL(dev))
> >> dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
> >>
> >> - I915_WRITE(reg, dspcntr);
> >> -
> >> linear_offset = y * fb->pitches[0] + x * (fb->bits_per_pixel / 8);
> >> intel_crtc->dspaddr_offset =
> >> intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
> >> fb->bits_per_pixel / 8,
> >> fb->pitches[0]);
> >> linear_offset -= intel_crtc->dspaddr_offset;
> >> + if (to_intel_plane(crtc->primary)->rotation == BIT(DRM_ROTATE_180)) {
> >> + dspcntr |= DISPPLANE_ROTATE_180;
> >> +
> >> + if (!IS_HASWELL(dev) && !IS_BROADWELL(dev)) {
> >> + x += (intel_crtc->config.pipe_src_w - 1);
> >> + y += (intel_crtc->config.pipe_src_h - 1);
> >> + linear_offset +=
> >> + (intel_crtc->config.pipe_src_h - 1) *
> >> + fb->pitches[0] + (intel_crtc->config.pipe_src_w - 1) *
> >> + pixel_size;
> >
> > Formatting looks rather wonky. In this case I think we need to
> > accept that we can't make it fit into 80 columns neatly and just
> > use longer lines.
> >
> > I would just make it:
> > linear_offset +=
> > (intel_crtc->config.pipe_src_h - 1) * fb->pitches[0] +
> > (intel_crtc->config.pipe_src_w - 1) * pixel_size;
> >
> > so that we have the x vs. y offsets neatly on their own lines. Same for
> > the other instance of this code. Or if you prefer you could to refactor
> > that bit of code into a small helper function.
> >
> >> + }
> >> + }
> >> +
> >> + I915_WRITE(reg, dspcntr);
> >>
> >> DRM_DEBUG_KMS("Writing base %08lX %08lX %d %d %d\n",
> >> i915_gem_obj_ggtt_offset(obj), linear_offset, x, y,
> >> @@ -11717,6 +11751,7 @@ static const struct drm_plane_funcs intel_primary_plane_funcs = {
> >> .update_plane = intel_primary_plane_setplane,
> >> .disable_plane = intel_primary_plane_disable,
> >> .destroy = intel_plane_destroy,
> >> + .set_property = intel_plane_set_property
> >> };
> >>
> >> static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> >> @@ -11734,6 +11769,7 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> >> primary->max_downscale = 1;
> >> primary->pipe = pipe;
> >> primary->plane = pipe;
> >> + primary->rotation = BIT(DRM_ROTATE_0);
> >> if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> >> primary->plane = !pipe;
> >>
> >> @@ -11749,6 +11785,19 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> >> &intel_primary_plane_funcs,
> >> intel_primary_formats, num_formats,
> >> DRM_PLANE_TYPE_PRIMARY);
> >> +
> >> + if (INTEL_INFO(dev)->gen >= 4) {
> >> + if (!dev->mode_config.rotation_property)
> >> + dev->mode_config.rotation_property =
> >> + drm_mode_create_rotation_property(dev,
> >> + BIT(DRM_ROTATE_0) |
> >> + BIT(DRM_ROTATE_180));
> >> + if (dev->mode_config.rotation_property)
> >> + drm_object_attach_property(&primary->base.base,
> >> + dev->mode_config.rotation_property,
> >> + primary->rotation);
> >> + }
> >> +
> >> return &primary->base;
> >> }
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index d683a20..3e5d6b3 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -1093,6 +1093,9 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
> >> int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane);
> >> void intel_flush_primary_plane(struct drm_i915_private *dev_priv,
> >> enum plane plane);
> >> +int intel_plane_set_property(struct drm_plane *plane,
> >> + struct drm_property *prop,
> >> + uint64_t val);
> >> int intel_plane_restore(struct drm_plane *plane);
> >> void intel_plane_disable(struct drm_plane *plane);
> >> int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >> index c8f744c..1ab3e11 100644
> >> --- a/drivers/gpu/drm/i915/intel_pm.c
> >> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >> @@ -581,6 +581,12 @@ void intel_update_fbc(struct drm_device *dev)
> >> DRM_DEBUG_KMS("framebuffer not tiled or fenced, disabling compression\n");
> >> goto out_disable;
> >> }
> >> + if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> >> + to_intel_plane(crtc->primary)->rotation != BIT(DRM_ROTATE_0)) {
> >> + if (set_no_fbc_reason(dev_priv, FBC_UNSUPPORTED_MODE))
> >> + DRM_DEBUG_KMS("Rotation unsupported, disabling\n");
> >> + goto out_disable;
> >> + }
> >>
> >> /* If the kernel debugger is active, always disable compression */
> >> if (in_dbg_master())
> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> >> index 0bdb00b..ad09ba1 100644
> >> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >> @@ -1218,12 +1218,15 @@ out_unlock:
> >> return ret;
> >> }
> >>
> >> -static int intel_plane_set_property(struct drm_plane *plane,
> >> +int intel_plane_set_property(struct drm_plane *plane,
> >> struct drm_property *prop,
> >> uint64_t val)
> >> {
> >> struct drm_device *dev = plane->dev;
> >> + struct drm_i915_private *dev_priv = dev->dev_private;
> >> struct intel_plane *intel_plane = to_intel_plane(plane);
> >> + struct drm_crtc *crtc = plane->crtc;
> >> + struct intel_crtc *intel_crtc;
> >> uint64_t old_val;
> >> int ret = -ENOENT;
> >>
> >> @@ -1232,6 +1235,32 @@ static int intel_plane_set_property(struct drm_plane *plane,
> >> if (hweight32(val & 0xf) != 1)
> >> return -EINVAL;
> >>
> >> + if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> >> + if (crtc == NULL)
> >> + return -EINVAL;
> >> +
> >> + intel_crtc = to_intel_crtc(plane->crtc);
> >> +
> >> + if (intel_crtc && intel_crtc->active &&
> >> + intel_crtc->primary_enabled) {
> >> + intel_crtc_wait_for_pending_flips(crtc);
> >> +
> >> + /* FBC does not work on some platforms for rotated
> >> + planes, so disable it when rotation is not 0 and update
> >> + it when rotation is set back to 0 */
> >> + if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev)) {
> >> + if (dev_priv->fbc.plane == intel_crtc->plane &&
> >> + intel_plane->rotation != BIT(DRM_ROTATE_0))
> >> + intel_disable_fbc(dev);
> >> + else if (intel_plane->rotation == BIT(DRM_ROTATE_0)) {
> >> + mutex_lock(&dev->struct_mutex);
> >> + intel_update_fbc(dev);
> >> + mutex_unlock(&dev->struct_mutex);
> >> + }
> >> + }
> >> + }
> >> + }
> >
> > I think Daniel wanted to shovel all of this into
> > intel_primary_plane_setplane().
> >
> I am not sure about that, Matt suggested this change and here we do
> disable fbc for rotated plane. So, it makes more sense to keep it in
> set_property.
> What do you suggest?
We anyway may have to disable fbc in setplace due to other factors
(pixel format etc.) so having all the logic in the same place makes
sense.
> > In fact the intel_update_fbc() is already there in
> > intel_pipe_set_base(), although I think should get moved out so that it
> > can be placed on the correct side of intel_enable_primary_hw_plane().
> > But that can be done as a followup later. And we also lack the
> > disable_fbc call from intel_primary_plane_disable().
> >
> >> +
> >> old_val = intel_plane->rotation;
> >> intel_plane->rotation = val;
> >> ret = intel_plane_restore(plane);
> >> @@ -1249,7 +1278,7 @@ int intel_plane_restore(struct drm_plane *plane)
> >> if (!plane->crtc || !plane->fb)
> >> return 0;
> >>
> >> - return intel_update_plane(plane, plane->crtc, plane->fb,
> >> + return plane->funcs->update_plane(plane, plane->crtc, plane->fb,
> >> intel_plane->crtc_x, intel_plane->crtc_y,
> >> intel_plane->crtc_w, intel_plane->crtc_h,
> >> intel_plane->src_x, intel_plane->src_y,
> >> --
> >> 1.7.10.4
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
