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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Op 03-05-16 om 15:48 schreef Patrik Jakobsson:
> On Thu, Apr 28, 2016 at 12:20:09PM +0200, Maarten Lankhorst wrote:
>> Op 28-04-16 om 11:54 schreef Patrik Jakobsson:
>>> On Thu, Apr 28, 2016 at 10:48:55AM +0200, Maarten Lankhorst wrote:
>>>> 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@xxxxxxxxxxxxxxx>
>>>>>> ---
>>>>>>  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.
>>> Yes, the barriers are required. My point is that _after/before_atomic() should
>>> only be used with set/clear/inc etc atomic operations. For atomic operations
>>> that return a value you should use other macros. At least that is how I
>>> interpret the documentation.
>>>
>>> Here's the part from Documentation/atomic_ops.txt:
>>>
>>> --
>>>
>>> If a caller requires memory barrier semantics around an atomic_t
>>> operation which does not return a value, a set of interfaces are
>>> defined which accomplish this:
>>>
>>> 	void smp_mb__before_atomic(void);
>>> 	void smp_mb__after_atomic(void);
>>>
>>> --
>>>
>>> So I interpret this as, there's no guarantee that you'll get a full memory
>>> barrier from these macros.
>>>
> Did you find an issue with this or is the current usage correct?
Needs smp_mb instead of *_after_atomic, thanks for catching!

The before_atomic ones are correct, afaict.
>>>>>>  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
>>> The commit message tells what the patch does but not why. This might be obvious
>>> if you're familiar with the code. I stumbled a bit here so I guess I'm not :)
>>>
>>> "Both intel_unpin_work.pending and intel_unpin_work.enable_stall_check were used
>>>  to see if work should be enabled"
>>>
>>> From this I imply that both checks are needed.
>>>
>>> "By only using pending some special cases are gone"
>>>
>>> This is what I don't find intuitive. Why can we suddently skip the
>>> enable_stall_check test? It would have been useful to know about the special
>>> case where a flip may be queued but stallcheck not yet active, and that it's no
>>> longer valid (and possibly why).
>> It looks like the pending member was added later to fix a race. It made enable_stall_check obsolete,
>> but I'm not 100% sure that this was the reason.
>>
>> In any case for flips we set pending right before queueing, which eliminates the race.
> Ok, can we add something like "A flip could previously be queued before
> stallcheck was active. With the addition of the pending member
> enable_stall_check became obsolete and can thus be removed. Pending is also set
> before queuing which eliminates the potential race"? Feel free to rephrase it.
>
> It's a bit verbose but I prefer that over misinterpreting the patch.
Ok.

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux