[Mesa-dev] [PATCH] i965/msaa: Only do multisample rasterization if GL_MULTISAMPLE enabled.
Paul Berry
stereotype441 at gmail.com
Tue Jun 19 11:27:17 PDT 2012
On 19 June 2012 10:29, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On 06/19/2012 08:16 AM, Paul Berry wrote:
>
>> From the GL 3.0 spec (p.116):
>>
>> "Multisample rasterization is enabled or disabled by calling
>> Enable or Disable with the symbolic constant MULTISAMPLE."
>>
>> Elsewhere in the spec, where multisample rasterization is described
>> (sections 3.4.3, 3.5.4, and 3.6.6), the following text is consistently
>> used:
>>
>> "If MULTISAMPLE is enabled, and the value of SAMPLE_BUFFERS is
>> one, then..."
>>
>> So, in other words, disabling GL_MULTISAMPLE should prevent
>> multisample rasterization from occurring, even if the draw framebuffer
>> is multisampled. This patch implements that behaviour by setting the
>> WM and SF stage's "multisample rasterization mode" to
>> MSRAST_ON_PATTERN only when the draw framebuffer is multisampled *and*
>> GL_MULTISAMPLE is enabled.
>> ---
>> src/mesa/drivers/dri/i965/**gen6_sf_state.c | 10 ++++++----
>> src/mesa/drivers/dri/i965/**gen6_wm_state.c | 15 ++++++++++-----
>> src/mesa/drivers/dri/i965/**gen7_sf_state.c | 10 ++++++----
>> src/mesa/drivers/dri/i965/**gen7_wm_state.c | 15 ++++++++++-----
>> 4 files changed, 32 insertions(+), 18 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/**gen6_sf_state.c
>> b/src/mesa/drivers/dri/i965/**gen6_sf_state.c
>> index e0aaa90..aeed369 100644
>> --- a/src/mesa/drivers/dri/i965/**gen6_sf_state.c
>> +++ b/src/mesa/drivers/dri/i965/**gen6_sf_state.c
>> @@ -122,9 +122,9 @@ upload_sf_state(struct brw_context *brw)
>> int i;
>> /* _NEW_BUFFER */
>> bool render_to_fbo = _mesa_is_user_fbo(brw->intel.**ctx.DrawBuffer);
>> - bool multisampled = false;
>> + bool multisampled_fbo = false;
>> if (ctx->DrawBuffer->_**ColorDrawBuffers[0])
>> - multisampled = ctx->DrawBuffer->_**ColorDrawBuffers[0]->**NumSamples
>> > 0;
>> + multisampled_fbo = ctx->DrawBuffer->_**ColorDrawBuffers[0]->**NumSamples
>> > 0;
>>
>> int attr = 0, input_index = 0;
>> int urb_entry_read_offset = 1;
>> @@ -242,7 +242,8 @@ upload_sf_state(struct brw_context *brw)
>> dw3 |= GEN6_SF_LINE_AA_MODE_TRUE;
>> dw3 |= GEN6_SF_LINE_END_CAP_WIDTH_1_**0;
>> }
>> - if (multisampled)
>> + /* _NEW_MULTISAMPLE */
>> + if (multisampled_fbo && ctx->Multisample.Enabled)
>> dw3 |= GEN6_SF_MSRAST_ON_PATTERN;
>>
>> /* _NEW_PROGRAM | _NEW_POINT */
>> @@ -349,7 +350,8 @@ const struct brw_tracked_state gen6_sf_state = {
>> _NEW_LINE |
>> _NEW_SCISSOR |
>> _NEW_BUFFERS |
>> - _NEW_POINT),
>> + _NEW_POINT |
>> + _NEW_MULTISAMPLE),
>>
>
> Whitespace errors here. I know Mesa isn't terribly consistent w.r.t. tabs
> or spaces, but in general these files use 3 space indent with 8 space tabs.
> Please use tabs to match the surrounding lines.
I'm reluctant to add fuel to coding convention debates, but I really don't
think "follow whatever tab/space style the surrounding code follows" is a
reasonable coding convention to enforce. Most editors do automatic
indentation of C/C++ code, and can be easily configured to either globally
use tabs or globally not use tabs. They can't be easily configured to
follow the tab/space style of the surrounding code, which means that if
this is our coding convention, it's going to be an extra developer burden.
Also, our documentation says we prefer spaces to tabs. From
docs/devinfo.html:
Here's the GNU indent command which will best approximate my
preferred style: (Note that it won't format switch statements
in the preferred way)
indent -br -i3 -npcs --no-tabs infile.c -o outfile.c
That "--no-tabs" option causes indent to use spaces, not tabs.
And the toplevel .emacs-dirvars file (which emacs automatically consults if
the dirvars package is in use) configures emacs to automatically indent
using spaces, not tabs, using this line:
indent-tabs-mode: nil
When I started working on Mesa code over a year ago I configured my editor
to insert spaces rather than tabs (on Chad's recommendation), and the only
time people have found fault with my whitespace has been situations like
this, where I was editing code that didn't follow our documented
convention. So I really think there is a good precedent for spaces being
our preferred indentation style, in spite of the fact that Mesa code is
very inconsistent about following it.
My preference would be to leave the patch as is. If that's not acceptable,
I'd propose changing tabs to spaces in the parts of the code that I'm
modifying, so that at least this patch moves us toward consistent use of
spaces instead of tabs, rather than away from it.
>
>
> .brw = (BRW_NEW_CONTEXT |
>> BRW_NEW_FRAGMENT_PROGRAM),
>> .cache = CACHE_NEW_VS_PROG
>> diff --git a/src/mesa/drivers/dri/i965/**gen6_wm_state.c
>> b/src/mesa/drivers/dri/i965/**gen6_wm_state.c
>> index cba2a57..662435e 100644
>> --- a/src/mesa/drivers/dri/i965/**gen6_wm_state.c
>> +++ b/src/mesa/drivers/dri/i965/**gen6_wm_state.c
>> @@ -98,11 +98,11 @@ upload_wm_state(struct brw_context *brw)
>> const struct brw_fragment_program *fp =
>> brw_fragment_program_const(**brw->fragment_program);
>> uint32_t dw2, dw4, dw5, dw6;
>> - bool multisampled = false;
>> + bool multisampled_fbo = false;
>>
>> /* _NEW_BUFFERS */
>> if (ctx->DrawBuffer->_**ColorDrawBuffers[0])
>> - multisampled = ctx->DrawBuffer->_**ColorDrawBuffers[0]->**NumSamples
>> > 0;
>> + multisampled_fbo = ctx->DrawBuffer->_**ColorDrawBuffers[0]->**NumSamples
>> > 0;
>>
>> /* CACHE_NEW_WM_PROG */
>> if (brw->wm.prog_data->nr_params == 0) {
>> @@ -197,8 +197,12 @@ upload_wm_state(struct brw_context *brw)
>>
>> dw6 |= _mesa_bitcount_64(brw->**fragment_program->Base.**InputsRead)
>> <<
>> GEN6_WM_NUM_SF_OUTPUTS_SHIFT;
>> - if (multisampled) {
>> - dw6 |= GEN6_WM_MSRAST_ON_PATTERN;
>> + if (multisampled_fbo) {
>> + /* _NEW_MULTISAMPLE */
>> + if (ctx->Multisample.Enabled)
>> + dw6 |= GEN6_WM_MSRAST_ON_PATTERN;
>> + else
>> + dw6 |= GEN6_WM_MSRAST_OFF_PIXEL;
>>
>
> Whitespace errors.
>
>
> dw6 |= GEN6_WM_MSDISPMODE_PERPIXEL;
>> } else {
>> dw6 |= GEN6_WM_MSRAST_OFF_PIXEL;
>> @@ -230,7 +234,8 @@ const struct brw_tracked_state gen6_wm_state = {
>> _NEW_COLOR |
>> _NEW_BUFFERS |
>> _NEW_PROGRAM_CONSTANTS |
>> - _NEW_POLYGON),
>> + _NEW_POLYGON |
>> + _NEW_MULTISAMPLE),
>> .brw = (BRW_NEW_FRAGMENT_PROGRAM |
>> BRW_NEW_BATCH),
>> .cache = (CACHE_NEW_SAMPLER |
>> diff --git a/src/mesa/drivers/dri/i965/**gen7_sf_state.c
>> b/src/mesa/drivers/dri/i965/**gen7_sf_state.c
>> index 8a6c09b..b1fe654 100644
>> --- a/src/mesa/drivers/dri/i965/**gen7_sf_state.c
>> +++ b/src/mesa/drivers/dri/i965/**gen7_sf_state.c
>> @@ -161,9 +161,9 @@ upload_sf_state(struct brw_context *brw)
>> float point_size;
>> /* _NEW_BUFFERS */
>> bool render_to_fbo = _mesa_is_user_fbo(brw->intel.**ctx.DrawBuffer);
>> - bool multisampled = false;
>> + bool multisampled_fbo = false;
>> if (ctx->DrawBuffer->_**ColorDrawBuffers[0])
>> - multisampled = ctx->DrawBuffer->_**ColorDrawBuffers[0]->**NumSamples
>> > 0;
>> + multisampled_fbo = ctx->DrawBuffer->_**ColorDrawBuffers[0]->**NumSamples
>> > 0;
>>
>> dw1 = GEN6_SF_STATISTICS_ENABLE |
>> GEN6_SF_VIEWPORT_TRANSFORM_**ENABLE;
>> @@ -261,7 +261,8 @@ upload_sf_state(struct brw_context *brw)
>> if (ctx->Line.StippleFlag && intel->is_haswell) {
>> dw2 |= HSW_SF_LINE_STIPPLE_ENABLE;
>> }
>> - if (multisampled)
>> + /* _NEW_MULTISAMPLE */
>> + if (multisampled_fbo && ctx->Multisample.Enabled)
>> dw2 |= GEN6_SF_MSRAST_ON_PATTERN;
>>
>> /* FINISHME: Last Pixel Enable? Vertex Sub Pixel Precision Select?
>> @@ -309,7 +310,8 @@ const struct brw_tracked_state gen7_sf_state = {
>> _NEW_LINE |
>> _NEW_SCISSOR |
>> _NEW_BUFFERS |
>> - _NEW_POINT),
>> + _NEW_POINT |
>> + _NEW_MULTISAMPLE),
>>
>
> Whitespace.
>
>
> .brw = BRW_NEW_CONTEXT,
>> .cache = CACHE_NEW_VS_PROG
>> },
>> diff --git a/src/mesa/drivers/dri/i965/**gen7_wm_state.c
>> b/src/mesa/drivers/dri/i965/**gen7_wm_state.c
>> index f46e3f2..45c8e46 100644
>> --- a/src/mesa/drivers/dri/i965/**gen7_wm_state.c
>> +++ b/src/mesa/drivers/dri/i965/**gen7_wm_state.c
>> @@ -39,12 +39,12 @@ upload_wm_state(struct brw_context *brw)
>> const struct brw_fragment_program *fp =
>> brw_fragment_program_const(**brw->fragment_program);
>> bool writes_depth = false;
>> - bool multisampled = false;
>> + bool multisampled_fbo = false;
>> uint32_t dw1, dw2;
>>
>> /* _NEW_BUFFERS */
>> if (ctx->DrawBuffer->_**ColorDrawBuffers[0])
>> - multisampled = ctx->DrawBuffer->_**ColorDrawBuffers[0]->**NumSamples
>> > 0;
>> + multisampled_fbo = ctx->DrawBuffer->_**ColorDrawBuffers[0]->**NumSamples
>> > 0;
>>
>> dw1 = dw2 = 0;
>> dw1 |= GEN7_WM_STATISTICS_ENABLE;
>> @@ -79,8 +79,12 @@ upload_wm_state(struct brw_context *brw)
>> dw1 & GEN7_WM_KILL_ENABLE) {
>> dw1 |= GEN7_WM_DISPATCH_ENABLE;
>> }
>> - if (multisampled) {
>> - dw1 |= GEN7_WM_MSRAST_ON_PATTERN;
>> + if (multisampled_fbo) {
>> + /* _NEW_MULTISAMPLE */
>> + if (ctx->Multisample.Enabled)
>> + dw1 |= GEN7_WM_MSRAST_ON_PATTERN;
>> + else
>> + dw1 |= GEN7_WM_MSRAST_OFF_PIXEL;
>>
>
> Whitespace.
>
>
> dw2 |= GEN7_WM_MSDISPMODE_PERPIXEL;
>> } else {
>> dw1 |= GEN7_WM_MSRAST_OFF_PIXEL;
>> @@ -97,7 +101,8 @@ upload_wm_state(struct brw_context *brw)
>> const struct brw_tracked_state gen7_wm_state = {
>> .dirty = {
>> .mesa = (_NEW_LINE | _NEW_POLYGON |
>> - _NEW_COLOR | _NEW_BUFFERS),
>> + _NEW_COLOR | _NEW_BUFFERS |
>> + _NEW_MULTISAMPLE),
>>
>
> And whitespace.
>
>
> .brw = (BRW_NEW_FRAGMENT_PROGRAM |
>> BRW_NEW_BATCH),
>> .cache = CACHE_NEW_WM_PROG,
>>
>>
> Otherwise, the patch looks good, so:
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120619/b9da35fa/attachment.html>
More information about the mesa-dev
mailing list