[Mesa-dev] [PATCH] winsys/radeon: fix a wrong NUM_TILE_PIPES value from the kernel

[Marek Olšák]

On Tue, Feb 9, 2016 at 11:38 PM, Alexandre Demers
<alexandre.f.demers at gmail.com> wrote:
> On Tue, 9 Feb 2016 at 15:17 Alex Deucher <alexdeucher at gmail.com> wrote:
>>
>> On Tue, Feb 9, 2016 at 12:47 PM, Marek Olšák <maraeo at gmail.com> wrote:
>> > On Tue, Feb 9, 2016 at 6:17 PM, Alexandre Demers
>> > <alexandre.f.demers at gmail.com> wrote:
>> >>> +            /* The kernel returns 12 for some cards for an unknown
>> >>> reason.
>> >>> +             * I thought this was supposed to be a power of two.
>> >>> +             */
>> >>> +            if (ws->gen == DRV_SI && ws->info.num_tile_pipes == 12)
>> >>> +                ws->info.num_tile_pipes = 8;
>> >>> +
>> >>
>> >> I may be late in the conversation, but shouldn't we have a look at why
>> >> the
>> >> value reported by the kernel is wrong for "some" cards? Which ones and
>> >> why
>> >> should be identified. It seems to be limited to Southern Islands as far
>> >> as
>> >> we know for now, which limits the scope for now.
>> >>
>> >> Also, about the patch itself, even if only some cards were reported to
>> >> be
>> >> problematic, why would we limit it to "ws->gen == DRV_SI"? Any cards
>> >> reporting a wrong value should be treated the same way by mapping its
>> >> value
>> >> from 12 to 8, no?
>> >
>> > No. Only one card is affected (Tahiti or Pitcairn, I don't remember
>> > which one). No other card reports 12.
>> >
>> > There is no point in looking into why the value is wrong and I haven't
>> > been able to find where the value had come from. It's part of the
>> > kernel ABI now anyway. Userspace won't use it anymore.
>>
>> I did it when I added SI support.  I think I get the value from tcore
>> or some hw features header.  I don't remember the details.  Anyway, I
>> think it may have been a hw value (related to the number of memory
>> channels) whereas from a sw perspective, we'd use 8 for tiling
>> computations.  E.g., when we set up rdev->config.si.tile_config which
>> is what we used to use, we set it to 8.
>>
>> Alex
>
>
> Ok, I had a look at what you were pointing. There was even a comment in
> si_gpu_init() about tile_config for the case of the problematic value:
>
> rdev->config.si.tile_config = 0;
> switch (rdev->config.si.num_tile_pipes) {
> [...]
> case 8:
> default:
> /* XXX what about 12? */
> rdev->config.si.tile_config |= (3 << 0);
> break;
> [...]
>
> Should we just clarify the comment in Mesa's committed patch according to
> Alex's explanation?

I don't think it's necessary. The value should be equal to the pipe
config setting in the tile mode array. That's the only thing that
matters here. Everything else is irrelevant AFAIK.

Marek