On Mon, May 7, 2018 at 11:04 AM, Nanley Chery <nanleych...@gmail.com> 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;
> }
>
> Thoughts?
>
How about something like this:
bool memset_surface = wants_memset;
bool memset_clear_value = has_indirect_clear;
alloc_flags = 0;
if (wants_memset && memset_value == 0) {
/* The allocator can do the memset to 0 for us */
alloc_flags |= BO_ALLOC_ZEROED;
wants_memset = false;
memset_clear_value = false;
}
if (!memset_surface && !memset_clear_value)
alloc_flags |= BO_ALLOC_BUSY;
I'm not sure that I'm helping either.... Yeah, let's just go with what you
have here.
> 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
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
