Re: [PATCH] drm/i915/cnl: Add support for horizontal plane flipping

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Dec 15, 2017 at 10:22:25PM +0000, Chris Wilson wrote:
> Quoting Rodrigo Vivi (2017-12-15 21:38:00)
> > From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > 
> > CNL supports horizontal plane flipping on non-linear plane formats.
> > 
> > v2:
> > - Avoid BUG unlike elsewhere in the code (Ville)
> > - Hoist the rotation-tiling restriction check (Ville)
> > 
> > v3 (Rodrigo):
> > - Rebased after a while.
> > - Fix small indentation issues.
> > 
> > Bspec: 7656
> > Suggested-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h      |  1 +
> >  drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++++++++++++++++++++-----
> >  2 files changed, 36 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 09bf043c1c2e..14ade8f88dcc 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6329,6 +6329,7 @@ enum {
> >  #define   PLANE_CTL_TILED_X                    (  1 << 10)
> >  #define   PLANE_CTL_TILED_Y                    (  4 << 10)
> >  #define   PLANE_CTL_TILED_YF                   (  5 << 10)
> > +#define   PLANE_CTL_FLIP_HORIZONTAL            (  1 << 8)
> >  #define   PLANE_CTL_ALPHA_MASK                 (0x3 << 4) /* Pre-GLK */
> >  #define   PLANE_CTL_ALPHA_DISABLE              (  0 << 4)
> >  #define   PLANE_CTL_ALPHA_SW_PREMULTIPLY       (  2 << 4)
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index efa6c6d19664..fb5ee413dc30 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3073,6 +3073,12 @@ int skl_check_plane_surface(struct intel_plane_state *plane_state)
> >         unsigned int rotation = plane_state->base.rotation;
> >         int ret;
> >  
> > +       if (rotation & DRM_MODE_REFLECT_X &&
> > +           fb->modifier == DRM_FORMAT_MOD_LINEAR) {
> > +               DRM_DEBUG_KMS("horizontal flip is not supported with linear surface formats\n");
> > +               return -EINVAL;
> > +       }
> > +
> >         if (!plane_state->base.visible)
> >                 return 0;
> >  
> > @@ -3453,9 +3459,9 @@ static u32 skl_plane_ctl_tiling(uint64_t fb_modifier)
> >         return 0;
> >  }
> >  
> > -static u32 skl_plane_ctl_rotation(unsigned int rotation)
> > +static u32 skl_plane_ctl_rotate(unsigned int rotate)
> >  {
> > -       switch (rotation) {
> > +       switch (rotate) {
> >         case DRM_MODE_ROTATE_0:
> >                 break;
> >         /*
> > @@ -3469,7 +3475,22 @@ static u32 skl_plane_ctl_rotation(unsigned int rotation)
> >         case DRM_MODE_ROTATE_270:
> >                 return PLANE_CTL_ROTATE_90;
> >         default:
> > -               MISSING_CASE(rotation);
> > +               MISSING_CASE(rotate);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static u32 cnl_plane_ctl_flip(unsigned int reflect)
> > +{
> > +       switch (reflect) {
> > +       case 0:
> > +               break;
> > +       case DRM_MODE_REFLECT_X:
> > +               return PLANE_CTL_FLIP_HORIZONTAL;
> > +       case DRM_MODE_REFLECT_Y:
> > +       default:
> > +               MISSING_CASE(reflect);
> >         }
> >  
> >         return 0;
> > @@ -3497,7 +3518,11 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> >  
> >         plane_ctl |= skl_plane_ctl_format(fb->format->format);
> >         plane_ctl |= skl_plane_ctl_tiling(fb->modifier);
> > -       plane_ctl |= skl_plane_ctl_rotation(rotation);
> > +       plane_ctl |= skl_plane_ctl_rotate(rotation & DRM_MODE_ROTATE_MASK);
> > +
> > +       if (INTEL_GEN(dev_priv) >= 10)
> > +               plane_ctl |= cnl_plane_ctl_flip(rotation &
> > +                                               DRM_MODE_REFLECT_MASK);
> >  
> >         if (key->flags & I915_SET_COLORKEY_DESTINATION)
> >                 plane_ctl |= PLANE_CTL_KEY_ENABLE_DESTINATION;
> > @@ -13300,7 +13325,12 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
> >         if (ret)
> >                 goto fail;
> >  
> > -       if (INTEL_GEN(dev_priv) >= 9) {
> > +       if (INTEL_GEN(dev_priv) >= 10) {
> > +               supported_rotations =
> > +                       DRM_MODE_ROTATE_0 | DRM_MODE_ROTATE_90 |
> > +                       DRM_MODE_ROTATE_180 | DRM_MODE_ROTATE_270 |
> > +                       DRM_MODE_REFLECT_X;
> 
> REFLECT_Y == ROTATE_180 | REFLECT_X
> 
> Where does that conversion belong? Should the kernel just say it
> supports REFLECT_Y and fixup skl_plane_ctl(), that seems pretty simple.

We have drm_rotation_simplify() for that exactly. I hooked it up for
msm some time ago: 574a37b1bb07 ("drm/msm/mdp5: Advertize 180 degree
rotation")

I guess we could even move that into to the core, but that would
require duplicating supported_rotations and plane_state->rotation
into user visible and internal values.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx