[Intel-gfx] [PATCH 2/2] drm/i915: Add 180 degree primary plane rotation support
Ville Syrjälä
ville.syrjala at linux.intel.com
Fri Aug 22 10:14:06 CEST 2014
On Fri, Aug 22, 2014 at 09:29:56AM +0530, Jindal, Sonika wrote:
>
>
> On 8/21/2014 7:07 PM, Ville Syrjälä wrote:
> > 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 at intel.com wrote:
> >>>> From: Sonika Jindal <sonika.jindal at intel.com>
> >>>>
> >>>> 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 at intel.com>
> >>>> Signed-off-by: Sagar Kamble <sagar.a.kamble at intel.com>
> >>>> Signed-off-by: Sonika Jindal <sonika.jindal at intel.com>
> >>>> ---
> >>>> 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;
> >>>> + }
> >>>>
> This check is present here, so assume it is safe to remove from
> set_property.
> >>>> /* 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.
> >
> Since, update_fbc is already getting called from intel_pipe_set_base and
> we are putting a check for rotation in update_fbc (as part of this
> patch), I am removing this part from here (set_property as well as
> setplane) altogether. Will be sending the updated patch.
We call update fbc too late. We need to call disable_fbc _before_
changing the rotation of plane to 180 degrees, and we need to call
update_fbc after to make sure fbc gets re-enabled if rotation is
changed back to 0. Eventually we need to refactor the update_fbc
checks a bit so that we don't have to duplicate the code for checking
this stuff.
Or someone could just take my fbc patches (or at least part of them)
which should fix it. Well those patchs probably would need some
rebasing by now. Anyways, for now I think it's enough to duplicate the
rotation check before we touch the plane and call disable_fbc when
appropriate. After changing the rotation, the current update_fbc call
is enough.
Anything beyond that amounts to actually fixing fbc. So until someone
else starts to care about fbc and either fixes it the way they like or
goes through my patches, we can accept a bit of code duplication IMO.
> >>> 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 at lists.freedesktop.org
> >>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>>
> >
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list