On Thu 14 Jul 2016, Jason Ekstrand wrote:
> On Thu, Jul 14, 2016 at 1:54 PM, Chad Versace <chad.vers...@intel.com>
> wrote:
>
> > On Wed 13 Jul 2016, Jason Ekstrand wrote:
> > > Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com>
> > > ---
> > > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 120
> > ++++++++++++++++++++++++++
> > > src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 5 ++
> > > 2 files changed, 125 insertions(+)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > index 7d3cec2..114959e 100644
> > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > @@ -3171,6 +3171,126 @@ intel_miptree_get_isl_surf(struct brw_context
> > *brw,
> > > surf->usage = 0; /* TODO */
> > > }
> > >
> > > +/* WARNING: THE SURFACE CREATED BY THIS FUNCTION IS NOT COMPLETE AND
> > CANNOT BE
> > > + * USED FOR ANY REAL CALCULATIONS. THE ONLY VALID USE OF SUCH A
> > SURFACE IS TO
> > > + * PASS IT INTO isl_surf_fill_state.
> > > + */
> > > +void
> > > +intel_miptree_get_aux_isl_surf(struct brw_context *brw,
> > > + const struct intel_mipmap_tree *mt,
> > > + struct isl_surf *surf,
> > > + enum isl_aux_usage *usage)
> > > +{
> >
> > This function doesn't update the value of isl_surf::image_alignnment_el,
> > as set by intel_miptree_get_isl_surf(). That bothers me. Convince me
> > that it's ok.
> >
>
> The output of this function only ever gets fed into isl_surf_fill_state as
> an aux_surf so that value is never used for anything ever.
That's what I suspected. Sounds ok.
> >
> > > + /* Much is the same as the regular surface */
> > > + intel_miptree_get_isl_surf(brw, mt->mcs_mt, surf);
> > > +
> > > + /* Figure out the layout */
> > > + if (_mesa_get_format_base_format(mt->format) == GL_DEPTH_COMPONENT) {
> > > + *usage = ISL_AUX_USAGE_HIZ;
> > > + } else if (mt->num_samples > 1) {
> > > + if (mt->msaa_layout == INTEL_MSAA_LAYOUT_CMS)
> > > + *usage = ISL_AUX_USAGE_MCS;
> > > + else
> > > + *usage = ISL_AUX_USAGE_NONE;
> >
> > How is ISL_AUX_USAGE_NONE possible? If the primary surface is using
> > INTEL_MSAA_LAYOUT_IMS or UMS, then it better have no auxiliary surface.
> > Assert!
> >
> > > + } else if (intel_miptree_is_lossless_compressed(brw, mt)) {
> > > + assert(brw->gen >= 9);
> > > + *usage = ISL_AUX_USAGE_CCS_E;
> > > + } else if (mt->fast_clear_state != INTEL_FAST_CLEAR_STATE_NO_MCS) {
> > > + *usage = ISL_AUX_USAGE_CCS_D;
> > > + } else {
> > > + /* Can we even get here? */
> > > + *usage = ISL_AUX_USAGE_NONE;
> >
> > I hope not! If we did, then we have big trouble. Assert!
> >
> > > + }
> > > +
> > > + /* Figure out the format and tiling of the auxiliary surface */
> > > + switch (*usage) {
> > > + case ISL_AUX_USAGE_NONE:
> > > + /* Can we even get here? */
> >
> > Again... assert! Even if the code *can* get here, it *shouldn't* get
> > here.
> >
>
> I just made a patch of assertions and we'll run it through Jenkins. I'll
> squash in if it's ok.
Ok.
> > > + break;
> > > +
> > > + case ISL_AUX_USAGE_HIZ:
> > > + surf->format = ISL_FORMAT_HIZ;
> > > + surf->tiling = ISL_TILING_HIZ;
> > > + surf->usage = ISL_SURF_USAGE_HIZ_BIT;
> > > + break;
> > > +
> > > + case ISL_AUX_USAGE_MCS:
> > > + /*
> > > + * From the SKL PRM:
> > > + * "When Auxiliary Surface Mode is set to AUX_CCS_D or
> > AUX_CCS_E,
> > > + * HALIGN 16 must be used."
> > > + */
> > > + if (brw->gen >= 9)
> > > + assert(mt->halign == 16);
> > > +
> > > + surf->usage = ISL_SURF_USAGE_MCS_BIT;
> > > +
> > > + switch (mt->num_samples) {
> > > + case 2: surf->format = ISL_FORMAT_MCS_2X; break;
> > > + case 4: surf->format = ISL_FORMAT_MCS_4X; break;
> > > + case 8: surf->format = ISL_FORMAT_MCS_8X; break;
> > > + case 16: surf->format = ISL_FORMAT_MCS_16X; break;
> > > + default:
> > > + unreachable("Invalid number of samples");
> > > + }
> > > + break;
> > > +
> > > + case ISL_AUX_USAGE_CCS_D:
> > > + case ISL_AUX_USAGE_CCS_E:
> > > + /*
> > > + * From the BDW PRM, Volume 2d, page 260 (RENDER_SURFACE_STATE):
> > > + *
> > > + * "When MCS is enabled for non-MSRT, HALIGN_16 must be used"
> > > + *
> > > + * From the hardware spec for GEN9:
> > > + *
> > > + * "When Auxiliary Surface Mode is set to AUX_CCS_D or
> > AUX_CCS_E,
> > > + * HALIGN 16 must be used."
> > > + */
> > > + if (brw->gen >= 9 || mt->num_samples == 1)
> > > + assert(mt->halign == 16);
> >
> > Here, samples should *always* equal 1. This hunk should drop
> > 'mt->num_samples == 1', and optionally replace it with an assertion.
> >
>
>
> Actually, it's worse, samples == 0 so the assertion was never getting run.
> I've updated it to "if (brw->gen >= 8) assert(mt->halign == 16)"
Good! Bug caught during review.
> > > +
> > > + surf->tiling = ISL_TILING_CCS;
> > > + surf->usage = ISL_SURF_USAGE_CCS_BIT;
> > > +
> > > + if (brw->gen >= 9) {
> > > + assert(mt->tiling == I915_TILING_Y);
> > > + switch (_mesa_get_format_bytes(mt->format)) {
> > > + case 4: surf->format = ISL_FORMAT_GEN9_CCS_32BPP; break;
> > > + case 8: surf->format = ISL_FORMAT_GEN9_CCS_64BPP; break;
> > > + case 16: surf->format = ISL_FORMAT_GEN9_CCS_128BPP; break;
> > > + default:
> > > + unreachable("Invalid format size for color compression");
> > > + }
> > > + } else if (mt->tiling == I915_TILING_Y) {
> > > + switch (_mesa_get_format_bytes(mt->format)) {
> > > + case 4: surf->format = ISL_FORMAT_GEN7_CCS_32BPP_Y; break;
> > > + case 8: surf->format = ISL_FORMAT_GEN7_CCS_64BPP_Y; break;
> > > + case 16: surf->format = ISL_FORMAT_GEN7_CCS_128BPP_X; break;
> >
> > Bug. case 16 should be ISL_FORMAT_GEN7_CCS_128BPP_Y.
> >
>
> Fixed
>
>
> > > + default:
> > > + unreachable("Invalid format size for color compression");
> > > + }
> > > + } else {
> > > + assert(mt->tiling == I915_TILING_X);
> > > + switch (_mesa_get_format_bytes(mt->format)) {
> > > + case 4: surf->format = ISL_FORMAT_GEN7_CCS_32BPP_X; break;
> > > + case 8: surf->format = ISL_FORMAT_GEN7_CCS_64BPP_X; break;
> > > + case 16: surf->format = ISL_FORMAT_GEN7_CCS_128BPP_X; break;
> > > + default:
> > > + unreachable("Invalid format size for color compression");
> > > + }
> > > + }
> > > + break;
> > > + }
> > > +
> > > + /* Auxiliary surfaces in ISL have compressed formats so
> > array_pitch_el_rows
> > ^^
> >
> > Perhaps s/so/and/? Because, array_pitch_el_rows is *always* in elements.
> >
>
> yup
>
>
> >
> > > + * is in elements. This doesn't match intel_mipmap_tree::qpitch
> > which is
> > > + * in elements of the primary color surface so we have to divide by
> > the
> > > + * compression block height.
> > > + */
> > > + surf->array_pitch_el_rows = mt->qpitch /
> > isl_format_get_layout(surf->format)->bh;
> >
> > The function doesn't update isl_surf::row_pitch, and I believe that's
> > correct.
> >
>
> Yes it is
>
>
> >
> > > +}
> > > +
> > > union isl_color_value
> > > intel_miptree_get_isl_clear_color(struct brw_context *brw,
> > > const struct intel_mipmap_tree *mt)
> > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > index a50f181..4388741 100644
> > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > @@ -801,6 +801,11 @@ void
> > > intel_miptree_get_isl_surf(struct brw_context *brw,
> > > const struct intel_mipmap_tree *mt,
> > > struct isl_surf *surf);
> > > +void
> > > +intel_miptree_get_aux_isl_surf(struct brw_context *brw,
> > > + const struct intel_mipmap_tree *mt,
> > > + struct isl_surf *surf,
> > > + enum isl_aux_usage *usage);
> > >
> > > union isl_color_value
> > > intel_miptree_get_isl_clear_color(struct brw_context *brw,
> >
With the assertions squashed in, and the fixes you acked,
Reviewed-by: Chad Versace <chad.vers...@intel.com>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
