glmark2

Merge lp://qastaging/~glmark2-dev/glmark2/canvas-drm-rebranch into lp://qastaging/glmark2/2011.11

  1. canvas-drm-rebranch
  2. Merge into trunk
Proposed by Jesse Barker
Status: Merged
Merged at revision: 257
Proposed branch: lp://qastaging/~glmark2-dev/glmark2/canvas-drm-rebranch
Merge into: lp://qastaging/glmark2/2011.11
Diff against target: 907 lines (+798/-12)
6 files modified
INSTALL (+1/-1)
src/canvas-drm.cpp (+587/-0)
src/canvas-drm.h (+132/-0)
src/main.cpp (+6/-2)
src/wscript_build (+43/-0)
wscript (+29/-9)
To merge this branch: bzr merge lp://qastaging/~glmark2-dev/glmark2/canvas-drm-rebranch
Reviewer Review Type Date Requested Status
Alexandros Frantzis Approve
Jesse Barker Needs Resubmitting
Review via email: mp+138605@code.qastaging.launchpad.net

Description of the change

CanvasDRM: Add a new canvas type that renders directly to DRM-managed buffers rather than going through X11. The canvas uses GBM for surface management, DRM (KMS) for mode control and page flip, and EGL for rendering context and config management. Client API support is predicated upon the EGL, but both OpenGL and OpenGL ES 2.0 are supported.

This work was originally done on lp:~glmark2-dev/glmark2/canvas-drm, however, based upon review comments there, it was decided it would be cleaner (with respect to other development branches that have been merged in the interim) to rebranch and reintegrate the work. So, I rebranched from current trunk and then did a 'bzr merge --interactive' from canvas-drm into this branch and de-selected the changes relating to egl-state.{h,cpp}. I think we lose a bit of the revision history, but actually gain a bit of clarity.

To post a comment you must log in.
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

Rendering works fine on the platforms I have tested. The only problems I see are during shutdown:

1. Pressing Ctrl+C occasionally doesn't seem to reset the crtc correctly, so I end up with a blank screen. It also always causes an error message to be displayed (Error in select: -1).

2. Pressing Enter (and therefore flushing stdin), causes a segfault. This happens because we are leaving DRMState::do_flip() with both surface buffers locked. Next time we call gbm_surface_lock_front_buffer(surface_) we get back a NULL value which we currently don't handle properly (i.e. we just use it). We get into the same situation if select fails etc

(1) is not critical, so I am ok with shipping this as part of an experimental drm backend release (perhaps we could add a warning about the experimental status when startinp up?).

On the other hand, I think (2) should be fixed before this release, if possible, as it's relatively easy for a user to cause a segfault. For example, a user that wants to quit will probably try pressing ESC (which won't work), then some keys then Enter.

review: Needs Fixing
Revision history for this message
Jesse Barker (jesse-barker) wrote :

> Rendering works fine on the platforms I have tested. The only problems I see
> are during shutdown:
>
> 1. Pressing Ctrl+C occasionally doesn't seem to reset the crtc correctly, so I
> end up with a blank screen. It also always causes an error message to be
> displayed (Error in select: -1).

This obviously works better for you than it does for me (possibly a driver-specific thing?).

>
> 2. Pressing Enter (and therefore flushing stdin), causes a segfault. This
> happens because we are leaving DRMState::do_flip() with both surface buffers
> locked. Next time we call gbm_surface_lock_front_buffer(surface_) we get back
> a NULL value which we currently don't handle properly (i.e. we just use it).
> We get into the same situation if select fails etc

Are you saying that there's a segfault when you press <enter> after glmark2 exits normally? I'm not clear I've ever see this, but I think I'm misunderstanding the scenario.

>
> (1) is not critical, so I am ok with shipping this as part of an experimental
> drm backend release (perhaps we could add a warning about the experimental
> status when startinp up?).

Probably should declare this a bit experimental in any case.

>
> On the other hand, I think (2) should be fixed before this release, if
> possible, as it's relatively easy for a user to cause a segfault. For example,
> a user that wants to quit will probably try pressing ESC (which won't work),
> then some keys then Enter.

Agreed. I'll try to reproduce. If you can provide more info on how to reproduce, that would be very useful.

cheers,
Jesse

lp://qastaging/~glmark2-dev/glmark2/canvas-drm-rebranch updated
256. By Jesse Barker

CanvasDRM: Fix handling of Ctrl-C. Because the Ctrl-C was being handled by
CanvasDRM directly, DRMState had no knowledge of the event, and wasn't able
to detect it properly when select() came back with an error. Move the
quit handler into DRMState, and give it a public should_quit() method that
CanvasDRM can call. This is better encapsulation, and allows DRMState to
properly detect the user interrupt and release the GBM resources so the
console isn't hung when the application exits.

Revision history for this message
Jesse Barker (jesse-barker) wrote :

OK, got it. I reproduced the behavior, and have pushed a fix at revision 256. This works fine for me from both a serial console, as well as the "main" console (Ctrl-Alt-F1). The signal handler is now better encapsulated within DRMState, and allows the resources to be released properly. I've squelched the "error in select" message by making it debug only, as the vast majority of the time we would see that is precisely the case where a user interrupt has broken us out. I also added a notice to CanvasDRM::init() to indicate that the DRM canvas is experimental and is subject to change with the evolution of buffer management and modesetting APIs.

review: Needs Resubmitting
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

Even with the latest changes, I still get stuck with the glmark2 buffer on screen occasionally (~1 out of 5 times) after exiting with Ctrl-C. There are no errors or hangs, and if I switch to the X11 VT and back again, everything is displayed correctly.

> Are you saying that there's a segfault when you press <enter> after glmark2 exits normally?
> I'm not clear I've ever see this, but I think I'm misunderstanding the scenario.

To reproduce, start glmark2*-drm and press enter while a benchmark is running.

Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

> Even with the latest changes, I still get stuck with the glmark2 buffer on
> screen occasionally (~1 out of 5 times) after exiting with Ctrl-C. There are
> no errors or hangs, and if I switch to the X11 VT and back again, everything
> is displayed correctly.

Note that this happens for me only with radeon; I can't reproduce it at all with i915 (tried until my fingers got tired of pressing Ctrl-C ;)). This could well by some kind of driver issue, so I don't think it's worth pursuing further for now.

> > Are you saying that there's a segfault when you press <enter> after glmark2
> exits normally?
> > I'm not clear I've ever see this, but I think I'm misunderstanding the
> scenario.
>
> To reproduce, start glmark2*-drm and press enter while a benchmark is running.

This issue, however, is 100% reproducible on both platforms.

lp://qastaging/~glmark2-dev/glmark2/canvas-drm-rebranch updated
257. By Jesse Barker

CanvasDRM: Further simplify the select() logic. Firstly, as we have a NULL
pointer for the timeout parameter, select() won't time out and we don't need to
handle that return value. Secondly, unless we have a really great idea for
handling user input (apart from Ctrl-C), we don't need to listen on stdin.

Revision history for this message
Jesse Barker (jesse-barker) wrote :

Got rid of the need (well, the need already wasn't there ;-) for listening on stdin for arbitrary user input. We also didn't need to watch for a time out. Things work fine for me, even with interspersed <Enter> and <Ctrl-C> presses.

review: Needs Resubmitting
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

Looks good.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: