<div dir="ltr"><div> </div><div class="gmail_extra"><div class="gmail_quote">On Fri, Nov 1, 2013 at 1:51 PM, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">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. </div>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? </blockquote><div> </div><div>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. </div> <div>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?</div> <div> </div><div>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.</div> <div> </div><div>Appreciate the feedback.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> Also... since this is modifying three separate functions, it should be three patches. <div class=""><div class="h5"> > --- > 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; > </div></div></blockquote></div> <div> </div>-- <div dir="ltr">Courtney Goeltzenleuchter <div>LunarG</div><div><img src="http://media.lunarg.com/wp-content/themes/LunarG/images/logo.png" width="96" height="65"> </div></div> </div></div>