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

[Alex Deucher]

On Tue, Feb 9, 2016 at 12:47 PM, Marek Olšák  wrote:
> On Tue, Feb 9, 2016 at 6:17 PM, Alexandre Demers
>  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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Alexandre Demers]

> +/* 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?

My late two cents here.
Alexandre Demers
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Alexandre Demers]

On Tue, 9 Feb 2016 at 15:17 Alex Deucher  wrote:

> On Tue, Feb 9, 2016 at 12:47 PM, Marek Olšák  wrote:
> > On Tue, Feb 9, 2016 at 6:17 PM, Alexandre Demers
> >  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?
Alexandre Demers
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Alexandre Demers]

On Tue, 9 Feb 2016 at 12:47 Marek Olšák  wrote:

> On Tue, Feb 9, 2016 at 6:17 PM, Alexandre Demers
>  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.
>
> Marek
>
Well, meanwhile, I went on and I had a look at the kernel settings. Here is
the answer and the "problem":

This was returned by radeon_info_ioctl(), case RADEON_INFO_NUM_TILE_PIPES,
else if (rdev->family >= CHIP_TAHITI) *value =
rdev->config.si.max_tile_pipes;
(
http://lxr.free-electrons.com/source/drivers/gpu/drm/radeon/radeon_kms.c#L353
)

Searching where max_tile_pipes was set, it seems the value comes from
si_gpu_init(), case CHIP_TAHITI, rdev->config.si.max_tile_pipes = 12
(
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/radeon/si.c
)

This is probably wrong, all other GPU having a power of 2 value (I had a
look at ni.c, si.c, evergreen.c). Either that or we have a special case for
Tahiti.

Also, if this is a special case, while comparing how things works between
si.c and cik.c, I saw that (si | cik)_tiling_mode_table_init() were not
exactly mapping gpus the same way: si.c uses the family, while cik.c uses
the max_tile_pipes value and defaults any value over 8 to be treated as 16.

If this can be of any help / reflection...
Alexandre Demers
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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
 wrote:
> On Tue, 9 Feb 2016 at 15:17 Alex Deucher  wrote:
>>
>> On Tue, Feb 9, 2016 at 12:47 PM, Marek Olšák  wrote:
>> > On Tue, Feb 9, 2016 at 6:17 PM, Alexandre Demers
>> >  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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Marek Olšák]

On Tue, Feb 9, 2016 at 6:17 PM, Alexandre Demers
 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.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Michel Dänzer]

On 08.02.2016 04:25, Marek Olšák wrote:
> From: Marek Olšák 
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94019
> ---
>  src/gallium/winsys/radeon/drm/radeon_drm_winsys.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c 
> b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
> index 35dc7e6..49c310c 100644
> --- a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
> +++ b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
> @@ -405,6 +405,12 @@ static boolean do_winsys_init(struct radeon_drm_winsys 
> *ws)
>  radeon_get_drm_value(ws->fd, RADEON_INFO_NUM_TILE_PIPES, NULL,
>   >info.num_tile_pipes);
>  
> +/* 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;
> +
>  if (radeon_get_drm_value(ws->fd, RADEON_INFO_BACKEND_MAP, NULL,
>>info.r600_gb_backend_map))
>  ws->info.r600_gb_backend_map_valid = TRUE;
> 

Reviewed-by: Michel Dänzer 


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Nick Sarnie]

Hi,

This fixes the bug for me.

Tested-by: Nick Sarnie 

Thanks

On Sun, Feb 7, 2016 at 2:25 PM, Marek Olšák  wrote:

> From: Marek Olšák 
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94019
> ---
>  src/gallium/winsys/radeon/drm/radeon_drm_winsys.c | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
> b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
> index 35dc7e6..49c310c 100644
> --- a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
> +++ b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
> @@ -405,6 +405,12 @@ static boolean do_winsys_init(struct
> radeon_drm_winsys *ws)
>  radeon_get_drm_value(ws->fd, RADEON_INFO_NUM_TILE_PIPES, NULL,
>   >info.num_tile_pipes);
>
> +/* 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;
> +
>  if (radeon_get_drm_value(ws->fd, RADEON_INFO_BACKEND_MAP,
> NULL,
>>info.r600_gb_backend_map))
>  ws->info.r600_gb_backend_map_valid = TRUE;
> --
> 2.1.4
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> 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

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

[Marek Olšák]

From: Marek Olšák 

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94019
---
 src/gallium/winsys/radeon/drm/radeon_drm_winsys.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c 
b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
index 35dc7e6..49c310c 100644
--- a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
+++ b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
@@ -405,6 +405,12 @@ static boolean do_winsys_init(struct radeon_drm_winsys *ws)
 radeon_get_drm_value(ws->fd, RADEON_INFO_NUM_TILE_PIPES, NULL,
  >info.num_tile_pipes);
 
+/* 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;
+
 if (radeon_get_drm_value(ws->fd, RADEON_INFO_BACKEND_MAP, NULL,
   >info.r600_gb_backend_map))
 ws->info.r600_gb_backend_map_valid = TRUE;
-- 
2.1.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev