[Intel-gfx] [PATCH 08/24] drm/i915: Do not add planes from intel_atomic_setup_scalers.

[Konduru, Chandra]

> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
> Sent: Wednesday, June 03, 2015 12:02 AM
> To: Konduru, Chandra; Roper, Matthew D
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 08/24] drm/i915: Do not add planes from
> intel_atomic_setup_scalers.
> 
> Op 03-06-15 om 03:52 schreef Konduru, Chandra:
> >
> >> -----Original Message-----
> >> From: Roper, Matthew D
> >> Sent: Tuesday, June 02, 2015 6:30 PM
> >> To: Maarten Lankhorst
> >> Cc: intel-gfx at lists.freedesktop.org; Konduru, Chandra
> >> Subject: Re: [Intel-gfx] [PATCH 08/24] drm/i915: Do not add planes from
> >> intel_atomic_setup_scalers.
> >>
> >> On Mon, Jun 01, 2015 at 03:27:11PM +0200, Maarten Lankhorst wrote:
> >>> This may postpone going to HQ mode until the plane is in the
> >>> drm_atomic_state if it's not using scaler 0, but it does allow moving
> >>> intel_atomic_setup_scalers to the crtc check function.
> >>>
> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/intel_atomic.c  | 41 ++++++++++++++++++-------------
> ---
> >> --
> >>>  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++---------
> >>>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> >>>  3 files changed, 39 insertions(+), 31 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> >> b/drivers/gpu/drm/i915/intel_atomic.c
> >>> index 1edd1651c045..a8202fa0daa8 100644
> >>> --- a/drivers/gpu/drm/i915/intel_atomic.c
> >>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> >>> @@ -100,14 +100,6 @@ int intel_atomic_check(struct drm_device *dev,
> >>>  	if (ret)
> >>>  		return ret;
> >>>
> >>> -	/*
> >>> -	 * FIXME: move to crtc atomic check function once this is
> >>> -	 * more atomic friendly.
> >>> -	 */
> >>> -	ret = intel_atomic_setup_scalers(dev, nuclear_crtc, crtc_state);
> >>> -	if (ret)
> >>> -		return ret;
> >>> -
> >>>  	return ret;
> >>>  }
> >>>
> >>> @@ -336,21 +328,10 @@ int intel_atomic_setup_scalers(struct drm_device
> >> *dev,
> >>>  			/* find the plane that set the bit as scaler_user */
> >>>  			plane = drm_state->planes[i];
> >>>
> >>> -			/*
> >>> -			 * to enable/disable hq mode, add planes that are using
> >> scaler
> >>> -			 * into this transaction
> >>> -			 */
> >>>  			if (!plane) {
> >>> -				struct drm_plane_state *state;
> >>> -				plane = drm_plane_from_index(dev, i);
> >>> -				state =
> >> drm_atomic_get_plane_state(drm_state, plane);
> >>> -				if (IS_ERR(state)) {
> >>> -					DRM_DEBUG_KMS("Failed to add
> >> [PLANE:%d] to drm_state\n",
> >>> -						plane->base.id);
> >>> -					return PTR_ERR(state);
> >>> -				}
> >>> +				DRM_DEBUG_KMS("Failed to find [PLANE:%d]
> >> in drm_state\n", plane->base.id);
> >>> +				continue;
> >>>  			}
> >>> -
> >>>  			intel_plane = to_intel_plane(plane);
> >>>
> >>>  			/* plane on different crtc cannot be a scaler user of this
> >> crtc */
> >>> @@ -396,6 +377,24 @@ int intel_atomic_setup_scalers(struct drm_device
> >> *dev,
> >>>  		}
> >>>  	}
> >>>
> >>> +	/* plane not part of mask must leave hq mode? */
> >>> +	if (num_scalers_need > 1 && scaler_state->scalers[0].in_use &&
> >>> +	    scaler_state->scalers[0].mode == PS_SCALER_MODE_HQ) {
> >>> +		scaler_state->scalers[0].mode = PS_SCALER_MODE_DYN;
> >>> +
> >>> +		intel_crtc->atomic.skl_update_scaler0 =
> >>> +			PS_SCALER_EN | PS_SCALER_MODE_DYN;
> >>> +	}
> >>> +
> >>> +	/* plane not part of mask can enter hq mode? */
> >>> +	if (num_scalers_need == 1 && scaler_state->scalers[0].in_use &&
> >>> +	    intel_crtc->pipe != PIPE_C && scaler_state->scalers[0].mode !=
> >> PS_SCALER_MODE_HQ) {
> >>> +		scaler_state->scalers[0].mode = PS_SCALER_MODE_HQ;
> >>> +
> >>> +		intel_crtc->atomic.skl_update_scaler0 =
> >>> +			PS_SCALER_EN | PS_SCALER_MODE_HQ;
> >>> +	}
> >>> +
> >> I don't have access to the hw spec at the moment; is scaler #0 the only
> >> one that can ever go into HQ mode?
> > Yes
> >
> >> If there isn't a hardware
> >> requirement about this, then it seems like we're missing the case where
> >> planes A and B get scalers 0 and 1.  Then plane A (and thus scaler 0) is
> >> disabled, which should allow scaler 1 to go into HQ mode.
> > In this case, scaler 0 to be allocated to plane B to operate in HQ mode.
> Is it really bad to keep it on scaler 1 for a while until the next time the plane is
> added?
> 
> >> I guess it's not immediately clear to me why we need to not pull the
> >> other planes into the transaction.  Is this just to avoid doing some
> >> extra work for a plane that hasn't changed, or does it cause a problem
> >> somehow?
> > Per atomic design, unchanged planes can be added to transaction.
> > And scaler implementation is using this design feature.
> > Not sure what the issue here, but we need this feature continue
> > to available.
> >
> Unchanged planes can be added, but this could pull in a primary plane, which
> would need
> to set atomic.wait_for_flips then. I can do that as special case when adding a
> plane if
> that's preferred.

Here primary plane can get added if that is the only plane using scaler which
isn't part of the transaction. But here addition of primary plane isn't adding
or changing its FB. So why it needs to set atomic.wait_for_flips?

> 
> ~Maarten