| Message ID | 1358178932-25505-6-git-send-email-thierry.reding@avionic-design.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 14, 2013 at 4:55 PM, Thierry Reding <thierry.reding@avionic-design.de> wrote: > +static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file) > +{ > + struct drm_crtc *crtc; > + > + list_for_each_entry(crtc, &drm->mode_config.crtc_list, head) > + tegra_dc_cancel_page_flip(crtc, file); > +} > + Why that? If userspace dies while a flip is outstanding, it's imo ok to execute it - otherwise there might be an accounting mismatch if the hw still scans out the old fb, but drm believes the new one is used. Or do I miss something? The reason I've skimmed through the patches is to check for implications with my modeset locking rework. Review on that would be highly appreciated ... Thanks, Daniel
On Tue, Jan 15, 2013 at 06:53:19PM +0100, Daniel Vetter wrote: > On Mon, Jan 14, 2013 at 4:55 PM, Thierry Reding > <thierry.reding@avionic-design.de> wrote: > > +static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file) > > +{ > > + struct drm_crtc *crtc; > > + > > + list_for_each_entry(crtc, &drm->mode_config.crtc_list, head) > > + tegra_dc_cancel_page_flip(crtc, file); > > +} > > + > > Why that? If userspace dies while a flip is outstanding, it's imo ok > to execute it - otherwise there might be an accounting mismatch if the > hw still scans out the old fb, but drm believes the new one is used. > Or do I miss something? I looked at the shmobile driver for inspiration and they do this as well. Doing so seemed reasonable since there'd be no file to deliver the event to. > The reason I've skimmed through the patches is to check for > implications with my modeset locking rework. Review on that would be > highly appreciated ... I'm not sure how suited I am for review given my limited experience, but I'll see if I can make some time to take a look. Thierry
On Tue, Jan 15, 2013 at 9:17 PM, Thierry Reding <thierry.reding@avionic-design.de> wrote: > On Tue, Jan 15, 2013 at 06:53:19PM +0100, Daniel Vetter wrote: >> On Mon, Jan 14, 2013 at 4:55 PM, Thierry Reding >> <thierry.reding@avionic-design.de> wrote: >> > +static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file) >> > +{ >> > + struct drm_crtc *crtc; >> > + >> > + list_for_each_entry(crtc, &drm->mode_config.crtc_list, head) >> > + tegra_dc_cancel_page_flip(crtc, file); >> > +} >> > + >> >> Why that? If userspace dies while a flip is outstanding, it's imo ok >> to execute it - otherwise there might be an accounting mismatch if the >> hw still scans out the old fb, but drm believes the new one is used. >> Or do I miss something? > > I looked at the shmobile driver for inspiration and they do this as > well. Doing so seemed reasonable since there'd be no file to deliver the > event to. Hm, is the code in drm_events_release not good enough? And if it's buggy, we need to fix it. Also adding Laurent to figure out why he added that code in shmob ... >> The reason I've skimmed through the patches is to check for >> implications with my modeset locking rework. Review on that would be >> highly appreciated ... > > I'm not sure how suited I am for review given my limited experience, but > I'll see if I can make some time to take a look. The commit message should nicely explain why I've picked the design and the various implications for drivers. So just checking whether anything collides with your upcoming stuff would be good ... -Daniel
On Wed, Jan 16, 2013 at 10:43:15AM +0100, Daniel Vetter wrote: > On Tue, Jan 15, 2013 at 9:17 PM, Thierry Reding > <thierry.reding@avionic-design.de> wrote: > > On Tue, Jan 15, 2013 at 06:53:19PM +0100, Daniel Vetter wrote: > >> On Mon, Jan 14, 2013 at 4:55 PM, Thierry Reding > >> <thierry.reding@avionic-design.de> wrote: > >> > +static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file) > >> > +{ > >> > + struct drm_crtc *crtc; > >> > + > >> > + list_for_each_entry(crtc, &drm->mode_config.crtc_list, head) > >> > + tegra_dc_cancel_page_flip(crtc, file); > >> > +} > >> > + > >> > >> Why that? If userspace dies while a flip is outstanding, it's imo ok > >> to execute it - otherwise there might be an accounting mismatch if the > >> hw still scans out the old fb, but drm believes the new one is used. > >> Or do I miss something? > > > > I looked at the shmobile driver for inspiration and they do this as > > well. Doing so seemed reasonable since there'd be no file to deliver the > > event to. > > Hm, is the code in drm_events_release not good enough? And if it's > buggy, we need to fix it. Also adding Laurent to figure out why he > added that code in shmob ... drm_events_release() should be enough to clean up the events, but I suspect the reason why Laurent put that code in was that the drm_crtc private data still has a reference to the event and needs to clear it. Otherwise the next page flip won't be scheduled because .page_flip() would return -EBUSY. However, it seems like {tegra_dc,shmob_drm_crtc}_cancel_page_flip() could both be simplified a lot and just set their event to NULL. Then again, maybe keeping a separate reference isn't all that useful. Maybe the better thing to do here is iterate over the list of pending VBLANK events in *_finish_page_flip() and process each of them? That would allow more than one user-space process to queue page flips. > >> The reason I've skimmed through the patches is to check for > >> implications with my modeset locking rework. Review on that would be > >> highly appreciated ... > > > > I'm not sure how suited I am for review given my limited experience, but > > I'll see if I can make some time to take a look. > > The commit message should nicely explain why I've picked the design > and the various implications for drivers. So just checking whether > anything collides with your upcoming stuff would be good ... Okay, I'll take a closer look. Thierry
On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding <thierry.reding@avionic-design.de> wrote: > drm_events_release() should be enough to clean up the events, but I > suspect the reason why Laurent put that code in was that the drm_crtc > private data still has a reference to the event and needs to clear it. > Otherwise the next page flip won't be scheduled because .page_flip() > would return -EBUSY. Hm, indeed we seem to have a nice bug in most drivers there :( > However, it seems like {tegra_dc,shmob_drm_crtc}_cancel_page_flip() > could both be simplified a lot and just set their event to NULL. Then > again, maybe keeping a separate reference isn't all that useful. Maybe > the better thing to do here is iterate over the list of pending VBLANK > events in *_finish_page_flip() and process each of them? That would > allow more than one user-space process to queue page flips. I think we need a slightly more generally useful solution, since most drivers are currently broken. I've read a bit through the code, but short of refcounting drm events and adding event->file_priv checks at relevent places I don't see a sane solution ... And even that one is rather invasive. Do you have an idea? Imo doing the cleanup in each driver will be rather error-prone, and since usually kms clients wait for flips to complete, also guaranteed to be little tested. -Daniel
On Wed, Jan 16, 2013 at 6:36 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding > <thierry.reding@avionic-design.de> wrote: >> drm_events_release() should be enough to clean up the events, but I >> suspect the reason why Laurent put that code in was that the drm_crtc >> private data still has a reference to the event and needs to clear it. >> Otherwise the next page flip won't be scheduled because .page_flip() >> would return -EBUSY. > > Hm, indeed we seem to have a nice bug in most drivers there :( > >> However, it seems like {tegra_dc,shmob_drm_crtc}_cancel_page_flip() >> could both be simplified a lot and just set their event to NULL. Then >> again, maybe keeping a separate reference isn't all that useful. Maybe >> the better thing to do here is iterate over the list of pending VBLANK >> events in *_finish_page_flip() and process each of them? That would >> allow more than one user-space process to queue page flips. > > I think we need a slightly more generally useful solution, since most > drivers are currently broken. I've read a bit through the code, but > short of refcounting drm events and adding event->file_priv checks at > relevent places I don't see a sane solution ... And even that one is > rather invasive. Do you have an idea? Imo doing the cleanup in each > driver will be rather error-prone, and since usually kms clients wait > for flips to complete, also guaranteed to be little tested. I suppose we could move the *pending_vblank_event to 'struct drm_crtc'.. I think probably all/most drivers need the same thing anyway. If a driver needs to do something special, it could just never set crtc->pending_vblank_event and instead do it's own cleanup. BR, -R > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Jan 16, 2013 at 01:36:17PM +0100, Daniel Vetter wrote: > On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding > <thierry.reding@avionic-design.de> wrote: > > drm_events_release() should be enough to clean up the events, but I > > suspect the reason why Laurent put that code in was that the drm_crtc > > private data still has a reference to the event and needs to clear it. > > Otherwise the next page flip won't be scheduled because .page_flip() > > would return -EBUSY. > > Hm, indeed we seem to have a nice bug in most drivers there :( > > > However, it seems like {tegra_dc,shmob_drm_crtc}_cancel_page_flip() > > could both be simplified a lot and just set their event to NULL. Then > > again, maybe keeping a separate reference isn't all that useful. Maybe > > the better thing to do here is iterate over the list of pending VBLANK > > events in *_finish_page_flip() and process each of them? That would > > allow more than one user-space process to queue page flips. > > I think we need a slightly more generally useful solution, since most > drivers are currently broken. I've read a bit through the code, but > short of refcounting drm events and adding event->file_priv checks at > relevent places I don't see a sane solution ... And even that one is > rather invasive. Do you have an idea? Imo doing the cleanup in each > driver will be rather error-prone, and since usually kms clients wait > for flips to complete, also guaranteed to be little tested. While this probably doesn't improve the situation much, maybe adding more extensive tests to libdrm or so would help. I wrote a couple of small programs to test vblank and plane support. The vblank one basically generates two framebuffers with different patterns and uses page-flipping to alternate between them. The plane test does something similar, sets up a plane and associates a buffer with it. It includes scaling the plane to test that functionality as well. Perhaps these tests could even be added to the existing libdrm tests, but maybe having separate binaries wouldn't hurt. Back to the original topic: should it not work to queue a page flip and immediately exit(0) after that to test the cleanup code? I suppose if that can be tested on all drivers we should have pretty good coverage. Thierry
On Wed, Jan 16, 2013 at 01:36:17PM +0100, Daniel Vetter wrote: > On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding > <thierry.reding@avionic-design.de> wrote: > > drm_events_release() should be enough to clean up the events, but I > > suspect the reason why Laurent put that code in was that the drm_crtc > > private data still has a reference to the event and needs to clear it. > > Otherwise the next page flip won't be scheduled because .page_flip() > > would return -EBUSY. > > Hm, indeed we seem to have a nice bug in most drivers there :( I think I may just recently have run into this bug on Intel hardware. Although perhaps I just used this wrongly. Just for the fun of it I wanted to implement Conway's Game of Life on top of DRM/KMS. So I use two dumb buffer objects to alternately render to. Then I wanted to use page-flipping to synchronize with VBLANK. So the sequence is basically: while (!done) { grid_tick(grid); grid_draw(grid, screen); screen_flip(screen); grid_swap(grid); } Where screen_flip() chooses the framebuffer and passes it to drmModePageFlip() like so: int fb = screen->fb[screen->current]; drmModePageFlip(screen->fd, screen->crtc, fb, DRM_MODE_PAGE_FLIP_EVENT, screen); This runs for about 3 seconds and then hangs, so the display is no longer updated. I've also verified that the same happens on Radeon. But maybe I am mistaken and this isn't the proper programming sequence? Thierry
On Wed, Jan 30, 2013 at 10:32:47AM +0100, Thierry Reding wrote: > On Wed, Jan 16, 2013 at 01:36:17PM +0100, Daniel Vetter wrote: > > On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding > > <thierry.reding@avionic-design.de> wrote: > > > drm_events_release() should be enough to clean up the events, but I > > > suspect the reason why Laurent put that code in was that the drm_crtc > > > private data still has a reference to the event and needs to clear it. > > > Otherwise the next page flip won't be scheduled because .page_flip() > > > would return -EBUSY. > > > > Hm, indeed we seem to have a nice bug in most drivers there :( > > I think I may just recently have run into this bug on Intel hardware. > Although perhaps I just used this wrongly. > > Just for the fun of it I wanted to implement Conway's Game of Life on > top of DRM/KMS. So I use two dumb buffer objects to alternately render > to. Then I wanted to use page-flipping to synchronize with VBLANK. > > So the sequence is basically: > > while (!done) { > grid_tick(grid); > grid_draw(grid, screen); > screen_flip(screen); > grid_swap(grid); > } > > Where screen_flip() chooses the framebuffer and passes it to > drmModePageFlip() like so: > > int fb = screen->fb[screen->current]; > > drmModePageFlip(screen->fd, screen->crtc, fb, > DRM_MODE_PAGE_FLIP_EVENT, screen); > > This runs for about 3 seconds and then hangs, so the display is no > longer updated. I've also verified that the same happens on Radeon. > But maybe I am mistaken and this isn't the proper programming sequence? You asked for page flip events. Do you actually handle them in your code?
On Wed, Jan 30, 2013 at 11:42:40AM +0200, Ville Syrjälä wrote: > On Wed, Jan 30, 2013 at 10:32:47AM +0100, Thierry Reding wrote: > > On Wed, Jan 16, 2013 at 01:36:17PM +0100, Daniel Vetter wrote: > > > On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding > > > <thierry.reding@avionic-design.de> wrote: > > > > drm_events_release() should be enough to clean up the events, but I > > > > suspect the reason why Laurent put that code in was that the drm_crtc > > > > private data still has a reference to the event and needs to clear it. > > > > Otherwise the next page flip won't be scheduled because .page_flip() > > > > would return -EBUSY. > > > > > > Hm, indeed we seem to have a nice bug in most drivers there :( > > > > I think I may just recently have run into this bug on Intel hardware. > > Although perhaps I just used this wrongly. > > > > Just for the fun of it I wanted to implement Conway's Game of Life on > > top of DRM/KMS. So I use two dumb buffer objects to alternately render > > to. Then I wanted to use page-flipping to synchronize with VBLANK. > > > > So the sequence is basically: > > > > while (!done) { > > grid_tick(grid); > > grid_draw(grid, screen); > > screen_flip(screen); > > grid_swap(grid); > > } > > > > Where screen_flip() chooses the framebuffer and passes it to > > drmModePageFlip() like so: > > > > int fb = screen->fb[screen->current]; > > > > drmModePageFlip(screen->fd, screen->crtc, fb, > > DRM_MODE_PAGE_FLIP_EVENT, screen); > > > > This runs for about 3 seconds and then hangs, so the display is no > > longer updated. I've also verified that the same happens on Radeon. > > But maybe I am mistaken and this isn't the proper programming sequence? > > You asked for page flip events. Do you actually handle them in your code? Duh. No I wasn't =) I suppose some queue must be running full if the event isn't handled by calling drmHandleEvent(). Okay, this now works properly with page-flipping. Thanks. Thierry
On Wed, Jan 30, 2013 at 12:14:36PM +0100, Thierry Reding wrote: > On Wed, Jan 30, 2013 at 11:42:40AM +0200, Ville Syrjälä wrote: > > On Wed, Jan 30, 2013 at 10:32:47AM +0100, Thierry Reding wrote: > > > On Wed, Jan 16, 2013 at 01:36:17PM +0100, Daniel Vetter wrote: > > > > On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding > > > > <thierry.reding@avionic-design.de> wrote: > > > > > drm_events_release() should be enough to clean up the events, but I > > > > > suspect the reason why Laurent put that code in was that the drm_crtc > > > > > private data still has a reference to the event and needs to clear it. > > > > > Otherwise the next page flip won't be scheduled because .page_flip() > > > > > would return -EBUSY. > > > > > > > > Hm, indeed we seem to have a nice bug in most drivers there :( > > > > > > I think I may just recently have run into this bug on Intel hardware. > > > Although perhaps I just used this wrongly. > > > > > > Just for the fun of it I wanted to implement Conway's Game of Life on > > > top of DRM/KMS. So I use two dumb buffer objects to alternately render > > > to. Then I wanted to use page-flipping to synchronize with VBLANK. > > > > > > So the sequence is basically: > > > > > > while (!done) { > > > grid_tick(grid); > > > grid_draw(grid, screen); > > > screen_flip(screen); > > > grid_swap(grid); > > > } > > > > > > Where screen_flip() chooses the framebuffer and passes it to > > > drmModePageFlip() like so: > > > > > > int fb = screen->fb[screen->current]; > > > > > > drmModePageFlip(screen->fd, screen->crtc, fb, > > > DRM_MODE_PAGE_FLIP_EVENT, screen); > > > > > > This runs for about 3 seconds and then hangs, so the display is no > > > longer updated. I've also verified that the same happens on Radeon. > > > But maybe I am mistaken and this isn't the proper programming sequence? > > > > You asked for page flip events. Do you actually handle them in your code? > > Duh. No I wasn't =) I suppose some queue must be running full if the > event isn't handled by calling drmHandleEvent(). Okay, this now works > properly with page-flipping. Just in case anybody's interested, the code is here: https://gitorious.org/thierryreding/kmslife Thierry
On Wednesday 16 January 2013 13:36:17 Daniel Vetter wrote: > On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding wrote: > > drm_events_release() should be enough to clean up the events, but I > > suspect the reason why Laurent put that code in was that the drm_crtc > > private data still has a reference to the event and needs to clear it. > > Otherwise the next page flip won't be scheduled because .page_flip() > > would return -EBUSY. > > Hm, indeed we seem to have a nice bug in most drivers there :( That's not the only reason. drm_events_release() handles vblank events added to the vblank_event_list. That list only contains vblank events added by drm_queue_vblank_event(), only called by drm_wait_vblank(). Page flip events never get there, so they need to be cleaned up manually. I wrote in the DRM documentation, in the page flipping section, "FIXME: Could drivers that don't need to wait for rendering to complete just add the event to dev->vblank_event_list and let the DRM core handle everything, as for "normal" vertical blanking events?" We should investigate that. > > However, it seems like {tegra_dc,shmob_drm_crtc}_cancel_page_flip() > > could both be simplified a lot and just set their event to NULL. Then > > again, maybe keeping a separate reference isn't all that useful. Maybe > > the better thing to do here is iterate over the list of pending VBLANK > > events in *_finish_page_flip() and process each of them? That would > > allow more than one user-space process to queue page flips. > > I think we need a slightly more generally useful solution, since most > drivers are currently broken. I've read a bit through the code, but > short of refcounting drm events and adding event->file_priv checks at > relevent places I don't see a sane solution ... And even that one is > rather invasive. Do you have an idea? Imo doing the cleanup in each > driver will be rather error-prone, and since usually kms clients wait > for flips to complete, also guaranteed to be little tested.
Hi Thierry, On Wednesday 16 January 2013 19:56:06 Thierry Reding wrote: > On Wed, Jan 16, 2013 at 01:36:17PM +0100, Daniel Vetter wrote: > > On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding wrote: > > > drm_events_release() should be enough to clean up the events, but I > > > suspect the reason why Laurent put that code in was that the drm_crtc > > > private data still has a reference to the event and needs to clear it. > > > Otherwise the next page flip won't be scheduled because .page_flip() > > > would return -EBUSY. > > > > Hm, indeed we seem to have a nice bug in most drivers there :( > > > > > However, it seems like {tegra_dc,shmob_drm_crtc}_cancel_page_flip() > > > could both be simplified a lot and just set their event to NULL. Then > > > again, maybe keeping a separate reference isn't all that useful. Maybe > > > the better thing to do here is iterate over the list of pending VBLANK > > > events in *_finish_page_flip() and process each of them? That would > > > allow more than one user-space process to queue page flips. > > > > I think we need a slightly more generally useful solution, since most > > drivers are currently broken. I've read a bit through the code, but > > short of refcounting drm events and adding event->file_priv checks at > > relevent places I don't see a sane solution ... And even that one is > > rather invasive. Do you have an idea? Imo doing the cleanup in each > > driver will be rather error-prone, and since usually kms clients wait > > for flips to complete, also guaranteed to be little tested. > > While this probably doesn't improve the situation much, maybe adding > more extensive tests to libdrm or so would help. I wrote a couple of > small programs to test vblank and plane support. > > The vblank one basically generates two framebuffers with different > patterns and uses page-flipping to alternate between them. The plane > test does something similar, sets up a plane and associates a buffer > with it. It includes scaling the plane to test that functionality as > well. > > Perhaps these tests could even be added to the existing libdrm tests, > but maybe having separate binaries wouldn't hurt. Further cleanup of the modetest application is somewhere on my to-do list (but probably so low that I'll never get to it unless there's a real incentive ;-)). It's a good candidate for more page flip testing (there's basic page flip support there already). > Back to the original topic: should it not work to queue a page flip and > immediately exit(0) after that to test the cleanup code? I suppose if > that can be tested on all drivers we should have pretty good coverage. Maybe we need some kind of compliance test suite tool ?
On Sat, Feb 2, 2013 at 12:05 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: >> Back to the original topic: should it not work to queue a page flip and >> immediately exit(0) after that to test the cleanup code? I suppose if >> that can be tested on all drivers we should have pretty good coverage. > > Maybe we need some kind of compliance test suite tool ? kms_flip from our intel-specific testsuite is probably the most paranoid thing for testing page flips out there: http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/tree/tests/kms_flip.c Cheers, Daniel
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 3aa7ccc..ce4e14a 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -161,7 +161,78 @@ void tegra_dc_disable_vblank(struct tegra_dc *dc) spin_unlock_irqrestore(&dc->lock, flags); } +static void tegra_dc_finish_page_flip(struct tegra_dc *dc) +{ + struct drm_pending_vblank_event *event; + struct drm_device *drm = dc->base.dev; + unsigned long flags; + struct timeval now; + + spin_lock_irqsave(&drm->event_lock, flags); + event = dc->event; + dc->event = NULL; + spin_unlock_irqrestore(&drm->event_lock, flags); + + if (!event) + return; + + event->event.sequence = drm_vblank_count_and_time(drm, dc->pipe, &now); + event->event.tv_sec = now.tv_sec; + event->event.tv_usec = now.tv_usec; + + spin_lock_irqsave(&drm->event_lock, flags); + list_add_tail(&event->base.link, &event->base.file_priv->event_list); + wake_up_interruptible(&event->base.file_priv->event_wait); + spin_unlock_irqrestore(&drm->event_lock, flags); + + drm_vblank_put(drm, dc->pipe); +} + +void tegra_dc_cancel_page_flip(struct drm_crtc *crtc, struct drm_file *file) +{ + struct tegra_dc *dc = to_tegra_dc(crtc); + struct drm_device *drm = crtc->dev; + unsigned long flags; + + spin_lock_irqsave(&drm->event_lock, flags); + + if (dc->event && dc->event->base.file_priv == file) { + dc->event->base.destroy(&dc->event->base); + drm_vblank_put(drm, dc->pipe); + dc->event = NULL; + } + + spin_unlock_irqrestore(&drm->event_lock, flags); +} + +static int tegra_dc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb, + struct drm_pending_vblank_event *event) +{ + struct tegra_framebuffer *newfb = to_tegra_fb(fb); + struct tegra_dc *dc = to_tegra_dc(crtc); + struct drm_device *drm = crtc->dev; + unsigned long flags; + + if (dc->event) + return -EBUSY; + + tegra_dc_set_base(dc, 0, 0, newfb); + + if (event) { + event->pipe = dc->pipe; + + spin_lock_irqsave(&drm->event_lock, flags); + dc->event = event; + spin_unlock_irqrestore(&drm->event_lock, flags); + + drm_vblank_get(drm, dc->pipe); + } + + return 0; +} + static const struct drm_crtc_funcs tegra_crtc_funcs = { + .page_flip = tegra_dc_page_flip, .set_config = drm_crtc_helper_set_config, .destroy = drm_crtc_cleanup, }; @@ -556,6 +627,7 @@ static irqreturn_t tegra_dc_irq(int irq, void *data) dev_dbg(dc->dev, "%s(): vertical blank\n", __func__); */ drm_handle_vblank(dc->base.dev, dc->pipe); + tegra_dc_finish_page_flip(dc); } if (status & (WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT)) { diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index 62f8121..7bab784 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -129,11 +129,20 @@ static void tegra_drm_disable_vblank(struct drm_device *drm, int pipe) tegra_dc_disable_vblank(dc); } +static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file) +{ + struct drm_crtc *crtc; + + list_for_each_entry(crtc, &drm->mode_config.crtc_list, head) + tegra_dc_cancel_page_flip(crtc, file); +} + struct drm_driver tegra_drm_driver = { .driver_features = DRIVER_BUS_PLATFORM | DRIVER_MODESET | DRIVER_GEM, .load = tegra_drm_load, .unload = tegra_drm_unload, .open = tegra_drm_open, + .preclose = tegra_drm_preclose, .lastclose = tegra_drm_lastclose, .get_vblank_counter = drm_vblank_count, diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h index bb5e9d3..3aa21b1 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -103,6 +103,9 @@ struct tegra_dc { struct drm_info_list *debugfs_files; struct drm_minor *minor; struct dentry *debugfs; + + /* page-flip handling */ + struct drm_pending_vblank_event *event; }; static inline struct tegra_dc *host1x_client_to_dc(struct host1x_client *client) @@ -152,6 +155,8 @@ extern int tegra_dc_setup_window(struct tegra_dc *dc, unsigned int index, const struct tegra_dc_window *window); extern void tegra_dc_enable_vblank(struct tegra_dc *dc); extern void tegra_dc_disable_vblank(struct tegra_dc *dc); +extern void tegra_dc_cancel_page_flip(struct drm_crtc *crtc, + struct drm_file *file); struct display { unsigned int width;
All the necessary support bits like .mode_set_base() and VBLANK are now available, so page-flipping case easily be implemented on top. Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> --- drivers/gpu/drm/tegra/dc.c | 72 +++++++++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/tegra/drm.c | 9 ++++++ drivers/gpu/drm/tegra/drm.h | 5 ++++ 3 files changed, 86 insertions(+)