On Tue, May 08, 2018 at 08:43:20AM +0300, Pohjolainen, Topi wrote:
> 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 <topi.pohjolai...@intel.com>
>
Great!
> >
> > 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.
>
Okay. I'll hold off for now.
-Nanley
> >
> > -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
