[Intel-gfx] [PATCH 13/14] drm/i915: Workaround VLV/CHV sprite1->sprite0 enable underrun

[Maarten Lankhorst]

Op 15-12-16 om 16:47 schreef Ville Syrjälä:
> On Thu, Dec 15, 2016 at 04:34:58PM +0100, Maarten Lankhorst wrote:
>> Op 12-12-16 om 21:35 schreef ville.syrjala at linux.intel.com:
>>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>
>>> On VLV/CHV enabling sprite0 when sprite1 has already been enabled may
>>> lead to an underrun. This only happens when sprite0 FIFO size is zero
>>> prior to enabling it. Hence an effective workaround is to always
>>> allocate at least one cacheline for sprite0 when sprite1 is active.
>>>
>>> I've not observed this sort of failure during any other type of plane
>>> enable/disable sequence.
>>>
>>> Testcase: igt/kms_plane_blinker
>>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_pm.c | 24 +++++++++++++++++++++++-
>>>  1 file changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>> index fa882cdcac52..75a5bde43723 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -1006,6 +1006,12 @@ static uint16_t vlv_compute_wm_level(const struct intel_crtc_state *crtc_state,
>>>  	return min_t(int, wm, USHRT_MAX);
>>>  }
>>>  
>>> +static bool vlv_need_sprite0_fifo_workaround(unsigned int active_planes)
>>> +{
>>> +	return (active_planes & (BIT(PLANE_SPRITE0) |
>>> +				 BIT(PLANE_SPRITE1))) == BIT(PLANE_SPRITE1);
>>> +}
>>> +
>>>  static int vlv_compute_fifo(struct intel_crtc_state *crtc_state)
>>>  {
>>>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>> @@ -1016,12 +1022,25 @@ static int vlv_compute_fifo(struct intel_crtc_state *crtc_state)
>>>  	int num_active_planes = hweight32(active_planes);
>>>  	const int fifo_size = 511;
>>>  	int fifo_extra, fifo_left = fifo_size;
>>> +	int sprite0_fifo_extra = 0;
>>>  	unsigned int total_rate;
>>>  	enum plane_id plane_id;
>>>  
>>> +	/*
>>> +	 * When enabling sprite0 after sprite1 has already been enabled
>>> +	 * we tend to get an underrun unless sprite0 already has some
>>> +	 * FIFO space allcoated. Hence we always allocate at least one
>>> +	 * cacheline for sprite0 whenever sprite1 is enabled.
>>> +	 *
>>> +	 * All other plane enable sequences appear immune to this problem.
>>> +	 */
>>> +	if (vlv_need_sprite0_fifo_workaround(active_planes))
>>> +		sprite0_fifo_extra = 1;
>> I don't think you need crtc_state->active_planes just for this, it adds a lot of tracking for something that could be done by
>>
>> if (noninverted->plane[SPRITE1] && !noninverted->plane[SPRITE0])
>> /* allocate 1 for sprite 0 */
>>
>> Maybe drop that patch?
> We'll want it for other things outside of the vlv watermark code as
> well. So figured this is a good excuse for getting the ball rolling,
I haven't seen a good reason yet, at least not one that requires visible_mask = BIT(plane->id)

~Maarten