[Mesa-dev] [RFC PATCH] egl/android: Dequeue buffers inside EGL calls
Mauro Rossi
issor.oruam at gmail.com
Mon Apr 3 22:54:39 UTC 2017
Hi Tomasz,
2017-04-03 10:00 GMT+02:00 Tomasz Figa <tfiga at chromium.org>:
> Hi Mauro,
>
> On Mon, Apr 3, 2017 at 2:48 AM, Mauro Rossi <issor.oruam at gmail.com> wrote:
> >
> >
> > 2017-03-31 13:05 GMT+02:00 Tapani Pälli <tapani.palli at intel.com>:
> >>
> >>
> >>
> >> On 03/31/2017 10:12 AM, Tapani Pälli wrote:
> >>>
> >>>
> >>>
> >>> On 03/31/2017 09:06 AM, Tapani Pälli wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 03/31/2017 08:24 AM, Rob Clark wrote:
> >>>>>
> >>>>> On Fri, Mar 31, 2017 at 12:22 AM, Tapani Pälli
> >>>>> <tapani.palli at intel.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 03/30/2017 05:57 PM, Emil Velikov wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> On 30 March 2017 at 15:30, Tomasz Figa <tfiga at chromium.org> wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On Thu, Mar 30, 2017 at 11:17 PM, Emil Velikov
> >>>>>>>> <emil.l.velikov at gmail.com>
> >>>>>>>> wrote:
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> On 30 March 2017 at 11:55, Tomasz Figa <tfiga at chromium.org>
> wrote:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Android buffer queues can be abandoned, which results in failing
> >>>>>>>>>> to
> >>>>>>>>>> dequeue next buffer. Currently this would fail somewhere deep
> >>>>>>>>>> within
> >>>>>>>>>> the DRI stack calling loader's getBuffers*(), without any error
> >>>>>>>>>> reporting to the client app. However Android framework code
> >>>>>>>>>> relies on
> >>>>>>>>>> proper signaling of this event, so we move buffer dequeue to
> >>>>>>>>>> createWindowSurface() and swapBuffers() call, which can generate
> >>>>>>>>>> proper
> >>>>>>>>>> EGL errors. To keep the performance benefits of delayed buffer
> >>>>>>>>>> handling,
> >>>>>>>>>> if any, fence wait and DRI image creation is kept delayed until
> >>>>>>>>>> getBuffers*() is called by the DRI driver.
> >>>>>>>>>>
> >>>>>>>>> Thank you Tomasz.
> >>>>>>>>>
> >>>>>>>>> I'm fairly confident that this should resolve the crash [in
> >>>>>>>>> swap_buffers] that Mauro was seeing.
> >>>>>>>>> Mauro can you give it a test ?
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Ah, I actually noticed a problem with existing code, supposedly
> >>>>>>>> fixed
> >>>>>>>> by [1], but I'm afraid it's still wrong.
> >>>>>>>>
> >>>>>>>> Current swap_buffers calls get_back_bo(), but doesn't call
> >>>>>>>> update_buffers(), which is the function that should be called
> before
> >>>>>>>> to actually dequeue a buffer from Android's buffer queue. Given
> >>>>>>>> that,
> >>>>>>>> get_back_bo() would simply fail with !dri2_surf->buffer, because
> no
> >>>>>>>> buffer was dequeued.
> >>>>>>>>
> >>>>>>> Right - I was wondering why we don't hit that on EGL/GBM or
> >>>>>>> EGL/Wayland.
> >>>>>>> From a quick look - may be because EGL/Android drops the dpy mutex
> in
> >>>>>>> droid_window_enqueue_buffer().
> >>>>>>>
> >>>>>>>> My patch removes update_buffers() and changes the buffer
> >>>>>>>> management so
> >>>>>>>> that there is always a buffer dequeued, starting from surface
> >>>>>>>> creation, unless there was an error somewhere.
> >>>>>>>>
> >>>>>>> Of the top of your head - is there something stopping us from using
> >>>>>>> the same method on $other platforms?
> >>>>>>>
> >>>>>>>> [1]
> >>>>>>>>
> >>>>>>>> https://cgit.freedesktop.org/mesa/mesa/commit/src/egl/
> drivers/dri2/platform_android.c?id=4d4558411db166d2d66f8cec9cb581
> 149dbe1597
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Not that huge of an expert on the Android specifics, so just a
> >>>>>>>>> humble
> >>>>>>>>> request:
> >>>>>>>>> Can we seek the code resuffle (droid_{alloc,free}_local_buffer,
> >>>>>>>
> >>>>>>>
> >>>>>>> Oops silly typo - s/seek/split/.
> >>>>>>>
> >>>>>>>>> other?) separate from the functionality changes ?
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Sure. Thanks for suggestion.
> >>>>>>>>
> >>>>>>> Please give it a day or two for others to comment.
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> I'm trying to debug why this causes our homescreen (wallpaper) to be
> >>>>>> black.
> >>>>>> Otherwise I haven't seen any issues with these changes.
> >>>>>>
> >>>>>
> >>>>> wallpaper seems to be a special sorta hell.. I wonder if there is
> >>>>> somehow some sort of interaction with what I fixed / worked-around in
> >>>>> a5e733c6b52e93de3000647d075f5ca2f55fcb71 ??
> >>>>>
> >>>>> Maybe at least try commenting out the temp-pbuffer thing to get max
> >>>>> texture size, and see if that "fixes" things
> >>>>
> >>>>
> >>>> Can you give more details, I still live in la la land and don't know
> >>>> about 'temp-pbuffer thing'?
> >>>>
> >>>
> >>> aa I did not recall the problem, you mean the 'dummy pbuffer' in
> >>> SurfaceFlinger .. yes I will check if this is related.
> >>>
> >>
> >> If I take away that dummy pbuffer usage (which is useless anyway),
> couple
> >> of errors disappear from the log. They are:
> >>
> >> SurfaceFlinger: releasePendingBuffer failed: Unknown error -1 (1)
> >> SurfaceFlinger: releasePendingBuffer failed: Unknown error -1 (1)
> >>
> >> but otherwise the desktop still stays black, live wallpapers seem to
> work
> >> so there is something special about this default wallpaper. Will
> continue
> >> digging ..
> >>
> >> // Tapani
> >
> >
> >
> > Hi,
> >
> > about the black wallpaper the only sign found in logcat is the following:
> >
> > --------- beginning of main
> > 04-02 15:09:43.148
> > ...
> > 04-02 15:10:41.710 1414 1414 E Layer :
> > [com.android.systemui.ImageWallpaper] rejecting buffer: bufWidth=1024,
> > bufHeight=768, front.active.{w=1, h=1}
> > ...
> > 04-02 15:10:41.726 1414 1414 E Layer :
> > [com.android.systemui.ImageWallpaper] rejecting buffer: bufWidth=1024,
> > bufHeight=768, front.active.{w=1, h=1}
> >
> > Are the expected width and height correct?
> >
> > In dmesg at relative dmesg timestamp around 58 seconds there is no
> > signal/error,
> > just the selinux log (set to permissive):
> >
> > [ 58.271833] type=1400 audit(1491145841.554:192): avc: denied { ioctl }
> > for pid=2141 comm="ndroid.systemui" path="/dev/dri/card0" dev="tmpfs"
> > ino=7199 ioctlcmd=6467 scontext=u:r:platform_app:s0:c512,c768
> > tcontext=u:object_r:device:s0 tclass=chr_file permissive=1
> > [ 58.271879] type=1400 audit(1491145841.554:193): avc: denied { read
> write
> > } for pid=2141 comm="ndroid.systemui" path="/dev/dri/card0" dev="tmpfs"
> > ino=7199 scontext=u:r:platform_app:s0:c512,c768
> > tcontext=u:object_r:device:s0 tclass=chr_file permissive=1
> >
> > I hope these information may help
>
> I found that the code was missing acquire fence wait in case of the
> DRI loader (droid_get_buffers_parse_attachments()). Would you be able
> to add the following chunk and check if the wallpaper is still broken?
>
> diff --git a/src/egl/drivers/dri2/platform_android.c
> b/src/egl/drivers/dri2/platform_android.c
> index 35aba3a7f0..dc29cc24b2 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -917,6 +917,10 @@ droid_get_buffers_parse_attachments(struct
> dri2_egl_surface *dri2_surf,
> if (!dri2_surf->buffer)
> continue;
>
> + /* Android might have given us an acquire fence to wait for.
> If so,
> + * we need to wait for it and close the descriptor after
> that. */
> + wait_and_close_acquire_fence(dri2_surf);
> +
> buf->attachment = attachments[i];
> buf->name = get_native_buffer_name(dri2_surf->buffer);
> buf->cpp = get_format_bpp(dri2_surf->buffer->format);
>
>
> Best regards,
> Tomasz
>
With this change applied the wallpaper is still black
Mauro
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170404/d8099675/attachment.html>
More information about the mesa-dev
mailing list