On 29 October 2013 15:59, Anuj Phogat <anuj.pho...@gmail.com> wrote:
>
>
>
> On Mon, Oct 28, 2013 at 6:48 PM, Paul Berry <stereotype...@gmail.com>wrote:
>
>> On 25 October 2013 16:45, Anuj Phogat <anuj.pho...@gmail.com> wrote:
>>
>>> - Enable GEN7_WM_MSDISPMODE_PERSAMPLE, GEN7_WM_POSOFFSET_SAMPLE,
>>> GEN7_WM_OMASK_TO_RENDER_TARGET as per extension's specification.
>>> - Only enable one of GEN7_WM_8_DISPATCH_ENABLE or
>>> GEN7_WM_16_DISPATCH_ENABLE
>>> when GEN7_WM_MSDISPMODE_PERSAMPLE is enabled. Refer IVB PRM Vol. 2,
>>> Part 1,
>>> Page 288 for details.
>>>
>>> V2: - Add a comment explaining why only SIMD8 mode is enabled with
>>> MSDISPMODE_PERSAMPLE.
>>> - Use shared function _mesa_get_min_invocations_per_fragment().
>>> - Use brw_wm_prog_data variables: uses_pos_offset, uses_omask.
>>> Signed-off-by: Anuj Phogat <anuj.pho...@gmail.com>
>>> ---
>>> src/mesa/drivers/dri/i965/gen7_wm_state.c | 53
>>> +++++++++++++++++++++++++++++--
>>> 1 file changed, 50 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/gen7_wm_state.c
>>> b/src/mesa/drivers/dri/i965/gen7_wm_state.c
>>> index a2046c3..493b107 100644
>>> --- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
>>> +++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
>>> @@ -27,6 +27,7 @@
>>> #include "brw_defines.h"
>>> #include "brw_util.h"
>>> #include "brw_wm.h"
>>> +#include "program/program.h"
>>> #include "program/prog_parameter.h"
>>> #include "program/prog_statevars.h"
>>> #include "intel_batchbuffer.h"
>>> @@ -82,9 +83,13 @@ upload_wm_state(struct brw_context *brw)
>>> GEN7_WM_BARYCENTRIC_INTERPOLATION_MODE_SHIFT;
>>>
>>> /* _NEW_COLOR, _NEW_MULTISAMPLE */
>>> + /* Enable if the pixel shader kernel generates and outputs oMask.
>>> + */
>>> if (fp->program.UsesKill || ctx->Color.AlphaEnabled ||
>>> - ctx->Multisample.SampleAlphaToCoverage)
>>> + ctx->Multisample.SampleAlphaToCoverage ||
>>> + brw->wm.prog_data->uses_omask) {
>>> dw1 |= GEN7_WM_KILL_ENABLE;
>>> + }
>>>
>>> /* _NEW_BUFFERS */
>>> if (brw_color_buffer_write_enabled(brw) || writes_depth ||
>>> @@ -97,7 +102,11 @@ upload_wm_state(struct brw_context *brw)
>>> dw1 |= GEN7_WM_MSRAST_ON_PATTERN;
>>> else
>>> dw1 |= GEN7_WM_MSRAST_OFF_PIXEL;
>>> - dw2 |= GEN7_WM_MSDISPMODE_PERPIXEL;
>>> +
>>> + if (_mesa_get_min_invocations_per_fragment(ctx,
>>> brw->fragment_program) > 1)
>>> + dw2 |= GEN7_WM_MSDISPMODE_PERSAMPLE;
>>> + else
>>> + dw2 |= GEN7_WM_MSDISPMODE_PERPIXEL;
>>> } else {
>>> dw1 |= GEN7_WM_MSRAST_OFF_PIXEL;
>>> dw2 |= GEN7_WM_MSDISPMODE_PERSAMPLE;
>>> @@ -169,6 +178,32 @@ upload_ps_state(struct brw_context *brw)
>>> if (brw->wm.prog_data->nr_params > 0)
>>> dw4 |= GEN7_PS_PUSH_CONSTANT_ENABLE;
>>>
>>> + /* From the IVB PRM, volume 2 part 1, page 287:
>>> + * "This bit is inserted in the PS payload header and made available
>>> to
>>> + * the DataPort (either via the message header or via header bypass)
>>> to
>>> + * indicate that oMask data (one or two phases) is included in Render
>>> + * Target Write messages. If present, the oMask data is used to mask
>>> off
>>> + * samples."
>>> + */
>>> + if (brw->wm.prog_data->uses_omask)
>>> + dw4 |= GEN7_PS_OMASK_TO_RENDER_TARGET;
>>> +
>>> + /* From the IVB PRM, volume 2 part 1, page 287:
>>> + * "If the PS kernel does not need the Position XY Offsets to
>>> + * compute a Position Value, then this field should be programmed
>>> + * to POSOFFSET_NONE."
>>> + * "SW Recommendation: If the PS kernel needs the Position Offsets
>>> + * to compute a Position XY value, this field should match Position
>>> + * ZW Interpolation Mode to ensure a consistent position.xyzw
>>> + * computation."
>>> + * We only require XY sample offsets. So, this recommendation doesn't
>>> + * look useful at the moment. We might need this in future.
>>> + */
>>> + if (brw->wm.prog_data->uses_pos_offset)
>>> + dw4 |= GEN7_PS_POSOFFSET_SAMPLE;
>>> + else
>>> + dw4 |= GEN7_PS_POSOFFSET_NONE;
>>> +
>>> /* CACHE_NEW_WM_PROG | _NEW_COLOR
>>> *
>>> * The hardware wedges if you have this bit set but don't turn on
>>> any dual
>>> @@ -184,8 +219,20 @@ upload_ps_state(struct brw_context *brw)
>>> if (brw->wm.prog_data->num_varying_inputs != 0)
>>> dw4 |= GEN7_PS_ATTRIBUTE_ENABLE;
>>>
>>> + /* In case of non 1x (i.e 4x, 8x) multisampling with
>>> MSDISPMODE_PERSAMPLE,
>>> + * only one of SIMD8 and SIMD16 should be enabled. So, we have two
>>> options
>>> + * in that case:
>>> + * 'SIMD8 only' dispatch: allowed on gen7.
>>> + * 'SIMD16 only' dispatch: allowed on gen7 except when in PERSAMPLE
>>> mode
>>> + * with number of multisamples >= 8.
>>> + * TODO: Currently we enable 'SIMD8 only' dispatch in above
>>> mentioned case.
>>> + * Make changes to allow 'SIMD16 only' dispatch for
>>> multisamples < 8.
>>>
>>
>> I believe this is incorrect (see my comment on patch 10/12). On Gen7, we
>> can enable SIMD8 or SIMD16 or both. (I'm fairly certain about this because
>> blorp enables SIMD16 mode with 8x PERSAMPLE dispatch, and it's been working
>> fine since May 2012).
>>
> We had this discussion on my earlier patch for Gen6. We reached a
> conclusion that on Gen7 with non 1x persample, we can enable either SIMD8
> or SIMD16.
>
> The only reason we are not enabling SIMD16 yet is because you had some
>> concerns that the code generation for SIMD16 might not be correct.
>>
> Right. I am also getting GPU hangs with SIMD16 only dispatch. I'll look
> in to it after pushing this series.
>
>>
>> I think the right way to deal with this is to drop this coment (and the
>> remainder of the patch), and instead, disable SIMD16 in the same way we do
>> for the other features that we don't have SIMD16 support for yet: by
>> calling fail() in brw_fs_visitor.cpp.
>>
>> That way (a) we won't generate a 16-wide shader that will never get used,
>> and (b) later, when we go looking for reasons why SIMD16 shaders aren't
>> getting generated, we will only have to consider one reason (shaders
>> failing SIMD16 compilation) instead of two.
>>
> sounds better. I'll call fail() if (dispatch_width == 16 &&
> _mesa_get_min_invocations_per_fragment(ctx, fp) > 1)
>
That won't work. You can't call _mesa_get_min_invocations_per_fragment()
from inside the visitor because it examines GL state. Inside the visitor
the only safe way to access GL state is through the key.
I think the right thing to do is to add:
if (dispatch_width == 16)
fail("...");
to whatever parts of the visitor you aren't confident will work properly in
SIMD16.
>
>> With that change, this patch is:
>>
>> Reviewed-by: Paul Berry <stereotype...@gmail.com>
>>
>>
>>> + */
>>> dw4 |= GEN7_PS_8_DISPATCH_ENABLE;
>>> - if (brw->wm.prog_data->prog_offset_16)
>>> +
>>> + if (brw->wm.prog_data->prog_offset_16 &&
>>> + !(_mesa_get_min_invocations_per_fragment(ctx,
>>> + brw->fragment_program )
>>> > 1))
>>> dw4 |= GEN7_PS_16_DISPATCH_ENABLE;
>>>
>>> dw5 |= (brw->wm.prog_data->first_curbe_grf <<
>>> --
>>> 1.8.1.4
>>>
>>>
>>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
