On Fri, Jul 29, 2016 at 07:04:40AM -0700, Jason Ekstrand wrote:
> On Jul 29, 2016 2:18 AM, "Pohjolainen, Topi"
> <[1]topi.pohjolai...@intel.com> wrote:
> >
> > On Fri, Jul 29, 2016 at 09:58:36AM +0300, Pohjolainen, Topi wrote:
> > > On Tue, Jul 26, 2016 at 03:11:13PM -0700, Jason Ekstrand wrote:
> > > > ---
> > > > src/mesa/drivers/dri/i965/brw_blorp.c | 43
> ++++++++++++++++++++---------------
> > > > src/mesa/drivers/dri/i965/brw_blorp.h | 4 ++++
> > > > 2 files changed, 29 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c
> b/src/mesa/drivers/dri/i965/brw_blorp.c
> > > > index cf1615f..97eddf9 100644
> > > > --- a/src/mesa/drivers/dri/i965/brw_blorp.c
> > > > +++ b/src/mesa/drivers/dri/i965/brw_blorp.c
> > > > @@ -136,8 +136,17 @@ brw_blorp_surface_info_init(struct
> brw_context *brw,
> > > > if (mt->mcs_mt) {
> > > > intel_miptree_get_aux_isl_surf(brw, mt, &info->aux_surf,
> > > > &info->aux_usage);
> > > > + info->aux_bo = mt->mcs_mt->bo;
> > > > + info->aux_offset = mt->mcs_mt->offset;
> > > > +
> > > > + /* We only really need a clear color if we also have an
> auxiliary
> > > > + * surface. Without one, it does nothing.
> > > > + */
> > > > + info->clear_color = intel_miptree_get_isl_clear_color(brw,
> mt);
> > > > } else {
> > > > info->aux_usage = ISL_AUX_USAGE_NONE;
> > > > + info->aux_bo = NULL;
> > > > + info->aux_offset = 0;
> > > > }
> > > >
> > > > info->view = (struct isl_view) {
> > > > @@ -341,20 +350,17 @@ brw_blorp_emit_surface_state(struct
> brw_context *brw,
> > > > surf.dim = ISL_SURF_DIM_2D;
> > > > }
> > > >
> > > > - union isl_color_value clear_color = { .u32 = { 0, 0, 0, 0 }
> };
> > > > -
> > > > - const struct isl_surf *aux_surf = NULL;
> > > > - uint64_t aux_offset = 0;
> > > > - if (surface->mt->mcs_mt) {
> > > > - aux_surf = &surface->aux_surf;
> > > > - assert(surface->mt->mcs_mt->offset == 0);
> > >
> > > We are dropping this assert. Follow-up question further down.
> > >
> > > > - aux_offset = surface->mt->mcs_mt->bo->offset64;
> > > > + /* Blorp doesn't support HiZ in any of the blit or slow-clear
> paths */
> > > > + enum isl_aux_usage aux_usage = surface->aux_usage;
> > > > + if (aux_usage == ISL_AUX_USAGE_HIZ)
> > > > + aux_usage = ISL_AUX_USAGE_NONE;
> > > >
> > > > - /* We only really need a clear color if we also have an
> auxiliary
> > > > - * surface. Without one, it does nothing.
> > > > - */
> > > > - clear_color = intel_miptree_get_isl_clear_color(brw,
> surface->mt);
> > > > - }
> > > > + /* If we don't have an aux surface, the clear color is
> meaningless. Don't
> > > > + * bother to set it up in the surface state.
> > > > + */
> > > > + union isl_color_value clear_color = surface->clear_color;
> > > > + if (aux_usage == ISL_AUX_USAGE_NONE)
> > > > + memset(&clear_color, 0, sizeof(clear_color));
> > > >
> > > > uint32_t surf_offset;
> > > > uint32_t *dw = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,
> > > > @@ -362,11 +368,12 @@ brw_blorp_emit_surface_state(struct
> brw_context *brw,
> > > > &surf_offset);
> > > >
> > > > const uint32_t mocs = is_render_target ? ss_info.rb_mocs :
> ss_info.tex_mocs;
> > > > + uint64_t aux_bo_offset = surface->aux_bo ?
> surface->aux_bo->offset64 : 0;
> > > >
> > > > isl_surf_fill_state(&brw->isl_dev, dw, .surf = &surf, .view =
> &surface->view,
> > > > .address = surface->bo->offset64 +
> surface->offset,
> > > > - .aux_surf = aux_surf, .aux_usage =
> surface->aux_usage,
> > > > - .aux_address = aux_offset,
> > > > + .aux_surf = &surface->aux_surf,
> .aux_usage = aux_usage,
> > > > + .aux_address = aux_bo_offset +
> surface->aux_offset,
> > >
> > > Am I also reading this correctly but here we actually start to take
> > > 'mt->mcs_mt->offset' into account?
> > >
> > > I would like to keep the assert one way or the other. I don't think
> we are
> > > ready for offsetting aux buffers yet. Or what do you think?
> >
> > Reading next patch I get the feeling you are thinking of using this
> for hiz?
> > If that is the case could we assert, for example:
> >
> > assert(surface->aux_offset == 0 || aux_usage ==
> ISL_AUX_USAGE_HIZ);
>
> Within the context of this series, it does get used for HiZ. When we
> start using blorp in Vulkan, all surfaces can be at offsets and all aux
> surfaces *will* be at offsets.
Ok, perhaps a word or two in the commit message. Anyway:
Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com>
>
> > >
> > > > .mocs = mocs, .clear_color = clear_color,
> > > > .x_offset_sa = surface->tile_x_sa,
> > > > .y_offset_sa = surface->tile_y_sa);
> > > > @@ -378,15 +385,15 @@ brw_blorp_emit_surface_state(struct
> brw_context *brw,
> > > > dw[ss_info.reloc_dw] -
> surface->bo->offset64,
> > > > read_domains, write_domain);
> > > >
> > > > - if (aux_surf) {
> > > > + if (aux_usage != ISL_AUX_USAGE_NONE) {
> > > > /* On gen7 and prior, the bottom 12 bits of the MCS base
> address are
> > > > * used to store other information. This should be ok,
> however, because
> > > > * surface buffer addresses are always 4K page alinged.
> > > > */
> > > > - assert((aux_offset & 0xfff) == 0);
> > > > + assert((surface->aux_offset & 0xfff) == 0);
> > > > drm_intel_bo_emit_reloc(brw->[2]batch.bo,
> > > > surf_offset + ss_info.aux_reloc_dw
> * 4,
> > > > - surface->mt->mcs_mt->bo,
> > > > + surface->aux_bo,
> > > > dw[ss_info.aux_reloc_dw] & 0xfff,
> > > > read_domains, write_domain);
> > > > }
> > > > diff --git a/src/mesa/drivers/dri/i965/brw_blorp.h
> b/src/mesa/drivers/dri/i965/brw_blorp.h
> > > > index 98a9436..d747880 100644
> > > > --- a/src/mesa/drivers/dri/i965/brw_blorp.h
> > > > +++ b/src/mesa/drivers/dri/i965/brw_blorp.h
> > > > @@ -76,8 +76,12 @@ struct brw_blorp_surface_info
> > > > uint32_t offset;
> > > >
> > > > struct isl_surf aux_surf;
> > > > + drm_intel_bo *aux_bo;
> > > > + uint32_t aux_offset;
> > > > enum isl_aux_usage aux_usage;
> > > >
> > > > + union isl_color_value clear_color;
> > > > +
> > > > struct isl_view view;
> > > >
> > > > /* Z offset into a 3-D texture or slice of a 2-D array
> texture. */
> > > > --
> > > > 2.5.0.400.gff86faf
> > > >
> > > > _______________________________________________
> > > > mesa-dev mailing list
> > > > [3]mesa-dev@lists.freedesktop.org
> > > > [4]https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > _______________________________________________
> > > mesa-dev mailing list
> > > [5]mesa-dev@lists.freedesktop.org
> > > [6]https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
> References
>
> 1. mailto:topi.pohjolai...@intel.com
> 2. http://batch.bo/
> 3. mailto:mesa-dev@lists.freedesktop.org
> 4. https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 5. mailto:mesa-dev@lists.freedesktop.org
> 6. 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
