On Mon, May 07, 2018 at 03:06:29PM +0300, Pohjolainen, Topi wrote:
> On Thu, May 03, 2018 at 12:03:53PM -0700, Nanley Chery wrote:
> > We have enough information to determine the optimal flags internally.
> > ---
> > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 29
> > +++++++++++++--------------
> > 1 file changed, 14 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index 566ead0d5c8..e065c2f62e0 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -1661,7 +1661,6 @@ intel_miptree_copy_teximage(struct brw_context *brw,
> > static struct intel_miptree_aux_buffer *
> > intel_alloc_aux_buffer(struct brw_context *brw,
> > const struct isl_surf *aux_surf,
> > - uint32_t alloc_flags,
> > bool wants_memset,
> > uint8_t memset_value,
> > struct intel_mipmap_tree *mt)
> > @@ -1685,6 +1684,17 @@ intel_alloc_aux_buffer(struct brw_context *brw,
> > buf->pitch = aux_surf->row_pitch;
> > buf->qpitch = isl_surf_get_array_pitch_sa_rows(aux_surf);
> >
> > + /* If the buffer needs to be initialised (requiring the buffer to be
> > + * immediately mapped to cpu space for writing), do not use the gpu
> > access
> > + * flag which can cause an unnecessary delay if the backing pages
> > happened
> > + * to be just used by the GPU.
> > + */
> > + const bool alloc_zeroed = wants_memset && memset_value == 0;
> > + const bool needs_memset =
> > + !alloc_zeroed && (wants_memset || has_indirect_clear);
> > + const uint32_t alloc_flags =
> > + alloc_zeroed ? BO_ALLOC_ZEROED : (needs_memset ? 0 : BO_ALLOC_BUSY);
> > +
>
> What you have is correct but double ternaries always make my head spin. How
> would you feel:
>
> uint32_t alloc_flags = 0;
> if (alloc_zeroed)
> alloc_flags = BO_ALLOC_ZEROED;
> else if (!wants_memset && !has_indirect_clear)
> alloc_flags = BO_ALLOC_BUSY;
>
I was hoping this nested ternary would survive, but I don't mind
replacing it. I'd prefer to be more explicit about the case in which we
want to assign alloc_flags to 0 with something like:
uint32_t alloc_flags;
if (alloc_zeroed) {
alloc_flags = BO_ALLOC_ZEROED;
} else if (needs_memset) {
alloc_flags = 0;
} else {
alloc_flags = BO_ALLOC_BUSY;
}
OR:
uint32_t alloc_flags;
if (needs_memset) {
alloc_flags = (memset_value == 0) ? BO_ALLOC_ZEROED : 0;
} else {
alloc_flags = BO_ALLOC_BUSY;
}
Thoughts?
I just noticed that the variable naming could use some work. Maybe:
* wants_memset -> wants_aux_surf_memset
* memset_value -> aux_surf_memset_value
* needs_memset -> aux_bo_needs_memset
Would you like me to do something like this in a follow-on patch?
-Nanley
> > /* ISL has stricter set of alignment rules then the drm allocator.
> > * Therefore one can pass the ISL dimensions in terms of bytes instead
> > of
> > * trying to recalculate based on different format block sizes.
> > @@ -1697,7 +1707,6 @@ intel_alloc_aux_buffer(struct brw_context *brw,
> > }
> >
> > /* Initialize the bo to the desired value */
> > - const bool needs_memset = wants_memset || has_indirect_clear;
> > if (needs_memset) {
> > assert(!(alloc_flags & BO_ALLOC_BUSY));
> >
> > @@ -1752,12 +1761,6 @@ intel_miptree_alloc_mcs(struct brw_context *brw,
> > isl_surf_get_mcs_surf(&brw->isl_dev, &mt->surf, &temp_mcs_surf);
> > assert(ok);
> >
> > - /* Buffer needs to be initialised requiring the buffer to be immediately
> > - * mapped to cpu space for writing. Therefore do not use the gpu access
> > - * flag which can cause an unnecessary delay if the backing pages
> > happened
> > - * to be just used by the GPU.
> > - */
> > - const uint32_t alloc_flags = 0;
> > /* From the Ivy Bridge PRM, Vol 2 Part 1 p326:
> > *
> > * When MCS buffer is enabled and bound to MSRT, it is required
> > that it
> > @@ -1768,8 +1771,7 @@ intel_miptree_alloc_mcs(struct brw_context *brw,
> > *
> > * Note: the clear value for MCS buffers is all 1's, so we memset to
> > 0xff.
> > */
> > - mt->aux_buf = intel_alloc_aux_buffer(brw, &temp_mcs_surf,
> > - alloc_flags, true, 0xFF, mt);
> > + mt->aux_buf = intel_alloc_aux_buffer(brw, &temp_mcs_surf, true, 0xFF,
> > mt);
> > if (!mt->aux_buf) {
> > free(aux_state);
> > return false;
> > @@ -1813,8 +1815,7 @@ intel_miptree_alloc_ccs(struct brw_context *brw,
> > * For CCS_D, do the same thing. On gen9+, this avoids having any
> > undefined
> > * bits in the aux buffer.
> > */
> > - mt->aux_buf = intel_alloc_aux_buffer(brw, &temp_ccs_surf,
> > - BO_ALLOC_ZEROED, false, 0, mt);
> > + mt->aux_buf = intel_alloc_aux_buffer(brw, &temp_ccs_surf, true, 0, mt);
> > if (!mt->aux_buf) {
> > free(aux_state);
> > return false;
> > @@ -1879,9 +1880,7 @@ intel_miptree_alloc_hiz(struct brw_context *brw,
> > isl_surf_get_hiz_surf(&brw->isl_dev, &mt->surf, &temp_hiz_surf);
> > assert(ok);
> >
> > - const uint32_t alloc_flags = 0;
> > - mt->aux_buf = intel_alloc_aux_buffer(brw, &temp_hiz_surf,
> > - alloc_flags, false, 0, mt);
> > + mt->aux_buf = intel_alloc_aux_buffer(brw, &temp_hiz_surf, false, 0, mt);
> >
> > if (!mt->aux_buf) {
> > free(aux_state);
> > --
> > 2.16.2
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
