<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Mar 13, 2020 at 5:02 PM Michel Dänzer <<a href="mailto:michel@daenzer.net">michel@daenzer.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 2020-03-13 1:35 p.m., Kazlauskas, Nicholas wrote:<br>
> On 2020-03-12 10:32 a.m., Alex Deucher wrote:<br>
>> On Thu, Mar 5, 2020 at 4:21 PM Mario Kleiner<br>
>> <<a href="mailto:mario.kleiner.de@gmail.com" target="_blank">mario.kleiner.de@gmail.com</a>> wrote:<br>
>>><br>
>>> Commit '16f17eda8bad ("drm/amd/display: Send vblank and user<br>
>>> events at vsartup for DCN")' introduces a new way of pageflip<br>
>>> completion handling for DCN, and some trouble.<br>
>>><br>
>>> The current implementation introduces a race condition, which<br>
>>> can cause pageflip completion events to be sent out one vblank<br>
>>> too early, thereby confusing userspace and causing flicker:<br>
>>><br>
>>> prepare_flip_isr():<br>
>>><br>
>>> 1. Pageflip programming takes the ddev->event_lock.<br>
>>> 2. Sets acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED<br>
>>> 3. Releases ddev->event_lock.<br>
>>><br>
>>> --> Deadline for surface address regs double-buffering passes on<br>
>>>      target pipe.<br>
>>><br>
>>> 4. dc_commit_updates_for_stream() MMIO programs the new pageflip<br>
>>>     into hw, but too late for current vblank.<br>
>>><br>
>>> => pflip_status == AMDGPU_FLIP_SUBMITTED, but flip won't complete<br>
>>>     in current vblank due to missing the double-buffering deadline<br>
>>>     by a tiny bit.<br>
>>><br>
>>> 5. VSTARTUP trigger point in vblank is reached, VSTARTUP irq fires,<br>
>>>     dm_dcn_crtc_high_irq() gets called.<br>
>>><br>
>>> 6. Detects pflip_status == AMDGPU_FLIP_SUBMITTED and assumes the<br>
>>>     pageflip has been completed/will complete in this vblank and<br>
>>>     sends out pageflip completion event to userspace and resets<br>
>>>     pflip_status = AMDGPU_FLIP_NONE.<br>
>>><br>
>>> => Flip completion event sent out one vblank too early.<br>
>>><br>
>>> This behaviour has been observed during my testing with measurement<br>
>>> hardware a couple of time.<br>
>>><br>
>>> The commit message says that the extra flip event code was added to<br>
>>> dm_dcn_crtc_high_irq() to prevent missing to send out pageflip events<br>
>>> in case the pflip irq doesn't fire, because the "DCH HUBP" component<br>
>>> is clock gated and doesn't fire pflip irqs in that state. Also that<br>
>>> this clock gating may happen if no planes are active. According to<br>
>>> Nicholas, the clock gating can also happen if psr is active, and the<br>
>>> gating is controlled independently by the hardware, so difficult to<br>
>>> detect if and when the completion code in above commit is needed.<br>
>>><br>
>>> This patch tries the following solution: It only executes the extra<br>
>>> pflip<br>
>>> completion code in dm_dcn_crtc_high_irq() iff the hardware reports<br>
>>> that there aren't any surface updated pending in the double-buffered<br>
>>> surface scanout address registers. Otherwise it leaves pflip completion<br>
>>> to the pflip irq handler, for a more race-free experience.<br>
>>><br>
>>> This would only guard against the order of events mentioned above.<br>
>>> If Step 5 (VSTARTUP trigger) happens before step 4 then this won't help<br>
>>> at all, because 1-3 + 5 might happen even without the hw being<br>
>>> programmed<br>
>>> at all, ie. no surface update pending because none yet programmed<br>
>>> into hw.<br>
>>><br>
>>> Therefore this patch also changes locking in amdgpu_dm_commit_planes(),<br>
>>> so that prepare_flip_isr() and dc_commit_updates_for_stream() are done<br>
>>> under event_lock protection within the same critical section.<br>
>>><br>
>>> v2: Take Nicholas comments into account, try a different solution.<br>
>>><br>
>>> Lightly tested on Polaris (locking) and Raven (the whole DCN stuff).<br>
>>> Seems to work without causing obvious new trouble.<br>
>><br>
>> Nick, any comments on this?  Can we get this committed or do you think<br>
>> it needs additional rework?<br>
>><br>
>> Thanks,<br>
>><br>
>> Alex<br>
> <br>
> Hi Alex, Mario,<br>
> <br>
> This might be a little strange, but if we want to get this in as a fix<br>
> for regressions caused by the original vblank and user events at<br>
> vstartup patch then I'm actually going to give my reviewed by on the<br>
> *v1* of this patch (but not this v2):<br>
> <br>
> Reviewed-by: Nicholas Kazlauskas <<a href="mailto:nicholas.kazlauskas@amd.com" target="_blank">nicholas.kazlauskas@amd.com</a>><br>
> <br>
> You can feel free to apply that one.<br>
> <br>
> Reason 1: After having thought about it some more I don't think we<br>
> enable anything today that has hubp powered down at the same time we<br>
> expect to be waiting for a flip - eg. DMCU powering down HUBP during PSR<br>
> entry. Static screen interrupt should happen after that flip finishes I<br>
> think.<br>
> <br>
> The CRTC can still be powered on with zero planes, and I don't think any<br>
> userspace explicitly asks for vblank events in this case but it doesn't<br>
> hurt to have the check.<br>
><br></blockquote><div><br></div><div>Ok, so the original commit that causes the races currently solves a non-existing - but potential future - problem?<br></div><div> </div><div>I guess then my v1 patch makes sense for the moment and fixes the immediate problem for Linux 5.6-rc.<br></div><div><br></div><div>Maybe there's a way to ask the hw if hubp is clock-gated and so if that code needs to run or not in the future?</div><div><br></div><div>As mentioned before, it would be helpful at least for me to get a better overview about which hw events happen when in vblank, which irq's fire in response etc., how this relates to things like power management actions, psr, vrr / drr, etc. A lot has changed in the hw during the last 10 years, and my guideline are still the public pdf files with the DCE register specs for DCE-1'ish hw from sometime around the year 2007.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> Reason 2: This new patch will need much more thorough testing from side<br>
> to fully understand the consequences of locking the entire DC commit<br>
> sequence. For just a page flip that sounds fine, but for anything more<br>
> than (eg. full updates, modesets, etc) I don't think we want to be<br>
> disabling interrupts for potentially many milliseconds.<br>
<br>
Ah! I was wondering where the attached splat comes from, but I think<br>
this explains it: With this patch amdgpu_dm_commit_planes keeps the<br>
pcrtc->dev->event_lock spinlock locked while calling<br>
dc_commit_updates_for_stream, which ends up calling<br>
smu_set_display_count, which tries to lock a mutex.<br>
<br></blockquote><div><br></div><div>Yep, sorry about that. My most modern machine has Raven Ridge / DCN1, and that function only gets called on Navi /DCN2 afaics. All my testing is limited to Polaris / DCE11 and Raven DCN1, my most modern hw atm.<br></div><div><br></div><div>thanks,<br></div><div>-mario</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
-- <br>
Earthling Michel Dänzer               |               <a href="https://redhat.com" rel="noreferrer" target="_blank">https://redhat.com</a><br>
Libre software enthusiast             |             Mesa and X developer<br>
</blockquote></div></div>