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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
