On 02/29/2016 09:10 PM, Dan Carpenter wrote:
> Hello Mario Kleiner,
>
> The patch e1d09dc0ccc6: "drm/amdgpu: Don't hang in
> amdgpu_flip_work_func on disabled crtc." from Feb 19, 2016, leads to
> the following static checker warning:
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_display.c:127 amdgpu_flip_work_func()
> warn: should this be 'repcnt == -1'
> drivers/gpu/drm/amd/amdgpu/amdgpu_display.c:136 amdgpu_flip_work_func()
> error: double unlock 'spin_lock:&crtc->dev->event_lock'
> drivers/gpu/drm/amd/amdgpu/amdgpu_display.c:136 amdgpu_flip_work_func()
> error: double unlock 'irqsave:flags'
>
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> 64 static void amdgpu_flip_work_func(struct work_struct *__work)
> 65 {
> 66 struct amdgpu_flip_work *work =
> 67 container_of(__work, struct amdgpu_flip_work,
> flip_work);
> 68 struct amdgpu_device *adev = work->adev;
> 69 struct amdgpu_crtc *amdgpuCrtc =
> adev->mode_info.crtcs[work->crtc_id];
> 70
> 71 struct drm_crtc *crtc = &amdgpuCrtc->base;
> 72 unsigned long flags;
> 73 unsigned i, repcnt = 4;
> 74 int vpos, hpos, stat, min_udelay = 0;
> 75 struct drm_vblank_crtc *vblank =
> &crtc->dev->vblank[work->crtc_id];
> 76
> 77 if (amdgpu_flip_handle_fence(work, &work->excl))
> 78 return;
> 79
> 80 for (i = 0; i < work->shared_count; ++i)
> 81 if (amdgpu_flip_handle_fence(work, &work->shared[i]))
> 82 return;
> 83
> 84 /* We borrow the event spin lock for protecting flip_status
> */
> 85 spin_lock_irqsave(&crtc->dev->event_lock, flags);
> 86
> 87 /* If this happens to execute within the "virtually
> extended" vblank
> 88 * interval before the start of the real vblank interval
> then it needs
> 89 * to delay programming the mmio flip until the real vblank
> is entered.
> 90 * This prevents completing a flip too early due to the way
> we fudge
> 91 * our vblank counter and vblank timestamps in order to work
> around the
> 92 * problem that the hw fires vblank interrupts before actual
> start of
> 93 * vblank (when line buffer refilling is done for a frame).
> It
> 94 * complements the fudging logic in
> amdgpu_get_crtc_scanoutpos() for
> 95 * timestamping and amdgpu_get_vblank_counter_kms() for
> vblank counts.
> 96 *
> 97 * In practice this won't execute very often unless on very
> fast
> 98 * machines because the time window for this to happen is
> very small.
> 99 */
> 100 while (amdgpuCrtc->enabled && repcnt--) {
> ^^^^^^^^
> Exists the loop with spin_lock held and repcnt == -1.
>
>
> 101 /* GET_DISTANCE_TO_VBLANKSTART returns distance to
> real vblank
> 102 * start in hpos, and to the "fudged earlier" vblank
> start in
> 103 * vpos.
> 104 */
> 105 stat = amdgpu_get_crtc_scanoutpos(adev->ddev,
> work->crtc_id,
> 106
> GET_DISTANCE_TO_VBLANKSTART,
> 107 &vpos, &hpos,
> NULL, NULL,
> 108 &crtc->hwmode);
> 109
> 110 if ((stat & (DRM_SCANOUTPOS_VALID |
> DRM_SCANOUTPOS_ACCURATE)) !=
> 111 (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE)
> ||
> 112 !(vpos >= 0 && hpos <= 0))
> 113 break;
> 114
> 115 /* Sleep at least until estimated real start of hw
> vblank */
> 116 spin_unlock_irqrestore(&crtc->dev->event_lock,
> flags);
> 117 min_udelay = (-hpos + 1) * max(vblank->linedur_ns /
> 1000, 5);
> 118 if (min_udelay > vblank->framedur_ns / 2000) {
> 119 /* Don't wait ridiculously long - something
> is wrong */
> 120 repcnt = 0;
>
> Exit with spin_lock released and repcnt == 0.
>
> 121 break;
> 122 }
> 123 usleep_range(min_udelay, 2 * min_udelay);
> 124 spin_lock_irqsave(&crtc->dev->event_lock, flags);
> 125 };
> 126
> 127 if (!repcnt)
> ^^^^^^
> Assumes exit with zero.
>
> 128 DRM_DEBUG_DRIVER("Delay problem on crtc %d:
> min_udelay %d, "
> 129 "framedur %d, linedur %d, stat %d,
> vpos %d, "
> 130 "hpos %d\n", work->crtc_id,
> min_udelay,
> 131 vblank->framedur_ns / 1000,
> 132 vblank->linedur_ns / 1000, stat,
> vpos, hpos);
> 133
> 134 /* set the flip status */
> 135 amdgpuCrtc->pflip_status = AMDGPU_FLIP_SUBMITTED;
> 136 spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>
> Assumes lock held.
>
> 137
> 138 /* Do the flip (mmio) */
> 139 adev->mode_info.funcs->page_flip(adev, work->crtc_id,
> work->base);
> 140 }
>
> regards,
> dan carpenter
>
Errors during error handling, my favorite bugs. Thanks for spotting
this! Patches are out for review.
-mario
