From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: "Jindal, Sonika" <sonika.jindal@intel.com>
Cc: intel-gfx@lists.freedesktop.org, Sagar Kamble <sagar.a.kamble@intel.com>
Subject: Re: [PATCH 2/2] drm/i915: Add 180 degree primary plane rotation support
Date: Thu, 21 Aug 2014 16:37:33 +0300 [thread overview]
Message-ID: <20140821133733.GM4193@intel.com> (raw)
In-Reply-To: <53F5DBA3.9000604@intel.com>
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@intel.com wrote:
> >> From: Sonika Jindal <sonika.jindal@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@intel.com>
> >> Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> >> Signed-off-by: Sonika Jindal <sonika.jindal@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;
> >> + }
> >>
> >> /* 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@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
--
Ville Syrjälä
Intel OTC
next prev parent reply other threads:[~2014-08-21 13:37 UTC|newest]
Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-15 8:40 [PATCH 0/6] Add 180 degree primary and sprite rotation sonika.jindal
2014-07-15 8:40 ` [PATCH 1/6] drm/i915: Add 180 degree sprite rotation support sonika.jindal
2014-07-15 8:40 ` [PATCH 2/6] drm/i915: Make intel_plane_restore() return an error sonika.jindal
2014-07-15 8:40 ` [PATCH 3/6] drm: Add rotation_property to mode_config sonika.jindal
2014-07-15 12:13 ` [PATCH] drm: Add rotation_property to mode_config and creating it sonika.jindal
2014-07-28 15:29 ` Ville Syrjälä
2014-07-28 18:47 ` Daniel Vetter
2014-07-29 9:40 ` Ville Syrjälä
2014-07-29 10:30 ` Daniel Vetter
2014-07-31 4:09 ` Jindal, Sonika
2014-08-04 10:29 ` Jindal, Sonika
2014-08-04 11:54 ` Ville Syrjälä
2014-08-04 12:04 ` Jindal, Sonika
2014-07-15 8:40 ` [PATCH 4/6] drm/i915: Add rotation property for sprites sonika.jindal
2014-07-15 12:12 ` [PATCH] " sonika.jindal
2014-07-15 8:40 ` [PATCH 5/6] drm/i915: Add 180 degree primary plane rotation support sonika.jindal
2014-07-15 9:11 ` Daniel Vetter
2014-07-15 9:38 ` Jindal, Sonika
2014-07-15 10:31 ` Daniel Vetter
2014-07-15 11:30 ` [PATCH 0/3 v2] Moving creation of rotation_property to drm layer sonika.jindal
2014-07-15 11:30 ` [PATCH 1/4] drm: Add rotation_property to mode_config and creating it sonika.jindal
2014-07-15 11:30 ` [PATCH 2/4] drm/i915: Add rotation property for sprites sonika.jindal
2014-07-15 11:30 ` [PATCH 3/4] drm/i915: Add 180 degree primary plane rotation support sonika.jindal
2014-07-15 12:02 ` [PATCH 0/3 v2] Moving creation of rotation_property to drm layer Daniel Vetter
2014-07-15 12:10 ` Jindal, Sonika
2014-07-15 12:10 ` [PATCH] drm/i915: Add 180 degree primary plane rotation support sonika.jindal
2014-08-07 11:45 ` [PATCH 5/6] " Daniel Vetter
2014-08-07 12:11 ` Daniel Vetter
2014-08-11 11:07 ` Jindal, Sonika
2014-08-11 11:42 ` Daniel Vetter
2014-08-11 12:07 ` Jindal, Sonika
2014-08-11 12:23 ` Daniel Vetter
2014-08-12 3:25 ` Jindal, Sonika
2014-08-19 6:26 ` [PATCH 0/2] 180 Primary plane rotation: calling setplane in set_property sonika.jindal
2014-08-19 6:26 ` [PATCH 1/2] drm/i915: Updating plane parameters for primary plane in setplane sonika.jindal
2014-08-19 20:50 ` Matt Roper
2014-08-20 5:53 ` Jindal, Sonika
2014-08-21 6:14 ` sonika.jindal
2014-08-25 20:42 ` Daniel Vetter
2014-08-19 6:26 ` [PATCH 2/2] drm/i915: Add 180 degree primary plane rotation support sonika.jindal
2014-08-19 22:51 ` Matt Roper
2014-08-20 5:36 ` Jindal, Sonika
2014-08-21 6:15 ` sonika.jindal
2014-08-21 8:33 ` Ville Syrjälä
2014-08-21 11:44 ` Jindal, Sonika
2014-08-21 13:37 ` Ville Syrjälä [this message]
2014-08-22 3:59 ` Jindal, Sonika
2014-08-22 6:58 ` [PATCH] " sonika.jindal
2014-08-22 8:14 ` [PATCH 2/2] " Ville Syrjälä
2014-08-22 8:22 ` Jindal, Sonika
2014-08-22 8:36 ` [PATCH] " sonika.jindal
2014-08-25 20:50 ` Daniel Vetter
2014-07-15 8:40 ` [PATCH 6/6] drm: Resetting rotation property sonika.jindal
2014-08-04 12:01 ` Ville Syrjälä
2014-08-04 12:03 ` Jindal, Sonika
2014-07-15 10:52 ` [PATCH 0/6] Add 180 degree primary and sprite rotation Damien Lespiau
2014-07-15 10:55 ` Jindal, Sonika
2014-07-15 10:57 ` Damien Lespiau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140821133733.GM4193@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=sagar.a.kamble@intel.com \
--cc=sonika.jindal@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox