On Wed, Jul 12, 2017 at 11:05 AM, Chad Versace <chadvers...@chromium.org>
wrote:
> On Thu 29 Jun 2017, Jason Ekstrand wrote:
> > From: Ben Widawsky <b...@bwidawsk.net>
> >
> > This code will disable actually creating these buffers for the scanout,
> > but it puts the allocation in place.
> >
> > Primarily this patch is split out for review, it can be squashed in
> > later if preferred.
> >
> > v2:
> > assert(mt->offset == 0) in ccs creation (as requested by Topi)
> > Remove bogus is_scanout check in miptree_release
> >
> > v3:
> > Remove is_scanout assert in intel_miptree_create. It doesn't work with
> > latest codebase - not sure it ever should have worked.
> >
> > v4:
> > assert(mt->last_level == 0) and assert(mt->first_level == 0) in ccs setup
> > (Topi)
> >
> > v5 (Jason Ekstrand):
> > - Base the decision to allocate a CCS on the image modifier
> >
> > Signed-off-by: Ben Widawsky <b...@bwidawsk.net>
> > Acked-by: Daniel Stone <dani...@collabora.com>
> > Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com>
> > ---
> > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 123
> +++++++++++++++++++++++---
> > 1 file changed, 113 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index 575f04f..7a22cbf 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -59,6 +59,11 @@ intel_miptree_alloc_mcs(struct brw_context *brw,
> > struct intel_mipmap_tree *mt,
> > GLuint num_samples);
> >
> > +static void
> > +intel_miptree_init_mcs(struct brw_context *brw,
> > + struct intel_mipmap_tree *mt,
> > + int init_value);
> > +
> > /**
> > * Determine which MSAA layout should be used by the MSAA surface being
> > * created, based on the chip generation and the surface type.
> > @@ -1026,6 +1031,67 @@ miptree_create_for_planar_image(struct
> brw_context *brw,
> > return planar_mt;
> > }
> >
> > +static bool
> > +create_ccs_buf_for_image(struct brw_context *brw,
> > + __DRIimage *image,
> > + struct intel_mipmap_tree *mt,
> > + enum isl_aux_state initial_state)
> > +{
> > + struct isl_surf temp_main_surf, temp_ccs_surf;
> > +
> > + /* There isn't anything specifically wrong with there being an
> offset, in
> > + * which case, the CCS miptree's offset should be mt->offset +
> > + * image->aux_offset. However, the code today only will have an
> offset when
> > + * this miptree is pointing to a slice from another miptree, and in
> that case
> > + * we'd need to offset within the AUX CCS buffer properly. It's
> questionable
> > + * whether our code handles that case properly, and since it can
> never happen
> > + * for scanout, just use the assertion to prevent it.
> > + */
> > + assert(mt->offset == 0);
> > +
> > + /* CCS is only supported for very simple miptrees */
> > + assert(image->aux_offset && image->aux_pitch);
>
> Why require that aux_offset > 0? Why reject images where the aux surface
> precedes the primary surface? This rejection seems arbitrary.
All throughout these patches image->aux_offset == 0 is used for "no aux".
As you pointed out earlier, that's a bit on the bogus side but also always
true. For scanout, the hardware requires the aux buffer to be placed after
the main surface so it's not *that* bogus.
> Please
> add a little explanatory comment.
>
> Also, please use `!= 0` above, to be consistent with the assertions
> above and below this one.
>
> > + assert(image->tile_x == 0 && image->tile_y == 0);
> > + assert(mt->num_samples <= 1);
> > + assert(mt->first_level == 0);
> > + assert(mt->last_level == 0);
> > + assert(mt->logical_depth0 == 1);
> > +
> > + /* We shouldn't already have a CCS */
> > + assert(!mt->mcs_buf);
> > +
> > + intel_miptree_get_isl_surf(brw, mt, &temp_main_surf);
> > + if (!isl_surf_get_ccs_surf(&brw->isl_dev, &temp_main_surf,
> &temp_ccs_surf))
> > + return false;
> > +
> > + assert(temp_ccs_surf.size <= image->bo->size - image->aux_offset);
>
> This assertion appears invalid, vulnerable to underflow when the
> incoming aux_offset is wrong and too large. It needs to be preceded
> by assert(image->aux_offset < image->bo->size).
>
Sure. I'm happy to add that.
> > + assert(temp_ccs_surf.row_pitch <= image->aux_pitch);
> > +
> > + mt->mcs_buf = calloc(sizeof(*mt->mcs_buf), 1);
> > + if (mt->mcs_buf == NULL)
> > + return false;
> > +
> > + mt->aux_state = create_aux_state_map(mt, initial_state);
> > + if (!mt->aux_state) {
> > + free(mt->mcs_buf);
> > + mt->mcs_buf = NULL;
> > + return false;
> > + }
> > +
> > + mt->mcs_buf->bo = image->bo;
> > + brw_bo_reference(image->bo);
> > +
> > + mt->mcs_buf->offset = image->aux_offset;
> > + mt->mcs_buf->size = image->bo->size - image->aux_offset;
> > + mt->mcs_buf->pitch = image->aux_pitch;
> > + mt->mcs_buf->qpitch = 0;
> > +
> > + intel_miptree_init_mcs(brw, mt, 0);
>
> Ack! This may be an imported image, and therefore the aux surface content
> is
> significant. But this patch clobbers it! What am I misunderstanding?
>
Nothing. You're 100% correct. I believe the correct answer is that the
original creator of an image with aux needs to do the init. I'll get that
fixed.
> > + mt->msaa_layout = INTEL_MSAA_LAYOUT_CMS;
>
> I flinched when I saw mt->msaa_layout = INTEL_MSAA_LAYOUT_CMS. And the
> comments
> below make me sad. Anyway, fixing that is a task for another day.
>
It makes me very sad too. I've been trying, as I refactor, to move us away
from that but it hasn't quite died yet.
> bool
> intel_miptree_is_lossless_compressed(const struct brw_context *brw,
> const struct intel_mipmap_tree
> *mt)
> {
> ...
>
> /* Single sample compression is represented re-using msaa
> compression
> * layout type: "Compressed Multisampled Surfaces".
> */
> if (mt->msaa_layout != INTEL_MSAA_LAYOUT_CMS)
> return false;
>
> /* And finally distinguish between msaa and single sample case. */
> return mt->num_samples <= 1;
> }
>
> > +
> > + return true;
> > +}
> > +
> > struct intel_mipmap_tree *
> > intel_miptree_create_for_dri_image(struct brw_context *brw,
> > __DRIimage *image, GLenum target,
> > @@ -1072,15 +1138,26 @@ intel_miptree_create_for_dri_image(struct
> brw_context *brw,
> > if (!brw->ctx.TextureFormatSupported[format])
> > return NULL;
> >
> > + const struct isl_drm_modifier_info *mod_info =
> > + isl_drm_modifier_get_info(image->modifier);
> > +
> > + uint32_t mt_layout_flags = 0;
> > +
> > + /* If this image comes in from a window system, then it may get
> promoted to
> > + * scanout at any time so we need to set the flag accordingly.
> > + */
> > + if (is_winsys_image)
> > + mt_layout_flags |= MIPTREE_LAYOUT_FOR_SCANOUT;
> > +
> > /* If this image comes in from a window system, we have different
> > * requirements than if it comes in via an EGL import operation.
> Window
> > * system images can use any form of auxiliary compression we wish
> because
> > * they get "flushed" before being handed off to the window system
> and we
> > - * have the opportunity to do resolves. Window system buffers also
> may be
> > - * used for scanout so we need to flag that appropriately.
> > + * have the opportunity to do resolves.
> > */
> > - const uint32_t mt_layout_flags =
> > - is_winsys_image ? MIPTREE_LAYOUT_FOR_SCANOUT :
> MIPTREE_LAYOUT_DISABLE_AUX;
> > + if (!is_winsys_image &&
> > + (!mod_info || mod_info->aux_usage == ISL_AUX_USAGE_NONE))
> > + mt_layout_flags |= MIPTREE_LAYOUT_DISABLE_AUX;
> >
> > /* Disable creation of the texture's aux buffers because the driver
> exposes
> > * no EGL API to manage them. That is, there is no API for resolving
> the aux
> > @@ -1117,15 +1194,41 @@ intel_miptree_create_for_dri_image(struct
> brw_context *brw,
> > }
> > }
> >
> > - /* Since CCS_E can compress more than just clear color, we create
> the CCS
> > - * for it up-front. For CCS_D which only compresses clears, we
> create the
> > - * CCS on-demand when a clear occurs that wants one.
> > - */
> > - if (mt->aux_usage == ISL_AUX_USAGE_CCS_E) {
> > - if (!intel_miptree_alloc_ccs(brw, mt)) {
> > + if (mod_info && mod_info->aux_usage != ISL_AUX_USAGE_NONE) {
> > + assert(mod_info->aux_usage == ISL_AUX_USAGE_CCS_E);
> > +
> > + mt->aux_usage = mod_info->aux_usage;
>
> Convince me that it's safe to clobber mt->aux_usage here. I will be easily
> convinced.
>
No aux buffer has been created yet. I'm happy to assert that.
> > + /* If we are a window system buffer, then we can support
> fast-clears
> > + * even if the modifier doesn't support them by doing a partial
> resolve
> > + * as part of the flush operation.
> > + */
> > + mt->supports_fast_clear =
> > + is_winsys_image || mod_info->supports_clear_color;
> > +
> > + /* We don't know the actual state of the surface when we get it
> but we
> > + * can make a pretty good guess based on the modifier. What we
> do know
> > + * for sure is that it isn't in the AUX_INVALID state, so we just
> assume
> > + * a worst case of compression.
> > + */
> > + enum isl_aux_state initial_state =
> > + mod_info->supports_clear_color ?
> ISL_AUX_STATE_COMPRESSED_CLEAR :
> > + ISL_AUX_STATE_COMPRESSED_NO_
> CLEAR;
> > +
> > + if (!create_ccs_buf_for_image(brw, image, mt, initial_state)) {
> > intel_miptree_release(&mt);
> > return NULL;
> > }
> > + } else if (mt->aux_usage != ISL_AUX_USAGE_NONE) {
> > + /* Since CCS_E can compress more than just clear color, we create
> the
> > + * CCS for it up-front. For CCS_D which only compresses clears,
> we
> > + * create the CCS on-demand when a clear occurs that wants one.
> > + */
> > + if (mt->aux_usage == ISL_AUX_USAGE_CCS_E) {
>
> The else-if-if chain here is ugly.
Yes it is. But I did it because it made more logical sense to me. "if we
have a modifier, follow it for aux else follow aux_usage." However, the
only aux_usage that will ever show up here is CCS_E.
> The inner if's condition should replace the
> outer else-if's condition.
>
Here's a better idea. How about I write an intel_miptree_alloc_aux helper
function and call that for aux_usage != NONE? It can handle CCS, MCS, and
HiZ so that's all together in one place.
--Jason
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
