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

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

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.

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