[Mesa-dev] [v3 PATCH 06/10] i965: Use _mesa_geometry_ functions appropriately
kevin.rogovin at intel.com
Thu May 21 16:44:49 PDT 2015
>> src/mesa/drivers/dri/i965/brw_misc_state.c | 10 +++++++---
> > src/mesa/drivers/dri/i965/brw_sf_state.c | 8 ++++++++
> As is this?
brw_misc_state.c is active for all Gens. The change to brw_sf_state.c is to add a comment warning that using gl_framebuffer::Width and so on is only ok if _HasAttachments is false.
> OUT_BATCH(_3DSTATE_DRAWING_RECTANGLE << 16 | (4 - 2));
> OUT_BATCH(0); /* xmin, ymin */
> - OUT_BATCH(((ctx->DrawBuffer->Width - 1) & 0xffff) |
> - ((ctx->DrawBuffer->Height - 1) << 16));
> + OUT_BATCH(((fb_width - 1) & 0xffff) |
> + ((fb_height - 1) << 16));
> Remove the tab on this line and indent it properly while we're changing it.
Actually it fits on one line, so that will be the change :)
> + /* Accessing the fields Width and Height of
> + * gl_framebuffer to produce the values to
> + * program the viewport and scissor is fine
> + * as long as the gl_framebuffer has atleast
> + * one attachment.
> Line wrapping...
> struct brw_state_flags state = brw->state.pipelines[pipeline];
> + int fb_samples = (int)_mesa_geometric_samples(ctx->DrawBuffer);
> The cast looks strange
Is there a spacing missing in the cast? or is it strange because of the types?
> These casts look weird. (Happens elsewhere in this patch too).
> Assuming brw_context::num_samples doesn't need to be signed, I'd be
> inclined to change its type to unsigned and remove the casts. Grepping
> for 'num_samples = ' shows some other instances of us implicitly
> converting unsigned <-> int.
I was hesitant to be changing types in the patch series. I wanted to minimize the changes and try to keep it consistent with what is left unchanged. If the types should be unsigned int, I can do that, but I would like to hear the opinions of others too.
> Extra new line.
More information about the mesa-dev