[PATCH/RFC] drm: omapdrm: Support unlinking page flip events prematurely
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Apr 16 13:50:12 PDT 2015
DRM page flip vblank events requested by page flips or atomic commits
are created by the DRM core and then passed to driver through CRTC
states (for atomic commit) or directly to the page flip handler (for
legacy page flips). The events are then kept aside until the page flip
completes, at which point drivers queue them for delivery with a call to
drm_send_vblank_event().
When a DRM file handle is closed events pending for delivery are cleaned
up automatically by the DRM core. Events that have been passed to the
driver but haven't completed yet, however, are not handled by the DRM
core. Drivers are responsible for destroying them and must not attempt
to queue them for delivery. This is usually handled by drivers'
preclose() handlers that cancel and destroy page flip events that
reference the file handle being closed.
With asynchronous atomic updates the story becomes more complex. Several
asynchronous atomic updates can be pending, each of them carrying
per-CRTC events. As the atomic_commit() operation doesn't receive a file
handle context, drivers can't know which file handle a pending update
refers to, making it difficult to cancel or wait for completion of
updates related to the file handle being closed.
It should be noted that cancelling page flips or waiting for atomic
updates completion isn't required by the DRM core when closing a file
handle. The only requirement is that no event gets queued for delivery
after the preclose() operation returns. This can easily be achieved by
storing events for atomic commits in a list, unlinking events from the
file handle being closed by setting the file_priv field to NULL, and
skipping delivery of unlinked events.
This logic replaces the page flip cancellation completely.
Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
---
drivers/gpu/drm/omapdrm/omap_crtc.c | 36 +++++++++++-------------------------
drivers/gpu/drm/omapdrm/omap_drv.c | 30 +++++++++++++++++++++++++++---
drivers/gpu/drm/omapdrm/omap_drv.h | 2 +-
3 files changed, 39 insertions(+), 29 deletions(-)
Hello,
This patch applies on top of the "[PATCH 00/31] Convert omapdrm to the atomic
updates API" series and fixes the WARN_ON() triggered in drm_release().
The implementation is cleaner than I expected, and code should be easy to move
to the DRM core. It gets rid of the current page flip cancel implementation
and replaces it with more generic code, opening a way to implement most of the
currently driver-specific preclose() code in the core.
I still have a slightly uneasy feeling about keeping around objects such as
events (and actually commits too) after the related file handle has been
closed, but I can't really pinpoint an objective reason why this would be a
problem. Races with unload() come to mind, I haven't checked that yet, but I
don't think this patch makes that specific problem worse anyway.
This patch uses the drm_pending_event link field to store the event in the
driver's private list. The field is documented in DocBook as being available
to drivers for internal use, until passing the even to the core for delivery
to userspace. It thus shouldn't be an issue according to me, but please let me
know if you disagree.
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 4472862..0864c28 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -255,30 +255,6 @@ static const struct dss_mgr_ops mgr_ops = {
* Setup, Flush and Page Flip
*/
-void omap_crtc_cancel_page_flip(struct drm_crtc *crtc, struct drm_file *file)
-{
- struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
- struct drm_pending_vblank_event *event;
- struct drm_device *dev = crtc->dev;
- unsigned long flags;
-
- /* Destroy the pending vertical blanking event associated with the
- * pending page flip, if any, and disable vertical blanking interrupts.
- */
-
- spin_lock_irqsave(&dev->event_lock, flags);
-
- event = omap_crtc->event;
- omap_crtc->event = NULL;
-
- if (event && event->base.file_priv == file) {
- event->base.destroy(&event->base);
- drm_crtc_vblank_put(crtc);
- }
-
- spin_unlock_irqrestore(&dev->event_lock, flags);
-}
-
static void omap_crtc_complete_page_flip(struct drm_crtc *crtc)
{
struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
@@ -292,7 +268,17 @@ static void omap_crtc_complete_page_flip(struct drm_crtc *crtc)
omap_crtc->event = NULL;
if (event) {
- drm_crtc_send_vblank_event(crtc, event);
+ list_del(&event->base.link);
+
+ /*
+ * Queue the event for delivery if it's still linked to a file
+ * handle, otherwise just destroy it.
+ */
+ if (event->base.file_priv)
+ drm_crtc_send_vblank_event(crtc, event);
+ else
+ event->base.destroy(&event->base);
+
wake_up(&omap_crtc->flip_wait);
drm_crtc_vblank_put(crtc);
}
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 14c8714..1ab8b13 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -105,6 +105,7 @@ static int omap_atomic_commit(struct drm_device *dev,
{
struct omap_drm_private *priv = dev->dev_private;
struct omap_atomic_state_commit *commit;
+ unsigned long flags;
unsigned int i;
int ret;
@@ -141,6 +142,17 @@ static int omap_atomic_commit(struct drm_device *dev,
return ret;
}
+ /* Keep track of all CRTC events to unlink them in preclose(). */
+ spin_lock_irqsave(&dev->event_lock, flags);
+ for (i = 0; i < dev->mode_config.num_crtc; ++i) {
+ struct drm_crtc_state *cstate = state->crtc_states[i];
+
+ if (cstate && cstate->event)
+ list_add_tail(&cstate->event->base.link,
+ &priv->commit.events);
+ }
+ spin_unlock_irqrestore(&dev->event_lock, flags);
+
/* Swap the state, this is the point of no return. */
drm_atomic_helper_swap_state(dev, state);
@@ -618,6 +630,7 @@ static int dev_load(struct drm_device *dev, unsigned long flags)
priv->wq = alloc_ordered_workqueue("omapdrm", 0);
init_waitqueue_head(&priv->commit.wait);
+ INIT_LIST_HEAD(&priv->commit.events);
spin_lock_init(&priv->list_lock);
INIT_LIST_HEAD(&priv->obj_list);
@@ -738,12 +751,23 @@ static void dev_lastclose(struct drm_device *dev)
static void dev_preclose(struct drm_device *dev, struct drm_file *file)
{
struct omap_drm_private *priv = dev->dev_private;
- unsigned int i;
+ struct drm_pending_event *event;
+ unsigned long flags;
DBG("preclose: dev=%p", dev);
- for (i = 0; i < priv->num_crtcs; ++i)
- omap_crtc_cancel_page_flip(priv->crtcs[i], file);
+ /*
+ * Unlink all pending CRTC events to make sure they won't be queued up
+ * by a pending asynchronous commit.
+ */
+ spin_lock_irqsave(&dev->event_lock, flags);
+ list_for_each_entry(event, &priv->commit.events, link) {
+ if (event->file_priv == file) {
+ file->event_space += event->event->length;
+ event->file_priv = NULL;
+ }
+ }
+ spin_unlock_irqrestore(&dev->event_lock, flags);
}
static void dev_postclose(struct drm_device *dev, struct drm_file *file)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index b105394..9cc2d34 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -109,6 +109,7 @@ struct omap_drm_private {
/* atomic commit */
struct {
+ struct list_head events;
wait_queue_head_t wait;
u32 pending;
} commit;
@@ -141,7 +142,6 @@ void omap_fbdev_free(struct drm_device *dev);
const struct omap_video_timings *omap_crtc_timings(struct drm_crtc *crtc);
enum omap_channel omap_crtc_channel(struct drm_crtc *crtc);
-void omap_crtc_cancel_page_flip(struct drm_crtc *crtc, struct drm_file *file);
void omap_crtc_pre_init(void);
void omap_crtc_pre_uninit(void);
struct drm_crtc *omap_crtc_init(struct drm_device *dev,
--
Regards,
Laurent Pinchart
More information about the dri-devel
mailing list