On Fri, 10 Dec 2021 at 19:51, Rob Clark <robdclark@xxxxxxxxx> wrote:
>
> 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.
I have nexus 7 here, so if you provide patches, I can test them.
Noting myself to acquire ifc6410.
>
> 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
--
With best wishes
Dmitry
