| Message ID | 20160826142513.7108-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/26/2016 04:35 PM, Lucas Stach wrote: > Am Freitag, den 26.08.2016, 16:25 +0200 schrieb Marek Vasut: >> The content of gpu->memory_base should point to start of RAM, not zero. >> >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Lucas Stach <l.stach@pengutronix.de> >> Cc: Christian Gmeiner <christian.gmeiner@gmail.com> >> Cc: Russell King <rmk+kernel@arm.linux.org.uk> > > NACK. This will cause memory corruption when used with a MC10 GPU. The > checks are there for a reason. ;) > > I'll have to think about a proper solution for MX6SX. Oh ok, thanks. >> --- >> drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c >> index ff6aa5d..a168532 100644 >> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c >> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c >> @@ -588,6 +588,8 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu) >> gpu->memory_base = PHYS_OFFSET; >> else >> gpu->memory_base = dma_mask - SZ_2G + 1; >> + } else { >> + gpu->memory_base = PHYS_OFFSET; >> } >> >> ret = etnaviv_hw_reset(gpu); > >
On Fri, Aug 26, 2016 at 04:25:13PM +0200, Marek Vasut wrote: > The content of gpu->memory_base should point to start of RAM, not zero. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Christian Gmeiner <christian.gmeiner@gmail.com> > Cc: Russell King <rmk+kernel@arm.linux.org.uk> > --- > drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c > index ff6aa5d..a168532 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c > @@ -588,6 +588,8 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu) > gpu->memory_base = PHYS_OFFSET; > else > gpu->memory_base = dma_mask - SZ_2G + 1; > + } else { > + gpu->memory_base = PHYS_OFFSET; > } The code is correct as-is. Etnaviv GPUs are not without their own weirdness. For MC1.0 3D GPUs (eg, GC600 as found on Dove), there is an issue where _most_ GPU accesses go through a memory offsetting via the memory base, but accesses by the tiler do not: they bypass the memory base offsetting. We have two options to work around that: 1. Maintain two GPU address spaces, and mark relocations according to their use. This is error prone: we would have to have the kernel verify the relocation type is valid for the load state address to avoid obvious security issues, and the maintanence of these different address spaces becomes more complex. 2. Force the address offset to zero so all GPU accesses map through the same method irrespective of which GPU block is performing the access. We've chosen (2) because it is much simpler, and less error prone, plus it fits with the platforms that we're aware of at the moment. I did push for the relocations to have an additional member which can be used to flag the type of the relocation if necessary in the future without changing the interface structure layouts, so we do have that option if we really need to do it - but we'd all prefer to avoid it. Also, iirc, the GPU tiler with MC1.0 is unable to access addresses >= 2GiB no matter what - if you do have a 3D GPU with MC1.0, and you have physical memory above 2GiB... there's no currently known solution... you lose.
On 08/26/2016 06:06 PM, Russell King - ARM Linux wrote: > On Fri, Aug 26, 2016 at 04:25:13PM +0200, Marek Vasut wrote: >> The content of gpu->memory_base should point to start of RAM, not zero. >> >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Lucas Stach <l.stach@pengutronix.de> >> Cc: Christian Gmeiner <christian.gmeiner@gmail.com> >> Cc: Russell King <rmk+kernel@arm.linux.org.uk> >> --- >> drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c >> index ff6aa5d..a168532 100644 >> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c >> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c >> @@ -588,6 +588,8 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu) >> gpu->memory_base = PHYS_OFFSET; >> else >> gpu->memory_base = dma_mask - SZ_2G + 1; >> + } else { >> + gpu->memory_base = PHYS_OFFSET; >> } > > The code is correct as-is. Etnaviv GPUs are not without their own > weirdness. I suspected as much. I think my GPU isn't MC2.0 if I read the minor_features0 bits right . > For MC1.0 3D GPUs (eg, GC600 as found on Dove), there is an issue > where _most_ GPU accesses go through a memory offsetting via the > memory base, but accesses by the tiler do not: they bypass the > memory base offsetting. > > We have two options to work around that: > > 1. Maintain two GPU address spaces, and mark relocations according > to their use. This is error prone: we would have to have the > kernel verify the relocation type is valid for the load state > address to avoid obvious security issues, and the maintanence > of these different address spaces becomes more complex. > > 2. Force the address offset to zero so all GPU accesses map through > the same method irrespective of which GPU block is performing the > access. > > We've chosen (2) because it is much simpler, and less error prone, > plus it fits with the platforms that we're aware of at the moment. > I did push for the relocations to have an additional member which > can be used to flag the type of the relocation if necessary in the > future without changing the interface structure layouts, so we do > have that option if we really need to do it - but we'd all prefer > to avoid it. I see, thanks for the detailed explanation. > Also, iirc, the GPU tiler with MC1.0 is unable to access addresses >> = 2GiB no matter what - if you do have a 3D GPU with MC1.0, and > you have physical memory above 2GiB... there's no currently known > solution... you lose. Yes, i.MX6SX has GC400T and the DRAM starts at 0x8000_0000 . Did I misunderstand something ?
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index ff6aa5d..a168532 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -588,6 +588,8 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu) gpu->memory_base = PHYS_OFFSET; else gpu->memory_base = dma_mask - SZ_2G + 1; + } else { + gpu->memory_base = PHYS_OFFSET; } ret = etnaviv_hw_reset(gpu);
The content of gpu->memory_base should point to start of RAM, not zero. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Lucas Stach <l.stach@pengutronix.de> Cc: Christian Gmeiner <christian.gmeiner@gmail.com> Cc: Russell King <rmk+kernel@arm.linux.org.uk> --- drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 2 ++ 1 file changed, 2 insertions(+)