Re: [PATCH 5/7] drm/i915: Use & instead if == to check for rotations

Ville Syrjälä
2016-07-20T14:14:46Z

On Wed, Jul 20, 2016 at 05:01:00PM +0300, Joonas Lahtinen wrote:
> On ke, 2016-07-20 at 16:18 +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Using == to check for 180 degree rotation only works as long as the
> > reflection bits aren't set. That will change soon enough for CHV, so
> > let's stop doing things the wrong way.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>
> Reading through the patch makes me wanna do the change;
>
> #define DRM_ROTATE_180 BIT(x)
>
> even more. Maybe with coccinelle.

Yeah, that one is annoying. One problem is the way the bits are
specified for bitmask properties (as shifts), so we still need the
names for the shifts too :(

>
> Regards, Joonas
>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 9 +++++----
> >  drivers/gpu/drm/i915/intel_sprite.c  | 6 +++---
> >  2 files changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 93ecb259c5ce..79c1a8b89d1d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2688,7 +2688,7 @@ static void i9xx_update_primary_plane(struct drm_plane *primary,
> >   intel_crtc->dspaddr_offset = linear_offset;
> >   }
> >
> > - if (rotation == BIT(DRM_ROTATE_180)) {
> > + if (rotation & BIT(DRM_ROTATE_180)) {
> >   dspcntr |= DISPPLANE_ROTATE_180;
> >
> >   x += (crtc_state->pipe_src_w - 1);
> > @@ -2791,7 +2791,8 @@ static void ironlake_update_primary_plane(struct drm_plane *primary,
> >   intel_compute_tile_offset(&x, &y, fb, 0,
> >     fb->pitches[0], rotation);
> >   linear_offset -= intel_crtc->dspaddr_offset;
> > - if (rotation == BIT(DRM_ROTATE_180)) {
> > +
> > + if (rotation & BIT(DRM_ROTATE_180)) {
> >   dspcntr |= DISPPLANE_ROTATE_180;
> >
> >   if (!IS_HASWELL(dev) && !IS_BROADWELL(dev)) {
> > @@ -10263,7 +10264,7 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
> >   if (HAS_DDI(dev))
> >   cntl |= CURSOR_PIPE_CSC_ENABLE;
> >
> > - if (plane_state->base.rotation == BIT(DRM_ROTATE_180))
> > + if (plane_state->base.rotation & BIT(DRM_ROTATE_180))
> >   cntl |= CURSOR_ROTATE_180;
> >   }
> >
> > @@ -10309,7 +10310,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> >
> >   /* ILK+ do this automagically */
> >   if (HAS_GMCH_DISPLAY(dev) &&
> > -     plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
> > +     plane_state->base.rotation & BIT(DRM_ROTATE_180)) {
> >   base += (plane_state->base.crtc_h *
> >    plane_state->base.crtc_w - 1) * 4;
> >   }
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index bc41c1bba04c..6b815d57d75a 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -444,7 +444,7 @@ vlv_update_plane(struct drm_plane *dplane,
> >      fb->pitches[0], rotation);
> >   linear_offset -= sprsurf_offset;
> >
> > - if (rotation == BIT(DRM_ROTATE_180)) {
> > + if (rotation & BIT(DRM_ROTATE_180)) {
> >   sprctl |= SP_ROTATE_180;
> >
> >   x += src_w;
> > @@ -577,7 +577,7 @@ ivb_update_plane(struct drm_plane *plane,
> >      fb->pitches[0], rotation);
> >   linear_offset -= sprsurf_offset;
> >
> > - if (rotation == BIT(DRM_ROTATE_180)) {
> > + if (rotation & BIT(DRM_ROTATE_180)) {
> >   sprctl |= SPRITE_ROTATE_180;
> >
> >   /* HSW and BDW does this automagically in hardware */
> > @@ -714,7 +714,7 @@ ilk_update_plane(struct drm_plane *plane,
> >      fb->pitches[0], rotation);
> >   linear_offset -= dvssurf_offset;
> >
> > - if (rotation == BIT(DRM_ROTATE_180)) {
> > + if (rotation & BIT(DRM_ROTATE_180)) {
> >   dvscntr |= DVS_ROTATE_180;
> >
> >   x += src_w;
> --
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation

--
Ville Syrjälä
Intel OTC