On Mon, May 07, 2018 at 11:04:20AM -0700, Nanley Chery wrote:
> 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;
> }
What you had originally starts to look the cleanest, and we have similar code
elsewhere.
Reviewed-by: Topi Pohjolainen <[email protected]>
>
> 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?
Up to you really, the scope of the function is pretty limited and therefore it
isn't hard to remember the semantics of the variables.
>
> -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
> > > [email protected]
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
