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

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

Hey,

Op 04-06-15 om 01:33 schreef Matt Roper:
> On Wed, Jun 03, 2015 at 12:32:43PM -0700, Konduru, Chandra wrote:
>>
>>> -----Original Message-----
>>> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
>>> Sent: Wednesday, June 03, 2015 12:02 AM
>>> To: Konduru, Chandra; Roper, Matthew D
>>> Cc: intel-gfx@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@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@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?
> In the atomic case, intel_check_primary_plane() sets
> atomic.wait_for_flips if the primary plane is part of the original
> transaction (no matter what about the primary plane is changing).
> However if we pull in the primary plane via
> intel_atomic_setup_scalers(), that gets called after we've finished
> checking all of the planes (that were originally part of the
> transaction), so I don't think wait_for_flips will get set in that case.
> So I think pulling in the plane as Chandra was will still avoid an
> unnecessary wait.  Even though it's in the transaction, I believe the
> 'check' step is bypassed (which means we need to be careful about doing
> stuff like this if it could affect derived state...but I think in this
> case it's safe).
>
> N.B.  The wait we're talking about here will only be triggered when a
> SKL or BXT platform disables a "sprite" plane (thus triggering the
> primary's scaler to switch to HQ mode) while there's a pending legacy
> pageflip on the primary plane.  I don't think we have a userspace today
> that can trigger this...Weston won't use sprites at all in a non-atomic
> manner and although SNA has an Xv sprite adapter, it won't ever be
> performing scaling of the primary plane afaik.
Correct. The wait will be a nop if there are no legacy page flips. But for completeness sake I included it. :-)

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