Re: [PATCH 2/5] drm/i915: Rename primary plane rotation property to "plane-rotation"

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

On Thu, Feb 13, 2014 at 03:46:39PM +0200, Ville Syrjälä wrote:
> On Thu, Feb 13, 2014 at 06:07:27PM +0530, Sagar Arun Kamble wrote:
> > On Wed, 2014-02-12 at 23:15 +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > I'd prefer have the crtc "rotation" property rotate the entire crtc
> > > (planes and all). So for that reason we'd need to come up with some
> > > other name for the "rotate the primary plane only" property.
> > > 
> > > Originally I had though that omapdrm had already made the decision for
> > > us, but after another look, it looks like it never attaches the
> > > "rotation" property to the crtc. So we can still change the name
> > > without any ABI breakage.
> > > 
> > > Suggestions for better naming scheme are also welcome....
> > I would suggest name to be "primary-rotation" or "primary_rotation". It
> > seems more aligned to member variable primary_rotation as well.
> 
> Well, "primary plane" is an Intel term, so I don't know if other people
> would find it sensible. But I guess you can consider any plane "primary"
> if it's assigned to act as the crtc scanout engine...

It seems I need to scrap this plan actually. Rob hit me with the clue
bat on irc, and omapdrm does in fact install the "rotation" prop on
the crtc. So I guess I need to rename the "rotation" prop to something
else "crtc-rotation" maybe? Anyone have a good name up their sleeve?

