On Tue, Oct 29, 2013 at 4:31 PM, Paul Berry <stereotype...@gmail.com> wrote:
> 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.
>
right.
> 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.
>
Yes. But that only covers the per sample shading enabled due to gl_SampleID
and gl_SamplePosition.
It still doesn't cover the case when there's no gl_SampleID or
gl_SamplePosition in fragment shader but GL_SAMPLE_SHADING is enabled.
To disable simd16 for this case I either need to keep the relevant changes
I made in gen7_wm_state.c
or
may be set simd16_instructions = null in brw_wm_fs_emit() when
GL_SAMPLE_SHADING is enabled. It should also be safe to
call _mesa_get_min_invocations_per_fragment() in brw_wm_fs_emit(). So, we
can take care of all the cases of sample shading enabled at the same place?
>
>>> 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
