[Mesa-dev] [PATCH V2 21/22] i965/gen9: Plugin the code for selecting YF/YS tiling on skl+
Anuj Phogat
anuj.phogat at gmail.com
Fri May 22 15:03:27 PDT 2015
On Thu, May 21, 2015 at 4:33 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
> On Fri, Apr 17, 2015 at 04:51:42PM -0700, Anuj Phogat wrote:
>> Note: Yf/Ys tiling stays disabled to avoid any piglit regressions. I'm
>> working on fixing the remaining piglit failures.
>>
>> We need to do some benchmarking to come up with conditions to choose
>> Ys (64 KB) over Yf (4 KB). Any thoughts on how big a texture should
>> be so that 64 KB tiling is preferred over 4KB?
>>
>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>> ---
>> src/mesa/drivers/dri/i965/brw_tex_layout.c | 81 ++++++++++++++++++++++++++++++
>> 1 file changed, 81 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c b/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> index 28927e9..b2a2dac 100644
>> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> @@ -684,6 +684,65 @@ intel_miptree_total_width_height(struct brw_context *brw,
>> }
>> }
>>
>> +static bool
>> +intel_miptree_choose_tr_mode(struct brw_context *brw,
>
> Either rename this function, or make it actually choose the tr_mode.
>
>> + mesa_format format,
>> + uint32_t width0,
>> + uint32_t num_samples,
>> + enum intel_miptree_tiling_mode requested,
>> + struct intel_mipmap_tree *mt,
>> + uint32_t tr_mode)
>
> As I state above and below, I don't think tr_mode should be passed in here. This
> function is supposed to "choose" the mode.
>
Got it.
>> +{
>> + const unsigned bpp = mt->cpp * 8;
>> +
>> + /* bpp must be power of 2. */
>> + if (!mt->compressed &&
>> + _mesa_is_format_color_format(mt->format) &&
>
> I am not finding the reason for the color format restriction. I believe it, just
> not seeing it...
>
I couldn't find it either. Found this in my notes:
Enable YF/YS tiling only for color buffers because depth and
stencil buffers are not supported in XY_FAST_COPY_BLT
and meta_pbo_{TexSubImage, GetTexSubImage} paths.
These are the only paths enabled by this series to handle
Yf/Ys surfaces.
I think we can enable them for depth and stencil if we
implement the tiled_memcpy() paths to do the software
decoding of Yf/Ys tiled surfaces. I'll add a comment here
to explain the restriction.
>> + (requested == INTEL_MIPTREE_TILING_Y ||
>> + requested == INTEL_MIPTREE_TILING_ANY) &&
>> + (bpp && (bpp & (bpp - 1)) == 0)) {
>> +
>
> is_power_of_2
>
yes.
>
> How about something like...
>
> /* Low bits are the highest priority modes */
> modes = INTEL_MIPTREE_TRMODE_YS | INTEL_MIPTREE_TRMODE_YF;
> while ((modes >>= 1) & 1) {
> ... try to allocate
> }
>
Yes. This looks better. We can later add conditions here to set the
priority of Yf/Ys based on buffer size, alignment etc.
>> + mt->tr_mode = tr_mode;
>
> you're leaking tr_mode state here if you fail below.
>
I'll reset it to TRMODE_NONE at the bottom of function. This patch
does it outside this function in the fallback path.
>> + mt->align_w = intel_horizontal_texture_alignment_unit(brw, mt);
>> + mt->align_h = intel_vertical_texture_alignment_unit(brw, mt);
>> +
>> + intel_miptree_total_width_height(brw, mt);
>> +
>> + /* pitch == 0 || height == 0 indicates the null texture */
>
> What is the null texture?
Copy-pasted comment. I'll get rid of it.
>
>> + if (!mt || !mt->total_width || !mt->total_height) {
>> + intel_miptree_release(&mt);
>> + return false;
>> + }
>> +
>> + mt->tiling = intel_miptree_choose_tiling(brw, format, width0,
>> + num_samples,
>> + requested, mt);
>> +
>> + if (mt->tiling == I915_TILING_Y ||
>> + mt->tiling == (I915_TILING_Y | I915_TILING_X)) {
>> +
>> + /* Don't allow YS tiling at the moment. Using 64KB tiling for small
>> + * textures might result in to wastage of memory.
>> + * FIXME: 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_YF)
>> + return true;
>> + }
>> +
>> + /* Can't use requested tr_mode. Free up the memory allocated for
>> + * miptree levels in intel_miptree_total_width_height().
>> + */
>> + unsigned level;
>> + for (level = mt->first_level; level <= mt->last_level; level++) {
>> + free(mt->level[level].slice);
>> + mt->level[level].slice = NULL;
>> + }
>
> intel_miptree_release(&mt); ? Either that or remove it above. Seems like
> returning false should always do the same thing.
>
intel_miptree_release() does more than just freeing up the allocated
slices, which we don't want to do here. We still want to keep the
allocated miptree and retry with different TRMODE. Returning false
here without freeing up the slices will cause an assertion failure
(assert(mt->level[level].slice == NULL)) in intel_miptree_set_level_info()
when we keep the same miptree and retry with different TRMODE.
>> + }
>> +
>> + return false;
>> +}
>> +
>> void
>> brw_miptree_layout(struct brw_context *brw,
>> mesa_format format,
>> @@ -695,6 +754,28 @@ brw_miptree_layout(struct brw_context *brw,
>> {
>> bool gen6_hiz_or_stencil = false;
>>
>> + /* Check in advance if we can do Y tiling with Yf or Ys tiled resource
>> + * modes. Fall back to using INTEL_MIPTREE_TRMODE_NONE.
>> + * FIXME: 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 and missing support of
>> + * few formats in meta path. To avoid piglit regressions disable the
>> + * Yf/Ys tiling at the moment.
>> + */
>> + if (brw->gen >= 9 && !for_bo && false /* disable Yf/Ys */) {
>> + /* Prefer YF over YS at the moment. */
>> + if (intel_miptree_choose_tr_mode(brw, format, width0, num_samples,
>> + requested, mt,
>> + INTEL_MIPTREE_TRMODE_YF) ||
>> + intel_miptree_choose_tr_mode(brw, format, width0, num_samples,
>> + requested, mt,
>> + INTEL_MIPTREE_TRMODE_YS))
>> + return;
>> + }
>
> I'm not a huge fan of the way you wrote this. It seems like the determination of
> the TILING would be better off in choose (and making the verb choose correct in
> the process).
>
>
> if ((brw->gen >= 9 && !for_bo && false /* disable Yf/Ys */) {
> if (intel_miptree_choose_tr_mode(brw, format...))
> return;
> }
>
Looks good to me.
>> +
>> + if (!mt)
>> + return;
>
> This can't be right for gen9. You just tried to deref mt above for the tr_mode
> determination. I'm guessing you rebased this and originally you had an
> allocation function for the mt - either way, this seems wrong.
>
Following code in brw_miptree_choose_tr_mode() can cause mt == NULL:
if (!mt || !mt->total_width || !mt->total_height) {
intel_miptree_release(&mt);
return false;
>> +
>> mt->tr_mode = INTEL_MIPTREE_TRMODE_NONE;
>>
>> if (brw->gen == 6 && mt->array_layout == ALL_SLICES_AT_EACH_LOD) {
>
> Looking forward to the next rev :-)
Thanks for the review Ben.
>
> --
> Ben Widawsky, Intel Open Source Technology Center
More information about the mesa-dev
mailing list