[Intel-gfx] [PATCH 02/19] drm/i915: Remove stallcheck special handling, v2.

[Maarten Lankhorst]

Op 27-04-16 om 15:24 schreef Patrik Jakobsson:
> On Tue, Apr 19, 2016 at 09:52:22AM +0200, Maarten Lankhorst wrote:
>> Both intel_unpin_work.pending and intel_unpin_work.enable_stall_check
>> were used to see if work should be enabled. By only using pending
>> some special cases are gone, and access to unpin_work can be simplified.
>>
>> Use this to only access work members untilintel_mark_page_flip_active
>> is called, or intel_queue_mmio_flip is used. This will prevent
>> use-after-free, and makes it easier to verify accesses.
>>
>> Changes since v1:
>> - Reword commit message.
>> - Do not access unpin_work after intel_mark_page_flip_active.
>> - Add the right memory barriers.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c  | 11 +++---
>>  drivers/gpu/drm/i915/intel_display.c | 71 ++++++++++++++----------------------
>>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>>  3 files changed, 34 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 931dc6086f3b..0092aaf47c43 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -612,9 +612,14 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data)
>>  			seq_printf(m, "No flip due on pipe %c (plane %c)\n",
>>  				   pipe, plane);
>>  		} else {
>> +			u32 pending;
>>  			u32 addr;
>>  
>> -			if (atomic_read(&work->pending) < INTEL_FLIP_COMPLETE) {
>> +			pending = atomic_read(&work->pending);
>> +			if (pending == INTEL_FLIP_INACTIVE) {
>> +				seq_printf(m, "Flip ioctl preparing on pipe %c (plane %c)\n",
>> +					   pipe, plane);
>> +			} else if (pending >= INTEL_FLIP_COMPLETE) {
>>  				seq_printf(m, "Flip queued on pipe %c (plane %c)\n",
>>  					   pipe, plane);
>>  			} else {
>> @@ -636,10 +641,6 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data)
>>  				   work->flip_queued_vblank,
>>  				   work->flip_ready_vblank,
>>  				   drm_crtc_vblank_count(&crtc->base));
>> -			if (work->enable_stall_check)
>> -				seq_puts(m, "Stall check enabled, ");
>> -			else
>> -				seq_puts(m, "Stall check waiting for page flip ioctl, ");
>>  			seq_printf(m, "%d prepares\n", atomic_read(&work->pending));
>>  
>>  			if (INTEL_INFO(dev)->gen >= 4)
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 4cb830e2a62e..97a8418f6539 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3896,8 +3896,6 @@ static void page_flip_completed(struct intel_crtc *intel_crtc)
>>  	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
>>  	struct intel_unpin_work *work = intel_crtc->unpin_work;
>>  
>> -	/* ensure that the unpin work is consistent wrt ->pending. */
>> -	smp_rmb();
>>  	intel_crtc->unpin_work = NULL;
>>  
>>  	if (work->event)
>> @@ -10980,16 +10978,13 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
>>  	spin_lock_irqsave(&dev->event_lock, flags);
>>  	work = intel_crtc->unpin_work;
>>  
>> -	/* Ensure we don't miss a work->pending update ... */
>> -	smp_rmb();
>> +	if (work && atomic_read(&work->pending) >= INTEL_FLIP_COMPLETE) {
>> +		/* ensure that the unpin work is consistent wrt ->pending. */
>> +		smp_mb__after_atomic();
> The docs on smp_mb__after/before_atomic() states that they are used with atomic
> functions that do not return a value. Why are we using it together with
> atomic_read() here?
>From Documentation/atomic_ops.txt:

*** WARNING: atomic_read() and atomic_set() DO NOT IMPLY BARRIERS! ***

Plus a whole warning below how the atomic ops may be reordered. The memory
barriers are definitely required.
>>  static void intel_mmio_flip_work_func(struct work_struct *work)
>> @@ -11529,15 +11517,14 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev,
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>  	struct intel_unpin_work *work = intel_crtc->unpin_work;
>>  	u32 addr;
>> +	u32 pending;
>>  
>> -	if (atomic_read(&work->pending) >= INTEL_FLIP_COMPLETE)
>> -		return true;
>> -
>> -	if (atomic_read(&work->pending) < INTEL_FLIP_PENDING)
>> -		return false;
>> +	pending = atomic_read(&work->pending);
>> +	/* ensure that the unpin work is consistent wrt ->pending. */
>> +	smp_mb__after_atomic();
> Why paired with atomic_read()?
See above. ^
>>  
>> -	if (!work->enable_stall_check)
>> -		return false;
>> +	if (pending != INTEL_FLIP_PENDING)
>> +		return pending == INTEL_FLIP_COMPLETE;
> Am I correct in assuming that we can remove the enable_stall_check test here
> since it's always enabled? If so, that would be useful to explain in the commit
> message.
The commit message says stallcheck special handling is removed entirely. I thought it would
imply that the special case, where a flip may be queued but stallcheck not yet active, is removed entirely.

~Maarten