[PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[Noralf Trønnes]

Den 22.03.2018 19.49, skrev Ville Syrjälä:
> On Thu, Mar 22, 2018 at 05:51:35PM +0100, Noralf Trønnes wrote:
>> tinydrm is also using plane->fb:
>>
>> $ grep -r "plane\.fb" drivers/gpu/drm/tinydrm/
>> drivers/gpu/drm/tinydrm/repaper.c:      if (tdev->pipe.plane.fb != fb)
>> drivers/gpu/drm/tinydrm/mipi-dbi.c:     if (tdev->pipe.plane.fb != fb)
>> drivers/gpu/drm/tinydrm/mipi-dbi.c:     struct drm_framebuffer *fb =
>> mipi->tinydrm.pipe.plane.fb;
> Oh dear, and naturally it's the most annoying one of the bunch. So we
> want to take the plane lock in the fb.dirty() hook to look at the fb,
> but mipi-dbi.c calls that directly from the bowels of its
> .atomic_enable() hook. So that would deadlock pretty neatly if the
> commit happens while already holding the lock.
>
> So we'd either need to plumb an acquire context into fb.dirty(),
> or maybe have tinydrm provide a lower level lockless dirty() hook
> that gets called by both (there are just too many layers in this
> particular cake to immediately see where such a hook would be best
> placed). Or maybe there's some other solution I didn't think of.
>
> Looking at this I'm also left wondering how this is supposed
> handle fb.dirty() getting called concurrently with a modeset.
> The dirty_mutex only seems to offer any protection for
> fb.dirty() against another fb.dirty()...
>

I have been waiting for the 'page-flip with damage'[1] work to come to
fruition so I could handle all flushing during atomic commit.
The idea is to use the same damage rect for a generic dirtyfb callback.

Maybe a temporary tinydrm specific solution is possible until that work
lands, but I don't know enough about how to implement such a dirtyfb
callback.

I imagine it could look something like this:

  struct tinydrm_device {
+    struct drm_clip_rect dirtyfb_rect;
  };

static int tinydrm_fb_dirty(struct drm_framebuffer *fb,
                 struct drm_file *file_priv,
                 unsigned int flags, unsigned int color,
                 struct drm_clip_rect *clips,
                 unsigned int num_clips)
{
     struct tinydrm_device *tdev = fb->dev->dev_private;
     struct drm_framebuffer *planefb;

     /* Grab whatever lock needed to check this */
     planefb = tdev->pipe.plane.state->fb;

     /* fbdev can flush even when we're not interested */
     if (fb != planefb)
         return 0;

     /* Protect dirtyfb_rect with a lock */
     tinydrm_merge_clips(&tdev->dirty_rectfb, clips, num_clips, flags,
                 fb->width, fb->height);

     /*
      * Somehow do an atomic commit that results in the atomic update
      * callback being called
      */
     ret = drm_atomic_commit(state);
...
}

static void mipi_dbi_flush(struct drm_framebuffer *fb,
                struct drm_clip_rect *clip)
{
     /* Flush out framebuffer */
}

void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
               struct drm_plane_state *old_state)
{
     struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
     struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
     struct drm_framebuffer *fb = pipe->plane.state->fb;
     struct drm_crtc *crtc = &tdev->pipe.crtc;

     if (!fb || (fb == old_state->fb && !tdev->dirty_rect.x2))
         goto out_vblank;

     /* Don't flush if the controller isn't initialized yet */
     if (!mipi->enabled)
         goto out_vblank;

     if (fb != old_state->fb) { /* Page flip or new, flush all */
         mipi_dbi_flush(fb, NULL);
     } else { /* dirtyfb ioctl */
         mipi_dbi_flush(fb, &tdev->dirtyfb_rect);
         memset(&tdev->dirtyfb_rect, 0, sizeof(tdev->dirtyfb_rect));
     }

out_vblank:
     if (crtc->state->event) {
         spin_lock_irq(&crtc->dev->event_lock);
         drm_crtc_send_vblank_event(crtc, crtc->state->event);
         spin_unlock_irq(&crtc->dev->event_lock);
         crtc->state->event = NULL;
     }
}

This is called from the driver pipe enable callback after the controller
has been initialized:

  void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe,
                struct drm_crtc_state *crtc_state)
  {
      struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
      struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
-     struct drm_framebuffer *fb = pipe->plane.fb;
+    struct drm_framebuffer *fb = pipe->plane.state->fb;

      DRM_DEBUG_KMS("\n");

      mipi->enabled = true;
-     if (fb)
-         fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
+    mipi_dbi_flush(fb, NULL);
      tinydrm_enable_backlight(mipi->backlight);
  }

I can make patches if this makes any sense and you can help me with the
dirtyfb implementation.

Noralf.

[1] 
https://lists.freedesktop.org/archives/dri-devel/2017-December/161003.html