[Mesa-dev] [PATCH] mesa: Change driver interface for ARB_viewport_array
Courtney Goeltzenleuchter
courtney at lunarg.com
Fri Nov 1 14:04:46 PDT 2013
On Fri, Nov 1, 2013 at 1:51 PM, Ian Romanick <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?
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131101/4166a2c1/attachment.html>
More information about the mesa-dev
mailing list