On Fri, Jun 26, 2015 at 01:23:41PM -0700, Anuj Phogat wrote:
> On Mon, Jun 22, 2015 at 5:23 PM, Anuj Phogat <anuj.pho...@gmail.com> wrote:
> > On Mon, Jun 22, 2015 at 2:53 PM, Ben Widawsky <b...@bwidawsk.net> wrote:
> >> On Wed, Jun 10, 2015 at 03:30:47PM -0700, Anuj Phogat wrote:
> >>> Buffers with Yf/Ys tiling end up using meta upload / download
> >>> paths or the blitter for cases where they used tiled_memcpy paths
> >>> in case of Y tiling. This has exposed some bugs in meta path. To
> >>> avoid any piglit regressions on SKL this patch keeps the Yf/Ys
> >>> tiling disabled at the moment.
> >>>
> >>> V3: Make brw_miptree_choose_tr_mode() actually choose TRMODE. (Ben)
> >>> Few cosmetic changes.
> >>> V4: Get rid of brw_miptree_choose_tr_mode().
> >>> Take care of all tile resource modes {Yf, Ys, none} for all
> >>> generations at one place.
> >>>
> >>> Signed-off-by: Anuj Phogat <anuj.pho...@gmail.com>
> >>> Cc: Ben Widawsky <b...@bwidawsk.net>
> >>> ---
> >>> src/mesa/drivers/dri/i965/brw_tex_layout.c | 97
> >>> ++++++++++++++++++++++++------
> >>> 1 file changed, 79 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> >>> b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> >>> index b9ac4cf..c0ef5cc 100644
> >>> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> >>> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> >>> @@ -807,27 +807,88 @@ brw_miptree_layout(struct brw_context *brw,
> >>> enum intel_miptree_tiling_mode requested,
> >>> struct intel_mipmap_tree *mt)
> >>> {
> >>> - mt->tr_mode = INTEL_MIPTREE_TRMODE_NONE;
> >>> + const unsigned bpp = mt->cpp * 8;
> >>> + const bool is_tr_mode_yf_ys_allowed =
> >>> + brw->gen >= 9 &&
> >>> + !for_bo &&
> >>> + !mt->compressed &&
> >>> + /* Enable YF/YS tiling only for color surfaces because depth and
> >>> + * stencil surfaces are not supported in blitter using fast copy
> >>> + * blit and meta PBO upload, download paths. No other paths
> >>> + * currently support Yf/Ys tiled surfaces.
> >>> + * FIXME: Remove this restriction once we have a tiled_memcpy()
> >>> + * path to do depth/stencil data upload/download to Yf/Ys tiled
> >>> + * surfaces.
> >>> + */
> >>
> >> I think it's more readable to move this comment above the variable
> >> declaration.
> >> Up to you though. Also I think "FINISHME" is the more appropriate
> >> classification
> >> for this type of thing.
> >>
> > Sure.
> >>> + _mesa_is_format_color_format(mt->format) &&
> >>> + (requested == INTEL_MIPTREE_TILING_Y ||
> >>> + requested == INTEL_MIPTREE_TILING_ANY) &&
> >>
> >> This is where my tiling flags would have helped a bit since you should be
> >> able
> >> to do flags & Y_TILED :P
> >>
> > Yes, I will do a follow up patch to make use of that.
> >>> + (bpp && is_power_of_two(bpp)) &&
> >>> + /* FIXME: To avoid piglit regressions keep the Yf/Ys tiling
> >>> + * disabled at the moment.
> >>> + */
> >>> + false;
> >>
> >> Also, "FINISHME"
> >>
> >>>
> >>> - intel_miptree_set_alignment(brw, mt);
> >>> - intel_miptree_set_total_width_height(brw, mt);
> >>> + /* Lower index (Yf) is the higher priority mode */
> >>> + const uint32_t tr_mode[3] = {INTEL_MIPTREE_TRMODE_YF,
> >>> + INTEL_MIPTREE_TRMODE_YS,
> >>> + INTEL_MIPTREE_TRMODE_NONE};
> >>> + int i = is_tr_mode_yf_ys_allowed ? 0 : ARRAY_SIZE(tr_mode) - 1;
> >>>
> >>> - if (!mt->total_width || !mt->total_height) {
> >>> - intel_miptree_release(&mt);
> >>> - return;
> >>> - }
> >>> + while (i < ARRAY_SIZE(tr_mode)) {
> >>> + if (brw->gen < 9)
> >>> + assert(tr_mode[i] == INTEL_MIPTREE_TRMODE_NONE);
> >>> + else
> >>> + assert(tr_mode[i] == INTEL_MIPTREE_TRMODE_YF ||
> >>> + tr_mode[i] == INTEL_MIPTREE_TRMODE_YS ||
> >>> + tr_mode[i] == INTEL_MIPTREE_TRMODE_NONE);
> >>>
> >>> - /* On Gen9+ the alignment values are expressed in multiples of the
> >>> block
> >>> - * size
> >>> - */
> >>> - if (brw->gen >= 9) {
> >>> - unsigned int i, j;
> >>> - _mesa_get_format_block_size(mt->format, &i, &j);
> >>> - mt->align_w /= i;
> >>> - mt->align_h /= j;
> >>> - }
> >>> + mt->tr_mode = tr_mode[i];
> >>> + intel_miptree_set_alignment(brw, mt);
> >>> + intel_miptree_set_total_width_height(brw, mt);
> >>>
> >>> - if (!for_bo)
> >>> - mt->tiling = brw_miptree_choose_tiling(brw, requested, mt);
> >>> + if (!mt->total_width || !mt->total_height) {
> >>> + intel_miptree_release(&mt);
> >>> + return;
> >>> + }
> >>> +
> >>> + /* On Gen9+ the alignment values are expressed in multiples of the
> >>> + * block size.
> >>> + */
> >>> + if (brw->gen >= 9) {
> >>> + unsigned int i, j;
> >>> + _mesa_get_format_block_size(mt->format, &i, &j);
> >>> + mt->align_w /= i;
> >>> + mt->align_h /= j;
> >>> + }
> >>
> >> Can we just combine this alignment calculation into
> >> intel_miptree_set_alignment()?
> >>
> > No. intel_miptree_set_total_width_height() called after
> > intel_miptree_set_alignment() needs align_w and align_h values in
> > pixels. We do the division later to directly use mt->align_w and
> > mt->align_h while setting the surface state which needs the values
> > in number of blocks. I have a cleanup patch moving this code to
> > surface state setup.
> >
> >>> +
> >>> + if (!for_bo)
> >>> + mt->tiling = brw_miptree_choose_tiling(brw, requested, mt);
> >>
> >> Perhaps (fwiw, I prefer break instead of returning within a loop, but
> >> that's
> >> just me)?
> > I'll change it use break.
> >> /* If there is already a BO, we cannot effect tiling modes */
> >> if (for_bo)
> >> break;
> >>
> >>
> >> mt->tiling = brw_miptree_choose_tiling(brw, requested, mt);;
> >> if (is_tr_mode_yf_ys_allowed) {
> >> ...
> >> }
> >>
> >> This sort of reflects how I felt earlier about pushing the YF/YS decision
> >> into
> >> choose tiling. The code is heading in that direction though, so I am
> >> content.
> >>
> >>
> >>> +
> >>> + if (is_tr_mode_yf_ys_allowed) {
> >>> + unsigned int level = 0;
> >>> + assert(brw->gen >= 9);
> >>
> >> I am assert happy, but this is a bit redundant even more my standards :-)
> >>
> > right :)
> >>> +
> >>> + if (mt->tiling == I915_TILING_Y ||
> >>> + mt->tiling == (I915_TILING_Y | I915_TILING_X) ||
> >>> + mt->tr_mode == INTEL_MIPTREE_TRMODE_NONE) {
> >>> + /* FIXME: Don't allow YS tiling at the moment. Using 64KB
> >>> tiling
> >>> + * for small textures might result in to memory wastage.
> >>> Revisit
> >>> + * this condition when we have more information about the
> >>> specific
> >>> + * cases where using YS over YF will be useful.
> >>> + */
> >>> + if (mt->tr_mode != INTEL_MIPTREE_TRMODE_YS)
> >>> + return;
> >>> + }
> >>> + /* Failed to use selected tr_mode. Free up the memory allocated
> >>> + * for miptree levels in intel_miptree_total_width_height().
> >>> + */
> >>> + for (level = mt->first_level; level <= mt->last_level; level++)
> >>> {
> >>> + free(mt->level[level].slice);
> >>> + mt->level[level].slice = NULL;
> >>> + }
> >>> + }
> >>> + i++;
> >>> + }
> >>> }
> >>>
> >>
> >> I'm still reviewing, but I don't think you need to change any of what I
> >> said
> >> unless you want to.
> I fixed these small issues in my review branch. Do you've any other comments?
What's in your review branch lgtm. That (5a17421dcad) is:
Reviewed-by: Ben Widawsky <b...@bwidawsk.net>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
