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
