On 11/01/2013 02:43 PM, Ian Romanick wrote:
> On 11/01/2013 02:04 PM, Courtney Goeltzenleuchter wrote:
>>
>> On Fri, Nov 1, 2013 at 1:51 PM, Ian Romanick <i...@freedesktop.org
>> <mailto:i...@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.
As a side note... this is part of the reason that small, self-contained
patches are preferred. It makes it much easier to review a bunch of
mechanical changes in 7 different drivers...
>> 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
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
