On 2018-07-05 12:09 PM, Andrey Grodzovsky wrote:
On 07/05/2018 12:06 PM, Greg KH wrote:
On Thu, Jul 05, 2018 at 12:05:09PM -0400, Andrey Grodzovsky wrote:
This patch is wrong as noted by MIchel a while ago - quote from his review of the patch.
"Actually, pflip_status should really only be set to AMDGPU_FLIP_SUBMITTED after the flip has been programmed to the hardware, at least as far as the lock holder is concerned. That's why the code was previously holding the lock until after the dc_commit_updates_for_stream call. Otherwise, it's at least theoretically possible that either:
- dm_pflip_high_irq is called before dc_commit_updates_for_stream, but sees
flip_status == AMDGPU_FLIP_SUBMITTED and sends the event to userspace prematurely
- dm_pflip_high_irq is called after dc_commit_updates_for_stream, but sees
flip_status != AMDGPU_FLIP_SUBMITTED, so it incorrectly doesn't send the event to userspace "
It shouldn't go in.
Is there a fix for this in Linus's tree for these problems? If not, why not? If so, what is that git commit id?
AFAIK there is still no fix for the original issue which this patch was intended to fix.
Harry, can you please comment on this ?
That can be better answered by Shirish.
In my opinion the issue described by Michel is a lesser evil than attempting to allocate memory inside a spinlocked area.
Shirish, we need to understand why dc_commit_updates_for_stream() is trying to do a UPDATE_TYPE_FULL in your scenario. This should never really happen through the amdgpu_dm_do_flip code path and we need to understand why. If we never do UPDATE_TYPE_FULL here we should not need to allocate memory and should be find with leaving the spinlock around dc_commit_updates_for_stream().
Harry
Andrey
thanks,
greg k-h