Re: [Mesa-dev] [PATCH] winsys/radeon: fix a wrong NUM_TILE_PIPES value from the kernel
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
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
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
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
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
> +/* 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
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, > &ws->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, >&ws->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
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, > &ws->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, >&ws->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
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, &ws->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, &ws->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