On Fri, Dec 10, 2021 at 5:45 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> [ This bug is obviously 8 and a half years old now. It's weird that
> no one has reported it. But the bug seems very clear although the
> fix is probably difficult. -dan ]
>
> Hello Rob Clark,
>
> The patch c8afe684c95c: "drm/msm: basic KMS driver for snapdragon"
> from Jun 26, 2013, leads to the following Smatch static checker
> warning:
>
> drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c:372 update_cursor()
> warn: sleeping in atomic context
>
> drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
> 355 static void update_cursor(struct drm_crtc *crtc)
> 356 {
> 357 struct mdp4_crtc *mdp4_crtc = to_mdp4_crtc(crtc);
> 358 struct mdp4_kms *mdp4_kms = get_kms(crtc);
> 359 struct msm_kms *kms = &mdp4_kms->base.base;
> 360 enum mdp4_dma dma = mdp4_crtc->dma;
> 361 unsigned long flags;
> 362
> 363 spin_lock_irqsave(&mdp4_crtc->cursor.lock, flags);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Takes a spin lock.
>
> 364 if (mdp4_crtc->cursor.stale) {
> 365 struct drm_gem_object *next_bo = mdp4_crtc->cursor.next_bo;
> 366 struct drm_gem_object *prev_bo = mdp4_crtc->cursor.scanout_bo;
> 367 uint64_t iova = mdp4_crtc->cursor.next_iova;
> 368
> 369 if (next_bo) {
> 370 /* take a obj ref + iova ref when we start scanning out: */
> 371 drm_gem_object_get(next_bo);
> --> 372 msm_gem_get_and_pin_iova(next_bo, kms->aspace, &iova);
> ^^^^^^^^^^^^^^^^^^^^^^^^^
> The msm_gem_get_and_pin_iova() function takes a mutex so it will sleep.
Hmm, it's even worse.. this is called from an irq handler..
It looks like initially this called msm_gem_get_iova_locked() but that
changed in 0e08270a1f01 ("drm/msm: Separate locking of buffer
resources from struct_mutex")
mdp4_crtc_cursor_set() already pins the iova, so all we _really_ need
to do is take an extra reference until until scanout completes.. but
we maybe don't even need that as drm_flip_work should defer
unref/unpin until after vblank. So might not be too hard to fix.. the
harder thing, for me at least, might be resurrecting some hw to test
the fix on. The last thing I know of that used mdp4 was the SoC in
the nexus 4.
BR,
-R
> 373
> 374 /* enable cursor: */
> 375 mdp4_write(mdp4_kms, REG_MDP4_DMA_CURSOR_SIZE(dma),
> 376 MDP4_DMA_CURSOR_SIZE_WIDTH(mdp4_crtc->cursor.width) |
> 377 MDP4_DMA_CURSOR_SIZE_HEIGHT(mdp4_crtc->cursor.height));
> 378 mdp4_write(mdp4_kms, REG_MDP4_DMA_CURSOR_BASE(dma), iova);
> 379 mdp4_write(mdp4_kms, REG_MDP4_DMA_CURSOR_BLEND_CONFIG(dma),
> 380 MDP4_DMA_CURSOR_BLEND_CONFIG_FORMAT(CURSOR_ARGB) |
> 381 MDP4_DMA_CURSOR_BLEND_CONFIG_CURSOR_EN);
> 382 } else {
> 383 /* disable cursor: */
> 384 mdp4_write(mdp4_kms, REG_MDP4_DMA_CURSOR_BASE(dma),
> 385 mdp4_kms->blank_cursor_iova);
> 386 }
> 387
> 388 /* and drop the iova ref + obj rev when done scanning out: */
> 389 if (prev_bo)
> 390 drm_flip_work_queue(&mdp4_crtc->unref_cursor_work, prev_bo);
> 391
> 392 mdp4_crtc->cursor.scanout_bo = next_bo;
> 393 mdp4_crtc->cursor.stale = false;
> 394 }
> 395
> 396 mdp4_write(mdp4_kms, REG_MDP4_DMA_CURSOR_POS(dma),
> 397 MDP4_DMA_CURSOR_POS_X(mdp4_crtc->cursor.x) |
> 398 MDP4_DMA_CURSOR_POS_Y(mdp4_crtc->cursor.y));
> 399
> 400 spin_unlock_irqrestore(&mdp4_crtc->cursor.lock, flags);
> 401 }
>
> regards,
> dan carpenter
