[Mesa-dev] [PATCH] mesa: Change driver interface for ARB_viewport_array
Ian Romanick
idr at freedesktop.org
Fri Nov 1 14:43:47 PDT 2013
On 11/01/2013 02:04 PM, Courtney Goeltzenleuchter wrote:
>
> On Fri, Nov 1, 2013 at 1:51 PM, Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
>
> On 10/31/2013 08:55 AM, Courtney Goeltzenleuchter wrote:
> > Add the index parameter to the Scissor, Viewport and
> > DepthRange driver methods. Update i965 and Gallium
> > to the change. Index always 0.
>
> I just sent out a patch series that cleans up a bunch of the driver
> implementations of dd_function_table::Viewport. One thing is worth
> noting... no driver uses any of the x, y, width, or height parameters
> passed to Viewport. I think we should just eliminate those parameters
> altogether.
>
> It also appears that every driver that implements
> dd_function_table::DepthRange uses the same (or nearly the same)
> implementation as dd_function_table::Viewport... and the parameters to
> it are also unused. We should eliminate those parameters as well.
>
> For dd_function_table::Scissor, it looks like i830 and i915 both use the
> parameters passed. I'm on the fence whether those should be modified to
> just look in the context (like other drivers do). Do you think that
> would simplify later patches?
>
> When I was writing this it troubled me that I was having to change
> driver code beyond the actual dd interface. It gets uglier when I extend
> the Viewport and Scissor attributes to be arrays. There are a lot of
> driver files touched.
> Isn't the dd interface supposed to help shield the driver from such
> changes? It would make future patches for ARB_viewport_array simpler if
> the drivers _did_ use the parameters from the dd interface. Then I
> wouldn't have to change driver elements as I extend the Mesa side to
> support viewport_arrays and only drivers that supported viewport_arrays
> would have to worry about an array index other than 0. I'd be curious to
> hear what the philosophy is around the dd interface. What would be
> right? Why?
A lot of the functions in dd_function_table are based on the premise
that drivers will emit some sort of commands when a particular GL
function is called. For a variety of reasons, no driver does that for
most functions (including glViewport, glDepthRange, and glScissor). The
alternative would be for the driver to make a copy of the parameters to
generate the command later, but the data is already stored in the
context. That seems rather pointless.
More often, the dd_function_table functions allow core Mesa to signal
the driver driver that something happened... and the driver may need to
do something in response. For DRI2 drivers, the Viewport function is a
good example. DRI2 drivers use that signal as a hint that the window
may have been resized.
Other dd_function_table functions are used by core Mesa to request the
driver create or destroy some resource (e.g., texture storage).
If it weren't for the way nouveau used the DepthRange and Scissor hooks,
I think we could just about get rid of them altogether. I wish that
driver just used the dirty state bits like everyone else. :(
> It would appear that it's common practice to grab the viewport, scissor
> and depth data right out of the gl_context structure. If all the drivers
> are going to get the data straight from the gl_context structure then
> removing the unused parameters seems like a good idea to me. I'll put
> that into the next round.
>
> Appreciate the feedback.
>
>
>
> Also... since this is modifying three separate functions, it should be
> three patches.
>
> > ---
> > src/mesa/drivers/common/driverfuncs.c | 2 +-
> > src/mesa/drivers/dri/i965/brw_context.c | 4 ++--
> > src/mesa/drivers/dri/i965/brw_context.h | 2 +-
> > src/mesa/drivers/dri/swrast/swrast.c | 3 ++-
> > src/mesa/main/dd.h | 10 +++++++---
> > src/mesa/main/scissor.c | 2 +-
> > src/mesa/main/viewport.c | 4 ++--
> > src/mesa/state_tracker/st_cb_viewport.c | 3 ++-
> > 8 files changed, 18 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/mesa/drivers/common/driverfuncs.c
> b/src/mesa/drivers/common/driverfuncs.c
> > index 5faa98a..e45dc0e 100644
> > --- a/src/mesa/drivers/common/driverfuncs.c
> > +++ b/src/mesa/drivers/common/driverfuncs.c
> > @@ -299,7 +299,7 @@ _mesa_init_driver_state(struct gl_context *ctx)
> > ctx->Driver.LogicOpcode(ctx, ctx->Color.LogicOp);
> > ctx->Driver.PointSize(ctx, ctx->Point.Size);
> > ctx->Driver.PolygonStipple(ctx, (const GLubyte *)
> ctx->PolygonStipple);
> > - ctx->Driver.Scissor(ctx, ctx->Scissor.X, ctx->Scissor.Y,
> > + ctx->Driver.Scissor(ctx, 0, ctx->Scissor.X, ctx->Scissor.Y,
> > ctx->Scissor.Width, ctx->Scissor.Height);
> > ctx->Driver.ShadeModel(ctx, ctx->Light.ShadeModel);
> > ctx->Driver.StencilFuncSeparate(ctx, GL_FRONT,
> > diff --git a/src/mesa/drivers/dri/i965/brw_context.c
> b/src/mesa/drivers/dri/i965/brw_context.c
> > index 3880e18..5b4d662d 100644
> > --- a/src/mesa/drivers/dri/i965/brw_context.c
> > +++ b/src/mesa/drivers/dri/i965/brw_context.c
> > @@ -125,13 +125,13 @@ intelGetString(struct gl_context * ctx,
> GLenum name)
> > }
> >
> > static void
> > -intel_viewport(struct gl_context *ctx, GLint x, GLint y, GLsizei
> w, GLsizei h)
> > +intel_viewport(struct gl_context *ctx, GLuint idx, GLint x, GLint
> y, GLsizei w, GLsizei h)
> > {
> > struct brw_context *brw = brw_context(ctx);
> > __DRIcontext *driContext = brw->driContext;
> >
> > if (brw->saved_viewport)
> > - brw->saved_viewport(ctx, x, y, w, h);
> > + brw->saved_viewport(ctx, idx, x, y, w, h);
> >
> > if (_mesa_is_winsys_fbo(ctx->DrawBuffer)) {
> > dri2InvalidateDrawable(driContext->driDrawablePriv);
> > diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> b/src/mesa/drivers/dri/i965/brw_context.h
> > index 3be2138..c261ae8 100644
> > --- a/src/mesa/drivers/dri/i965/brw_context.h
> > +++ b/src/mesa/drivers/dri/i965/brw_context.h
> > @@ -1411,7 +1411,7 @@ struct brw_context
> >
> > __DRIcontext *driContext;
> > struct intel_screen *intelScreen;
> > - void (*saved_viewport)(struct gl_context *ctx,
> > + void (*saved_viewport)(struct gl_context *ctx, GLuint idx,
> > GLint x, GLint y, GLsizei width,
> GLsizei height);
> > };
> >
> > diff --git a/src/mesa/drivers/dri/swrast/swrast.c
> b/src/mesa/drivers/dri/swrast/swrast.c
> > index bfa2efd..ffb1fa0 100644
> > --- a/src/mesa/drivers/dri/swrast/swrast.c
> > +++ b/src/mesa/drivers/dri/swrast/swrast.c
> > @@ -618,7 +618,8 @@ update_state( struct gl_context *ctx, GLuint
> new_state )
> > }
> >
> > static void
> > -viewport(struct gl_context *ctx, GLint x, GLint y, GLsizei w,
> GLsizei h)
> > +viewport(struct gl_context *ctx, GLuint idx,
> > + GLint x, GLint y, GLsizei w, GLsizei h)
> > {
> > struct gl_framebuffer *draw = ctx->WinSysDrawBuffer;
> > struct gl_framebuffer *read = ctx->WinSysReadBuffer;
> > diff --git a/src/mesa/main/dd.h b/src/mesa/main/dd.h
> > index 5011921..7f57a39 100644
> > --- a/src/mesa/main/dd.h
> > +++ b/src/mesa/main/dd.h
> > @@ -479,7 +479,8 @@ struct dd_function_table {
> > /** Enable or disable writing into the depth buffer */
> > void (*DepthMask)(struct gl_context *ctx, GLboolean flag);
> > /** Specify mapping of depth values from NDC to window
> coordinates */
> > - void (*DepthRange)(struct gl_context *ctx, GLclampd nearval,
> GLclampd farval);
> > + void (*DepthRange)(struct gl_context *ctx, GLuint idx,
> > + GLclampd nearval, GLclampd farval);
> > /** Specify the current buffer for writing */
> > void (*DrawBuffer)( struct gl_context *ctx, GLenum buffer );
> > /** Specify the buffers for writing for fragment programs*/
> > @@ -519,7 +520,9 @@ struct dd_function_table {
> > /** Set rasterization mode */
> > void (*RenderMode)(struct gl_context *ctx, GLenum mode );
> > /** Define the scissor box */
> > - void (*Scissor)(struct gl_context *ctx, GLint x, GLint y,
> GLsizei w, GLsizei h);
> > + void (*Scissor)(struct gl_context *ctx, GLuint idx,
> > + GLint x, GLint y,
> > + GLsizei width, GLsizei height);
> > /** Select flat or smooth shading */
> > void (*ShadeModel)(struct gl_context *ctx, GLenum mode);
> > /** OpenGL 2.0 two-sided StencilFunc */
> > @@ -541,7 +544,8 @@ struct dd_function_table {
> > struct gl_texture_object *texObj,
> > GLenum pname, const GLfloat *params);
> > /** Set the viewport */
> > - void (*Viewport)(struct gl_context *ctx, GLint x, GLint y,
> GLsizei w, GLsizei h);
> > + void (*Viewport)(struct gl_context *ctx, GLuint idx,
> > + GLint x, GLint y, GLsizei w, GLsizei h);
> > /*@}*/
> >
> >
> > diff --git a/src/mesa/main/scissor.c b/src/mesa/main/scissor.c
> > index 0eddaa6..4eb6337 100644
> > --- a/src/mesa/main/scissor.c
> > +++ b/src/mesa/main/scissor.c
> > @@ -79,7 +79,7 @@ _mesa_set_scissor(struct gl_context *ctx,
> > ctx->Scissor.Height = height;
> >
> > if (ctx->Driver.Scissor)
> > - ctx->Driver.Scissor( ctx, x, y, width, height );
> > + ctx->Driver.Scissor( ctx, 0, x, y, width, height );
> > }
> >
> >
> > diff --git a/src/mesa/main/viewport.c b/src/mesa/main/viewport.c
> > index 91578ba..5da10ba 100644
> > --- a/src/mesa/main/viewport.c
> > +++ b/src/mesa/main/viewport.c
> > @@ -99,7 +99,7 @@ _mesa_set_viewport(struct gl_context *ctx, GLint
> x, GLint y,
> > /* Many drivers will use this call to check for window size
> changes
> > * and reallocate the z/stencil/accum/etc buffers if needed.
> > */
> > - ctx->Driver.Viewport(ctx, x, y, width, height);
> > + ctx->Driver.Viewport(ctx, 0, x, y, width, height);
> > }
> > }
> >
> > @@ -143,7 +143,7 @@ _mesa_DepthRange(GLclampd nearval, GLclampd
> farval)
> > #endif
> >
> > if (ctx->Driver.DepthRange) {
> > - ctx->Driver.DepthRange(ctx, nearval, farval);
> > + ctx->Driver.DepthRange(ctx, 0, nearval, farval);
> > }
> > }
> >
> > diff --git a/src/mesa/state_tracker/st_cb_viewport.c
> b/src/mesa/state_tracker/st_cb_viewport.c
> > index d654ed6..d48127e 100644
> > --- a/src/mesa/state_tracker/st_cb_viewport.c
> > +++ b/src/mesa/state_tracker/st_cb_viewport.c
> > @@ -48,7 +48,8 @@ st_ws_framebuffer(struct gl_framebuffer *fb)
> > return NULL;
> > }
> >
> > -static void st_viewport(struct gl_context * ctx, GLint x, GLint y,
> > +static void st_viewport(struct gl_context * ctx, GLuint idx,
> > + GLint x, GLint y,
> > GLsizei width, GLsizei height)
> > {
> > struct st_context *st = ctx->st;
> >
> --
> Courtney Goeltzenleuchter
> LunarG
More information about the mesa-dev
mailing list