> 
> > > 
> > > TODO: squash into "drm/i915: Add 180 degree primary plane rotation support"?
> > > 
> > > Cc: Sagar Kamble <sagar.a.kamble@intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_dma.c      | 11 +++++++----
> > >  drivers/gpu/drm/i915/i915_drv.h      |  3 ++-
> > >  drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++--------------
> > >  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
> > >  drivers/gpu/drm/i915/intel_pm.c      |  2 +-
> > >  5 files changed, 25 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > index 9232fdf..4a3ef34 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -1898,13 +1898,16 @@ void i915_driver_lastclose(struct drm_device * dev)
> > >  	if (!dev_priv)
> > >  		return;
> > >  
> > > -	if (dev_priv->rotation_property) {
> > > +	if (dev_priv->plane_rotation_property) {
> > >  		list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
> > > -			crtc->rotation = BIT(DRM_ROTATE_0);
> > > +			crtc->primary_rotation = BIT(DRM_ROTATE_0);
> > >  			drm_object_property_set_value(&crtc->base.base,
> > > -						dev_priv->rotation_property,
> > > -						crtc->rotation);
> > > +						dev_priv->plane_rotation_property,
> > > +						crtc->primary_rotation);
> > >  		}
> > > +	}
> > > +
> > > +	if (dev_priv->rotation_property) {
> > >  		list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) {
> > >  			plane->rotation = BIT(DRM_ROTATE_0);
> > >  			drm_object_property_set_value(&plane->base.base,
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 5a46788..6af78ee 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1560,7 +1560,8 @@ typedef struct drm_i915_private {
> > >  
> > >  	struct drm_property *broadcast_rgb_property;
> > >  	struct drm_property *force_audio_property;
> > > -	struct drm_property *rotation_property;
> > > +	struct drm_property *rotation_property; /* "rotation" */
> > > +	struct drm_property *plane_rotation_property; /* "plane-rotation" */
> > >  
> > >  	uint32_t hw_context_size;
> > >  	struct list_head context_list;
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index bab17fd..37b23d1 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2133,7 +2133,7 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> > >  		intel_crtc->dspaddr_offset = linear_offset;
> > >  	}
> > >  
> > > -	if (intel_crtc->rotation == BIT(DRM_ROTATE_180)) {
> > > +	if (intel_crtc->primary_rotation == BIT(DRM_ROTATE_180)) {
> > >  		dspcntr |= DISPPLANE_ROTATE_180;
> > >  
> > >  		x += (intel_crtc->config.pipe_src_w - 1);
> > > @@ -2238,7 +2238,7 @@ static int ironlake_update_plane(struct drm_crtc *crtc,
> > >  					       fb->pitches[0]);
> > >  	linear_offset -= intel_crtc->dspaddr_offset;
> > >  
> > > -	if (intel_crtc->rotation == BIT(DRM_ROTATE_180)) {
> > > +	if (intel_crtc->primary_rotation == BIT(DRM_ROTATE_180)) {
> > >  		dspcntr |= DISPPLANE_ROTATE_180;
> > >  
> > >  		if (!IS_HASWELL(dev) && !IS_BROADWELL(dev)) {
> > > @@ -8820,13 +8820,13 @@ static int intel_crtc_set_property(struct drm_crtc *crtc,
> > >  	uint64_t old_val;
> > >  	int ret = -ENOENT;
> > >  
> > > -	if (prop == dev_priv->rotation_property) {
> > > +	if (prop == dev_priv->plane_rotation_property) {
> > >  		/* exactly one rotation angle please */
> > >  		if (hweight32(val & 0xf) != 1)
> > >  			return -EINVAL;
> > >  
> > > -		old_val = intel_crtc->rotation;
> > > -		intel_crtc->rotation = val;
> > > +		old_val = intel_crtc->primary_rotation;
> > > +		intel_crtc->primary_rotation = val;
> > >  
> > >  		if (intel_crtc->active) {
> > >  			intel_crtc_wait_for_pending_flips(crtc);
> > > @@ -8834,12 +8834,12 @@ static int intel_crtc_set_property(struct drm_crtc *crtc,
> > >  			/* FBC does not work on some platforms for rotated planes */
> > >  			if (dev_priv->fbc.plane == intel_crtc->plane &&
> > >  			    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> > > -			    intel_crtc->rotation != BIT(DRM_ROTATE_0))
> > > +			    intel_crtc->primary_rotation != BIT(DRM_ROTATE_0))
> > >  				intel_disable_fbc(dev);
> > >  
> > >  			ret = dev_priv->display.update_plane(crtc, crtc->fb, 0, 0);
> > >  			if (ret)
> > > -				intel_crtc->rotation = old_val;
> > > +				intel_crtc->primary_rotation = old_val;
> > >  		}
> > >  	}
> > >  
> > > @@ -10387,7 +10387,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> > >  	 */
> > >  	intel_crtc->pipe = pipe;
> > >  	intel_crtc->plane = pipe;
> > > -	intel_crtc->rotation = BIT(DRM_ROTATE_0);
> > > +	intel_crtc->primary_rotation = BIT(DRM_ROTATE_0);
> > >  	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) {
> > >  		DRM_DEBUG_KMS("swapping pipes & planes for FBC\n");
> > >  		intel_crtc->plane = !pipe;
> > > @@ -10399,15 +10399,15 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> > >  	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
> > >  
> > >  	if (INTEL_INFO(dev)->gen >= 4) {
> > > -		if (!dev_priv->rotation_property)
> > > -			dev_priv->rotation_property =
> > > -				drm_mode_create_rotation_property(dev, "rotation",
> > > +		if (!dev_priv->plane_rotation_property)
> > > +			dev_priv->plane_rotation_property =
> > > +				drm_mode_create_rotation_property(dev, "plane-rotation",
> > >  								BIT(DRM_ROTATE_0) |
> > >  								BIT(DRM_ROTATE_180));
> > > -		if (dev_priv->rotation_property)
> > > +		if (dev_priv->plane_rotation_property)
> > >  			drm_object_attach_property(&intel_crtc->base.base,
> > > -						dev_priv->rotation_property,
> > > -						intel_crtc->rotation);
> > > +						dev_priv->plane_rotation_property,
> > > +						intel_crtc->primary_rotation);
> > >  	}
> > >  
> > >  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 82f0f9a..8c17a82 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -331,7 +331,7 @@ struct intel_crtc {
> > >  	struct drm_crtc base;
> > >  	enum pipe pipe;
> > >  	enum plane plane;
> > > -	unsigned int rotation;
> > > +	unsigned int primary_rotation; /* primary plane in relation to the pipe */
> > >  
> > >  	u8 lut_r[256], lut_g[256], lut_b[256];
> > >  	/*
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 9c9ddfe..5ebeb78 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -558,7 +558,7 @@ void intel_update_fbc(struct drm_device *dev)
> > >  	}
> > >  
> > >  	if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> > > -	    intel_crtc->rotation != BIT(DRM_ROTATE_0)) {
> > > +	    intel_crtc->primary_rotation != BIT(DRM_ROTATE_0)) {
> > >  		if (set_no_fbc_reason(dev_priv, FBC_UNSUPPORTED_MODE))
> > >  			DRM_DEBUG_KMS("mode incompatible with compression, "
> > >  				      "disabling\n");
> > 
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC