[Mesa-dev] [v3 PATCH 06/10] i965: Use _mesa_geometry_ functions appropriately

[Rogovin, Kevin]

>>  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.

>     BEGIN_BATCH(4);
>     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...

OK.

>     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.
OK.