[Mesa-dev] [PATCH 3/4] tnl: Maintain the _WindowMap matrix in TNLcontext v2.

[Brian Paul]

On 08/07/2015 08:28 AM, Ilia Mirkin wrote:
> On Fri, Aug 7, 2015 at 2:46 AM, Mathias Fröhlich
> <Mathias.Froehlich at gmx.net <mailto:Mathias.Froehlich at gmx.net>> wrote:
>
>     __
>
>     Hi,
>
>     On Thursday, August 06, 2015 12:32:18 Ilia Mirkin wrote:
>
>     > > @@ -182,6 +187,13 @@ _tnl_InvalidateState( struct gl_context *ctx, GLuint new_state )
>
>     > >           }
>
>     > >        }
>
>     > >     }
>
>     > > +
>
>     > > +   if (new_state & (_NEW_VIEWPORT | _NEW_BUFFERS)) {
>
>     > > +      double scale[3], translate[3];
>
>     > > +      _mesa_get_viewport_xform(ctx, 0, scale, translate);
>
>     > > +      _math_matrix_viewport(&tnl->_WindowMap, scale, translate,
>
>     > > +                            ctx->DrawBuffer->_DepthMaxF);
>
>     >
>
>     > This appears to crash nouveau_vieux on startup. See
>
>     >https://bugs.freedesktop.org/show_bug.cgi?id=91570
>     <https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.freedesktop.org_show-5Fbug.cgi-3Fid-3D91570&d=BQMFaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=T0t4QG7chq2ZwJo6wilkFznRSFy-8uDKartPGbomVj8&m=O9wy0sfwhXEUP8EenUsLpI_E1YZOhJvq5EMm7zYPgmo&s=aGSt6eWYwal-8XD-AYEBy97MYdp1gTN4KJu3HJMIPS8&e=>
>     . I suspect that
>
>     > ctx->DrawBuffer is null. What's the proper way to handle that
>
>     > situation -- predicate on a drawbuffer being bound? Or make sure that
>
>     > something is bound to the drawbuffer before we call tnl_wakeup?
>
>     >
>
>     It looks like nouveau_vieux uses _tnl_InvalidateState for early
>     initialization.
>
>     I assume that _mesa_make_current is called later on to set the
>     DrawBuffer
>
>     and there the _NEW_BUFFERS flag is set. So, when
>     _tnl_InvalidateState is called
>
>     while validating state before the first draw and past a bound draw
>     buffer, we should
>
>     get the desired update on the _WindowMap matrix.
>
>     So just checking for the presence of the DrawBuffer before grabbing data
>
>     from it seems to be safe.
>
>     The aim of the change was to move the _WindowMap matrix out of the
>     non tnl drivers
>
>     and track it in the tnl module where it is exclusively used today.
>
>     At the higher mesa level we did/do not invalidate DrawBuffer related
>     state that early.
>
>     One alternative I can see is to call _tnl_InvalidateState without
>     the _NEW_VIEWPORT and
>
>     _NEW_BUFFERS flag set instead of _tnl_wakeup in the two nouveau
>     early initialization
>
>     code paths in nouveau_swtnl_t.c and nv04_render.c. That should also
>     bring us back to
>
>     the original behavior.
>
>
> That seems like a bit of a hack. If you think it's OK, I'm just going to
> introduce the DrawBuffer check. Brian, does that seem OK to you? I'm
> mostly unfamiliar with the TnL module, its APIs, and when/how they're
> supposed to be used.

I haven't look at the tnl validation code in ages.  Off-hand, I'd say 
adding the null ctx->DrawBuffer check in _tnl_InvalidateState() is 
probably the simplest/best thing to do.

If at a later point ctx->DrawBuffer becomes non-null we should get a 
_NEW_BUFFERS signal and revalidate the matrix anyway.

I guess one other possibility is something like:

if (ctx->DrawBuffer)
    depthMaxF = ctx->DrawBuffer->_DepthMaxF;
else
    depthMaxF = _mesa_compute_depth_max_from_bits(ctx->Visual.depthBits);

_math_matrix_viewport(..., depthMaxF);


where _mesa_compute_depth_max_from_bits() would be a redo of 
compute_depth_max().



> But nouveau_vieux calls tnl_wakeup right in its
> context creation pass. nv04/nv05 don't have hwtnl at all, nv10+ have it,
> but it has various limitations, with which we end up using swtnl.
>
>    -ilia

Re:

> it seems like _tnl_wakeup is meant for "oh hey, I shut you down and
> haven't been sending you updates, but I want to use you again, so
> please update your stuff to match reality"

I believe that interpretation is correct.

-Brian