Patch for RPI crash

Submitted by Yves De Muyter on Oct. 9, 2014, 8:37 a.m.

Details

Message ID 1412843860.26745.59.camel@T7400
State Superseded
Delegated to: Pekka Paalanen
Headers show

Not browsing as part of any series.

Commit Message

Yves De Muyter Oct. 9, 2014, 8:37 a.m.
Hello,

Here is a fix for an issue that pops up here when I close a window on a
Raspberry-pi that is using GLES2 to draw contents.
It also reproducible with the clients/simple-egl application by simply
pressing 'esc' on the window (closes the window) or doing 'ctrl-c' on
the terminal. The result is a Weston crash.

What happens is that the buffer is set to NULL while still used.
The patch is against current master.

Signed-off-by: Yves De Muyter <yves@alfavisio.be>









Thanks,

Yves

Patch hide | download patch | download mbox

diff --git a/src/rpi-renderer.c b/src/rpi-renderer.c
index c222eb6..d75ff6d 100644
--- a/src/rpi-renderer.c
+++ b/src/rpi-renderer.c
@@ -703,6 +703,9 @@  rpir_view_compute_rects(struct rpir_view *view,
                struct weston_buffer *buffer =
                        view->surface->egl_front->buffer_ref.buffer;
 
+               if( buffer == NULL ){
+                       return -1;
+               }
                src_width = buffer->width << 16;
                src_height = buffer->height << 16;
        } else {

Comments

Hi,

On 9 October 2014 10:37, Yves De Muyter <yves@alfavisio.be> wrote:

> Here is a fix for an issue that pops up here when I close a window on a
> Raspberry-pi that is using GLES2 to draw contents.
> It also reproducible with the clients/simple-egl application by simply
> pressing 'esc' on the window (closes the window) or doing 'ctrl-c' on
> the terminal. The result is a Weston crash.
>
> What happens is that the buffer is set to NULL while still used.
> The patch is against current master.
>

Hmm, I don't think this is quite the right patch ... more a workaround than
solving the root problem.

It seems like the correct patch would be to set surface->buffer_type to
BUFFER_TYPE_NULL in, e.g., rpi_renderer_attach, and making sure it's not
even considered by the draw stack to begin with. If we ever get to draw a
surface with no buffer, we've screwed up somewhere.

Unfortunately I don't have a RPi with me, so can't test any of this.

Cheers,
Daniel
On Mon, 20 Oct 2014 12:36:57 +0200
Daniel Stone <daniel@fooishbar.org> wrote:

> Hi,
> 
> On 9 October 2014 10:37, Yves De Muyter <yves@alfavisio.be> wrote:
> 
> > Here is a fix for an issue that pops up here when I close a window on a
> > Raspberry-pi that is using GLES2 to draw contents.
> > It also reproducible with the clients/simple-egl application by simply
> > pressing 'esc' on the window (closes the window) or doing 'ctrl-c' on
> > the terminal. The result is a Weston crash.
> >
> > What happens is that the buffer is set to NULL while still used.
> > The patch is against current master.

Do you perhaps have some sort of window closing animation in use?

So, are you getting rpir_view_computer_rects() called from
rpir_view_dmx_add() or rpir_view_dmx_move()?

Seeing the whole backtrace would help pinpointing the right place to
add the check, but I think I have a fair guess on what happens.

> Hmm, I don't think this is quite the right patch ... more a workaround than
> solving the root problem.
> 
> It seems like the correct patch would be to set surface->buffer_type to
> BUFFER_TYPE_NULL in, e.g., rpi_renderer_attach, and making sure it's not
> even considered by the draw stack to begin with. If we ever get to draw a
> surface with no buffer, we've screwed up somewhere.
> 
> Unfortunately I don't have a RPi with me, so can't test any of this.

Hmm, yeah. If the wl_buffer gets destroyed for any reason, be it client
exit or client explicitly destroying it, we should just skip the
surface in repaint completely.

I don't remember the code too well anymore, but I have a feeling the
check should be in rpir_view_update(). It should cause the same path to
be taken, when 'obscured' is true. That way we properly remove the
DispmanX Element from the screen, when we have nothing to show on it.

This naturally breaks any window closing animations, but I don't think
that can be helped, because we don't know if the client is going to
re-use the backing storage of the wl_buffer for something. In the
unlikely event that it does, we'd see garbage during the animation.
OTOH, is skipping the closing animation any better... I'm not sure.

If we wanted to take the (very) small risk of showing garbage instead,
then we'd just need to take the size from somewhere else than the
egl_front->buffer_ref.buffer, that is, we need to save the size when
buffer is attached, not rely on reading it out later. This would be
good to do anyway.

Then again, I think the rpi-renderer and especially its experimental EGL
support are just waiting to be replaced by proper kernel DRM drivers
and the Mesa VC4 driver, but that is a long way to go still.

So if you fix the coding style in this patch (spaces are off, braces
not needed), and add a backtrace and proper commit title to the patch
message, I can push it.


Thanks,
pq
On Wed, Nov 19, 2014 at 02:45:24PM +0200, Pekka Paalanen wrote:
> Then again, I think the rpi-renderer and especially its experimental EGL
> support are just waiting to be replaced by proper kernel DRM drivers
> and the Mesa VC4 driver, but that is a long way to go still.
> 
> So if you fix the coding style in this patch (spaces are off, braces
> not needed), and add a backtrace and proper commit title to the patch
> message, I can push it.

That's exactly my reasoning, the amount of code-change is minimal, it works pretty fine for me and it is going to be deprecated code anyway.
I totally agree that for new or well maintained code this would not be OK, but that would require me to investigate and honestly probably rework a lot of stuff to take the quality a level higher, which means more time and resources than it's worth.

I'll update the tabs and remove the braces later.

-Yves