[Mesa-dev] [PATCH 7/8] i965/gen7: Enable the features required for GL_ARB_sample_shading
Anuj Phogat
anuj.phogat at gmail.com
Thu Oct 24 00:33:26 CEST 2013
On Sun, Oct 20, 2013 at 7:37 AM, Paul Berry <stereotype441 at gmail.com> wrote:
> On 16 October 2013 15:20, Ian Romanick <idr at freedesktop.org> wrote:
>>
>> On 10/16/2013 02:57 PM, Anuj Phogat wrote:
>> > On Tue, Oct 15, 2013 at 3:48 PM, Kenneth Graunke <kenneth at whitecape.org>
>> > wrote:
>> >> On 10/14/2013 10:12 AM, Anuj Phogat wrote:
>> >>> - Enable GEN7_WM_MSDISPMODE_PERSAMPLE, GEN7_WM_POSOFFSET_SAMPLE,
>> >>> GEN7_WM_OMASK_TO_RENDER_TARGET as per extension's specification.
>> >>> - Don't enable GEN7_WM_16_DISPATCH_ENABLE when
>> >>> GEN7_WM_MSDISPMODE_PERSAMPLE
>> >>> is enabled. Refer IVB PRM Vol. 2, Part 1, Page 288 for details.
>> >>>
>> >>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>> >>> ---
>> >>> src/mesa/drivers/dri/i965/gen7_wm_state.c | 70
>> >>> +++++++++++++++++++++++++++++--
>> >>> 1 file changed, 67 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..0267e0e 100644
>> >>> --- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
>> >>> +++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
>> >>> @@ -82,9 +82,15 @@ 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 ||
>> >>> + (ctx->Multisample.SampleShading &&
>> >>> + (fp->program.Base.OutputsWritten &
>> >>> + BITFIELD64_BIT(FRAG_RESULT_SAMPLE_MASK)))) {
>> >>> dw1 |= GEN7_WM_KILL_ENABLE;
>> >>> + }
>> >>>
>> >>> /* _NEW_BUFFERS */
>> >>> if (brw_color_buffer_write_enabled(brw) || writes_depth ||
>> >>> @@ -97,7 +103,25 @@ 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;
>> >>> + /* From arb_sample_shading specification:
>> >>> + * "If MULTISAMPLE or SAMPLE_SHADING_ARB is disabled, sample
>> >>> shading
>> >>> + * has no effect."
>> >>> + *
>> >>> + * "Using gl_SampleID in a fragment shader causes the entire
>> >>> shader
>> >>> + * to be evaluated per-sample."
>> >>> + * "Using gl_SamplePosition in a fragment shader causes the
>> >>> entire
>> >>> + * shader to be evaluated per-sample."
>> >>> + *
>> >>> + * I interprate the above four lines as enable the sample
>> >>> shading
>> >>> + * if fragment shader uses gl_SampleID or gl_SamplePosition.
>> >>> + */
>> >>> + if (ctx->Multisample.Enabled &&
>> >>> + (ctx->Multisample.SampleShading ||
>> >>> + brw->fragment_program->Base.InputsRead &
>> >>> VARYING_BIT_SAMPLE_ID ||
>> >>> + brw->fragment_program->Base.InputsRead &
>> >>> VARYING_BIT_SAMPLE_POS))
>> >>> + dw2 |= GEN7_WM_MSDISPMODE_PERSAMPLE;
>> >>> + else
>> >>> + dw2 |= GEN7_WM_MSDISPMODE_PERPIXEL;
>> >>> } else {
>> >>> dw1 |= GEN7_WM_MSRAST_OFF_PIXEL;
>> >>> dw2 |= GEN7_WM_MSDISPMODE_PERSAMPLE;
>> >>> @@ -129,6 +153,8 @@ upload_ps_state(struct brw_context *brw)
>> >>> uint32_t dw2, dw4, dw5;
>> >>> const int max_threads_shift = brw->is_haswell ?
>> >>> HSW_PS_MAX_THREADS_SHIFT : IVB_PS_MAX_THREADS_SHIFT;
>> >>> + bool msdispmode_persample = false;
>> >>> + bool multisampled_fbo = ctx->DrawBuffer->Visual.samples > 1;
>> >>>
>> >>> /* BRW_NEW_PS_BINDING_TABLE */
>> >>> BEGIN_BATCH(2);
>> >>> @@ -169,6 +195,34 @@ 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 (ctx->Extensions.ARB_sample_shading &&
>> >>> + (brw->fragment_program->Base.OutputsWritten &
>> >>> + BITFIELD64_BIT(FRAG_RESULT_SAMPLE_MASK)))
>> >>> + 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->fragment_program->Base.InputsRead &
>> >>> VARYING_BIT_SAMPLE_POS)
>> >>> + 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
>> >>> @@ -185,7 +239,17 @@ upload_ps_state(struct brw_context *brw)
>> >>> dw4 |= GEN7_PS_ATTRIBUTE_ENABLE;
>> >>>
>> >>> dw4 |= GEN7_PS_8_DISPATCH_ENABLE;
>> >>> - if (brw->wm.prog_data->prog_offset_16)
>> >>> + msdispmode_persample =
>> >>> + ctx->Multisample.Enabled &&
>> >>> + (ctx->Multisample.SampleShading ||
>> >>> + brw->fragment_program->Base.InputsRead & VARYING_BIT_SAMPLE_ID
>> >>> ||
>> >>> + brw->fragment_program->Base.InputsRead &
>> >>> VARYING_BIT_SAMPLE_POS);
>> >>
>> >> There are two copies of this expression in the Gen7 patch. You should
>> >> probably pull this up earlier, and remove the second copy (like you did
>> >> for the SNB code).
>> >>
>> > These expressions appear in different functions upload_ps_state() and
>> > upload_wm_state(). That's the reason I couldn't do what I did for SNB.
>>
>> So... this code appears in three places? Sounds like it could be a
>> helper function. _mesa_is_sample_shading_enabled(ctx, prog).
>
>
> Intel chips only support two modes: per-sample and per-fragment shading.
> But ARB_sample_shading is written in such a way that it permits other
> possibilities (for instance, some hardware might support 4x MSAA, where only
> 2 unique color values and sets of texture coordinates are dispatched per
> fragment, and that would be activated by setting MinSampleShadingARB(0.5)).
> In order to make this function useful for implementations that support those
> other possibilities, I'd recommend that the function should return the
> minimum number of fragment shader invocations that should occur for each
> fragment.
>
> That is, if the shader uses gl_SampleID or gl_SamplePosition it should
> return ctx->DrawBuffer->Visual.samples. Otherwise it should return this
> value (from the ARB_sample_shading spec):
>
> max(ceil(MIN_SAMPLE_SHADING_VALUE_ARB*SAMPLES),1)
>
> So maybe we should call it something like
> _mesa_get_min_invocations_per_fragment().
>
> Since i965 only supports per-sample and per-fragment dispatch, i965 code can
> then do:
>
> (_mesa_get_min_invocations_per_fragment(...) > 1)
>
> to determine whether to do per-fragment or per-sample shading.
>
Yes. This makes the function more useful. I'll do that.
>>
>>
>> >> Otherwise, the same comments for patch 6 apply. This looks good.
>> >>
>> >> Patch 8 is Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>> >>
>> >>> + /* In case of non 1x (i.e 4x, 8x) multisampling with
>> >>> MDISPMODE_PERSAMPLE,
>> >>> + * only one of SIMD8 and SIMD16 should be enabled.
>> >>> + */
>> >>> + if (brw->wm.prog_data->prog_offset_16 &&
>> >>> + !(multisampled_fbo && msdispmode_persample))
>> >>> dw4 |= GEN7_PS_16_DISPATCH_ENABLE;
>> >>>
>> >>> dw5 |= (brw->wm.prog_data->first_curbe_grf <<
>> >>>
>> > _______________________________________________
>> > mesa-dev mailing list
>> > mesa-dev at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
More information about the mesa-dev
mailing list