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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 14-04-16 om 03:58 schreef Matt Roper:
> 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).
If you don't have to, you don't want to grab the plane state. It's probably fine to do so temporarily, but a solution
like for ILK would be better eventually. :)
>> 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.
Just being part of the state is enough. intel_prepare_plane_fb will be called on those planes,
so with my async unpin patch series you need to set the flag or you may not have async unpins, resulting in errors much later on when the fb is freed.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx