[Mesa-dev] [PATCH v5 06/19] intel/isl: Add support to emit clear value address.
Pohjolainen, Topi
topi.pohjolainen at gmail.com
Fri Apr 6 04:04:18 UTC 2018
On Thu, Apr 05, 2018 at 04:11:33PM -0700, Nanley Chery wrote:
> On Wed, Apr 04, 2018 at 07:40:43AM +0300, Pohjolainen, Topi wrote:
> > On Tue, Apr 03, 2018 at 09:38:52PM -0700, Jason Ekstrand wrote:
> > > On Tue, Apr 3, 2018 at 8:23 PM, Pohjolainen, Topi <
> > > topi.pohjolainen at gmail.com> wrote:
> > >
> > > > On Tue, Apr 03, 2018 at 02:55:31PM -0700, Jason Ekstrand wrote:
> > > > > On Tue, Apr 3, 2018 at 8:05 AM, Pohjolainen, Topi <
> > > > > topi.pohjolainen at gmail.com> wrote:
> > > > >
> > > > > > On Thu, Mar 29, 2018 at 10:58:40AM -0700, Rafael Antognolli wrote:
> > > > > > > gen10 can emit the clear color by setting it on a buffer somewhere,
> > > > and
> > > > > > > then adding only the address to the surface state.
> > > > > > >
> > > > > > > This commit add support for that on isl_surf_fill_state, and if that
> > > > is
> > > > > > > requested, skip setting the clear value itself.
> > > > > > >
> > > > > > > v2: Add assert to make sure we are at least on gen10.
> > > > > > >
> > > > > > > Signed-off-by: Rafael Antognolli <rafael.antognolli at intel.com>
> > > > > > > Reviewed-by: Jordan Justen <jordan.l.justen at intel.com>
> > > > > > > ---
> > > > > > > src/intel/isl/isl.h | 9 +++++++++
> > > > > > > src/intel/isl/isl_surface_state.c | 18 ++++++++++++++----
> > > > > > > 2 files changed, 23 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
> > > > > > > index 2edf0522e32..c50b78d4701 100644
> > > > > > > --- a/src/intel/isl/isl.h
> > > > > > > +++ b/src/intel/isl/isl.h
> > > > > > > @@ -1307,6 +1307,15 @@ struct isl_surf_fill_state_info {
> > > > > > > */
> > > > > > > union isl_color_value clear_color;
> > > > > > >
> > > > > > > + /**
> > > > > > > + * Send only the clear value address
> > > > > > > + *
> > > > > > > + * If set, we only pass the clear address to the GPU and it will
> > > > > > fetch it
> > > > > > > + * from wherever it is.
> > > > > > > + */
> > > > > > > + bool use_clear_address;
> > > > > > > + uint64_t clear_address;
> > > > > > > +
> > > > > > > /**
> > > > > > > * Surface write disables for gen4-5
> > > > > > > */
> > > > > > > diff --git a/src/intel/isl/isl_surface_state.c
> > > > > > b/src/intel/isl/isl_surface_state.c
> > > > > > > index 32a5429f2bf..bff9693f02d 100644
> > > > > > > --- a/src/intel/isl/isl_surface_state.c
> > > > > > > +++ b/src/intel/isl/isl_surface_state.c
> > > > > > > @@ -637,11 +637,21 @@ isl_genX(surf_fill_state_s)(const struct
> > > > > > isl_device *dev, void *state,
> > > > > > > #endif
> > > > > > >
> > > > > > > if (info->aux_usage != ISL_AUX_USAGE_NONE) {
> > > > > > > + if (info->use_clear_address) {
> > > > > > > +#if GEN_GEN >= 10
> > > > > > > + s.ClearValueAddressEnable = true;
> > > > > >
> > > > > > This will set it for multisampled also and upset piglit tests. We need
> > > > > > something of this sort:
> > > > > >
> > > > > > s.ClearValueAddressEnable = info->aux_usage !=
> > > > > > ISL_AUX_USAGE_MCS;
> > > > > >
> > > > >
> > > > > Can we assert instead? If the caller asks for a clear address to be set
> > > > > they should get it and not have it magically disabled when they ask for
> > > > MCS.
> > > >
> > > > Right, here an assert would be just fine. I should have made the comment
> > > > in patch in 19 where it belongs. There we start setting the flag
> > > > unconditionally for "info->aux_usage != ISL_AUX_USAGE_NONE". If we don't,
> > > > then on ICL we start failing multisampling tests.
> > > >
> > >
> > > Is this a known hardware restriction or is there just something weird going
> > > on with MCS?
> >
> > Bspec says the clear address is only for CCS_D and CCS_E. And setting it for
> > MCS seems to upset both the simulator and hardware. At least with
> >
> > ext_framebuffer_multisample-accuracy 2 color
> >
>
> Could you help me find the Bspec restriction?
>
> The Bspec says that the clear address exists for
>
> [Auxiliary Surface Mode] == 'AUX_CCS_D' OR
> [Auxiliary Surface Mode] == 'AUX_CCS_E'
>
> In the definition of Auxiliary Surface Mode, it states that AUX_CCS_* is
> equal to MCS if the sample count > 1.
Right you are, I keep forgetting that CCS_D/CCS_E stands for MCS when
sample count > 1. So it should work for multisampled surfaces as well. Looks
like there is more debugging for me to do to understand why the tests start
failing on ICL if clear color address is enabled for MCS surface. Thanks for
reading it more carefully than I did!
>
> -Nanley
>
> > >
> > >
> > > > >
> > > > >
> > > > > > > + s.ClearValueAddress = info->clear_address;
> > > > > > > +#else
> > > > > > > + unreachable("Gen9 and earlier do not support indirect clear
> > > > > > colors");
> > > > > > > +#endif
> > > > > > > + }
> > > > > > > #if GEN_GEN >= 9
> > > > > > > - s.RedClearColor = info->clear_color.u32[0];
> > > > > > > - s.GreenClearColor = info->clear_color.u32[1];
> > > > > > > - s.BlueClearColor = info->clear_color.u32[2];
> > > > > > > - s.AlphaClearColor = info->clear_color.u32[3];
> > > > > > > + if (!info->use_clear_address) {
> > > > > > > + s.RedClearColor = info->clear_color.u32[0];
> > > > > > > + s.GreenClearColor = info->clear_color.u32[1];
> > > > > > > + s.BlueClearColor = info->clear_color.u32[2];
> > > > > > > + s.AlphaClearColor = info->clear_color.u32[3];
> > > > > > > + }
> > > > > > > #elif GEN_GEN >= 7
> > > > > > > /* Prior to Sky Lake, we only have one bit for the clear color
> > > > > > which
> > > > > > > * gives us 0 or 1 in whatever the surface's format happens
> > > > to be.
> > > > > > > --
> > > > > > > 2.14.3
> > > > > > >
> > > > > > > _______________________________________________
> > > > > > > mesa-dev mailing list
> > > > > > > mesa-dev at lists.freedesktop.org
> > > > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > > > > _______________________________________________
> > > > > > mesa-dev mailing list
> > > > > > mesa-dev at lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > > > >
> > > >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list