[Intel-gfx] [PATCH] drm/i915/cnl: Add support for horizontal plane flipping

[Rodrigo Vivi]

On Sat, Dec 16, 2017 at 03:25:01PM +0000, Ville Syrjälä wrote:
> 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 at 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 at intel.com>
> > > Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > Cc: Anusha Srivatsa <anusha.srivatsa at intel.com>
> > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > > Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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.

Ok... So I'm taking this as good enough for now and merging the patch.

We can continue the discussion, but for following up patches if we
decided we need something different...

Thanks,
Rodrigo.

> 
> -- 
> Ville Syrjälä
> Intel OTC