[Mesa-dev] [PATCH 2/5] i965/gen9: Plugin the code for selecting YF/YS tiling on skl+
Anuj Phogat
anuj.phogat at gmail.com
Fri Jun 26 13:23:41 PDT 2015
On Mon, Jun 22, 2015 at 5:23 PM, Anuj Phogat <anuj.phogat at gmail.com> wrote:
> On Mon, Jun 22, 2015 at 2:53 PM, Ben Widawsky <ben at 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.phogat at gmail.com>
>>> Cc: Ben Widawsky <ben at 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?
More information about the mesa-dev
mailing list