Re: drm/amdgpu: Don't hang in amdgpu_flip_work_func on disabled crtc.

[Mario Kleiner <mario.kleiner.de@gmail.com>]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel