[Mesa-dev] [PATCH] mesa: Prevent classic swrast crash on a surfaceless context v2.

[Emil Velikov]

On 2019/05/27, Mathias.Froehlich at gmx.net wrote:
> From: Mathias Fröhlich <mathias.froehlich at web.de>
> 
> Hi Emil,
> 
> thanks for that hint to look at _mesa_get_incomplete_framebuffer.
> That one seems definitely more appropriate!
> 
> Though, I miss a bit the idea how I can create either a sensible
> helper function for that task or how I can create something above
> in the call stack to the MakeCurrent call that already catches
> this case. Since that incomplete framebuffer is a mesa side thing I
> cannot easily pull that above the __DriverAPIRec::MakeCurrent call.
> But really putting those hand full lines of code into a helper does
> as well not gain much. So I implemented that for the swrast case
> directly.
> 
> 
For the patch:
Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

Here is a quick and dirty example. Feel free to follow-up if you like
the idea.

Note: it also updates/considers the fact that only one of the two
drawables can be NULL. Technically a bugfix - not sure if many tests
flex that code-path.

diff --git a/src/mesa/drivers/dri/i915/intel_context.c b/src/mesa/drivers/dri/i915/intel_context.c
index aa3175816cf..035abe13ff8 100644
--- a/src/mesa/drivers/dri/i915/intel_context.c
+++ b/src/mesa/drivers/dri/i915/intel_context.c
@@ -617,45 +617,62 @@ intelUnbindContext(__DRIcontext * driContextPriv)
    return true;
 }
 
+/* XXX: make me public - aka move to src/mesa/ or sr/mesa/main even */
+/* when TRUE is returned read/writeFb are set. on FALSE we should call make_current(null...) */
+bool
+make_current_setup(__DRIcontext * driContextPriv,
+                 __DRIdrawable * driDrawPriv,
+                 __DRIdrawable * driReadPriv,
+                struct **gl_framebuffer readFb,
+                struct **gl_framebuffer writeFb)
+{
+   if (!driContextPriv)
+      return false;
+
+   struct *gl_framebuffer incomplete = _mesa_get_incomplete_framebuffer();
+
+   if (driReadPriv == NULL) {
+      *readFb = incomplete;
+   } else {
+       *readFb = driReadPriv->driverPrivate;
+       driContextPriv->dri2.read_stamp = driReadPriv->dri2.stamp - 1;
+    }
+
+   if (driDrawPriv == NULL) {
+       *writeFb = incomplete;
+   } else {
+       *writeFb = driDrawPriv->driverPrivate;
+       driContextPriv->dri2.draw_stamp = driDrawPriv->dri2.stamp - 1;
+   }
+
+   return true;
+}
+
 GLboolean
 intelMakeCurrent(__DRIcontext * driContextPriv,
                  __DRIdrawable * driDrawPriv,
                  __DRIdrawable * driReadPriv)
 {
    struct intel_context *intel;
+   struct gl_framebuffer *fb, *readFb;
 
-   if (driContextPriv)
-      intel = (struct intel_context *) driContextPriv->driverPrivate;
-   else
-      intel = NULL;
-
-   if (driContextPriv) {
-      struct gl_context *ctx = &intel->ctx;
-      struct gl_framebuffer *fb, *readFb;
-      
-      if (driDrawPriv == NULL && driReadPriv == NULL) {
-	 fb = _mesa_get_incomplete_framebuffer();
-	 readFb = _mesa_get_incomplete_framebuffer();
-      } else {
-	 fb = driDrawPriv->driverPrivate;
-	 readFb = driReadPriv->driverPrivate;
-	 driContextPriv->dri2.draw_stamp = driDrawPriv->dri2.stamp - 1;
-	 driContextPriv->dri2.read_stamp = driReadPriv->dri2.stamp - 1;
-      }
-
-      intel_prepare_render(intel);
-      _mesa_make_current(ctx, fb, readFb);
-
-      /* We do this in intel_prepare_render() too, but intel->ctx.DrawBuffer
-       * is NULL at that point.  We can't call _mesa_makecurrent()
-       * first, since we need the buffer size for the initial
-       * viewport.  So just call intel_draw_buffer() again here. */
-      intel_draw_buffer(ctx);
-   }
-   else {
+   if (!make_current_setup(driContextPriv, driDrawPriv, driReadPriv, &readFb, &fb)) {
       _mesa_make_current(NULL, NULL, NULL);
+      return true;
    }
 
+   intel = (struct intel_context *) driContextPriv->driverPrivate;
+   struct gl_context *ctx = &intel->ctx;
+
+   intel_prepare_render(intel);
+   _mesa_make_current(ctx, fb, readFb);
+
+   /* We do this in intel_prepare_render() too, but intel->ctx.DrawBuffer
+    * is NULL at that point.  We can't call _mesa_makecurrent()
+    * first, since we need the buffer size for the initial
+    * viewport.  So just call intel_draw_buffer() again here. */
+   intel_draw_buffer(ctx);
+
    return true;
 }
 

> So, to mention, I sent that change including our egl device code
> with some unrelated egl device fixes through intels CI and did not
> get regressions.
>
Thanks, having a look. Too many fires going at the same time :-\
 
-Emil