Re: [PATCH 07/16] drm/i915/gen9: Allow skl_allocate_pipe_ddb() to operate on in-flight state

[Matt Roper <matthew.d.roper@intel.com>]

On Tue, Apr 05, 2016 at 11:35:31AM +0200, Maarten Lankhorst wrote:
> Op 01-04-16 om 03:46 schreef Matt Roper:
> > We eventually want to calculate watermark values at atomic 'check' time
> > instead of atomic 'commit' time so that any requested configurations
> > that result in impossible watermark requirements are properly rejected.
> > The first step along this path is to allocate the DDB at atomic 'check'
> > time.  As we perform this transition, allow the main allocation function
> > to operate successfully on either an in-flight state or an
> > already-commited state.  Once we complete the transition in a future
> > patch, we'll come back and remove the unnecessary logic for the
> > already-committed case.
> >
> > Note one other minor change here...when working with the
> > already-committed state, we pull the active CRTC's from
> > hweight(dev_priv->active_crtcs) instead of
> > dev_priv->wm.config.num_pipes_active.  The values should be equivalent,
> > but dev_priv->wm.config is pretty much unused at this point and it would
> > be nice to eventually remove it entirely.
> > ---
...
> > @@ -3078,13 +3109,26 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> >  	total_data_rate = skl_get_total_relative_data_rate(cstate);
> >  
> >  	start = alloc->start;
> > -	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> > +	for_each_intel_plane_mask(dev, intel_plane, cstate->base.plane_mask) {
> >  		struct drm_plane *plane = &intel_plane->base;
> > -		struct drm_plane_state *pstate = intel_plane->base.state;
> > +		struct drm_plane_state *pstate;
> >  		unsigned int data_rate, y_data_rate;
> >  		uint16_t plane_blocks, y_plane_blocks = 0;
> >  		int id = skl_wm_plane_id(intel_plane);
> >  
> > +		/*
> > +		 * TODO: Remove support for already-committed state once we
> > +		 * only allocate DDB on in-flight states.
> > +		 */
> > +		if (cstate->base.state) {
> > +			pstate = drm_atomic_get_plane_state(cstate->base.state,
> > +							    plane);
> >
> Yuck again? :( No way around this by storing data rates for example?

Is there a reason to try to avoid grabbing the plane state here?  If we
get here, then we already have the CRTC lock, so we're not preventing
any potential for parallel updates of this plane (at least not on Intel
hardware where planes are tied to the CRTC).

> Oh well, at least set cstate->base.planes_changed please.

I'm not sure I follow this one.  We're using pstate in a read-only
manner so nothing we're doing here should necessitate programming planes
as far as I can see.


Matt


-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx