Nouveau - Apr 2015 - [PATCH 1/6] platform: specify the IOMMU physical translation bit

On Thu, Apr 16, 2015 at 8:06 PM, Vince Hsu <vinceh at nvidia.com>
wrote:
> The IOMMU physical translation bit might vary with different SoCs. So add
> a variable to specify this bit for GK20A.
>
> Signed-off-by: Vince Hsu <vinceh at nvidia.com>
> ---
>  drm/nouveau/nouveau_platform.c | 19 +++++++++++++++++++
>  drm/nouveau/nouveau_platform.h |  1 +
>  2 files changed, 20 insertions(+)
>
> diff --git a/drm/nouveau/nouveau_platform.c
b/drm/nouveau/nouveau_platform.c
> index 775277f1edb0..0d002f73e356 100644
> --- a/drm/nouveau/nouveau_platform.c
> +++ b/drm/nouveau/nouveau_platform.c
> @@ -25,6 +25,7 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/reset.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/iommu.h>
> @@ -92,6 +93,22 @@ static int nouveau_platform_power_down(struct
nouveau_platform_gpu *gpu)
>         return 0;
>  }
>
> +static unsigned long nouveau_platform_get_iommu_bit(struct device *dev)
> +{
> +       const struct of_device_id *match;
> +
> +       match = of_match_device(dev->driver->of_match_table, dev);
> +       if (!match) {
> +               dev_warn(dev, "cannot find OF match for
device\n");
> +               return 0;
> +       }
> +
> +       if (!strcmp(match->compatible, "nvidia,gk20a"))
> +               return 34;
> +       else
> +               return 0;
> +}
Instead of this function, you should probably use the data field of
struct of_device_id. Define a local struct called, say,
nouveau_platform_params containing (for now) a single iommu_addr_bit
field and instanciate one for each entry of nouveau_platform_match.
Then you can cast match->data and retrieve the field directly instead
of using strcmp.

I'd say this is then simple enough to do directly in
nouveau_platform_probe_iommu() instead of having a function dedicated
to it. It is also safer because when we add a new chip, we have to
update nouveau_platform_match but might very well forget about your
function, and will end up with bit 0 being set on all our GPU
addresses and endless hours of fun figuring out how this happened. :)

While I am at it: how about defining this as a u64 mask to set/unset
on GPU addresses instead of just a bit? This is more flexible and
again safer in case someone "omits" to specify the correct bit for a
chip.

On 04/17/2015 02:26 PM, Alexandre Courbot wrote:
> On Thu, Apr 16, 2015 at 8:06 PM, Vince Hsu <vinceh at nvidia.com>
wrote:
>> The IOMMU physical translation bit might vary with different SoCs. So
add
>> a variable to specify this bit for GK20A.
>>
>> Signed-off-by: Vince Hsu <vinceh at nvidia.com>
>> ---
>>   drm/nouveau/nouveau_platform.c | 19 +++++++++++++++++++
>>   drm/nouveau/nouveau_platform.h |  1 +
>>   2 files changed, 20 insertions(+)
>>
>> diff --git a/drm/nouveau/nouveau_platform.c
b/drm/nouveau/nouveau_platform.c
>> index 775277f1edb0..0d002f73e356 100644
>> --- a/drm/nouveau/nouveau_platform.c
>> +++ b/drm/nouveau/nouveau_platform.c
>> @@ -25,6 +25,7 @@
>>   #include <linux/module.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/of.h>
>> +#include <linux/of_device.h>
>>   #include <linux/reset.h>
>>   #include <linux/regulator/consumer.h>
>>   #include <linux/iommu.h>
>> @@ -92,6 +93,22 @@ static int nouveau_platform_power_down(struct
nouveau_platform_gpu *gpu)
>>          return 0;
>>   }
>>
>> +static unsigned long nouveau_platform_get_iommu_bit(struct device
*dev)
>> +{
>> +       const struct of_device_id *match;
>> +
>> +       match = of_match_device(dev->driver->of_match_table,
dev);
>> +       if (!match) {
>> +               dev_warn(dev, "cannot find OF match for
device\n");
>> +               return 0;
>> +       }
>> +
>> +       if (!strcmp(match->compatible, "nvidia,gk20a"))
>> +               return 34;
>> +       else
>> +               return 0;
>> +}
> Instead of this function, you should probably use the data field of
> struct of_device_id. Define a local struct called, say,
> nouveau_platform_params containing (for now) a single iommu_addr_bit
> field and instanciate one for each entry of nouveau_platform_match.
> Then you can cast match->data and retrieve the field directly instead
> of using strcmp.
Actually I did that way when I was based on your tree since you already 
defined the probe data. I admit I jerry-built this patch while I rebased 
to Ben's tree. Will fix in the next version.
>
> I'd say this is then simple enough to do directly in
> nouveau_platform_probe_iommu() instead of having a function dedicated
> to it. It is also safer because when we add a new chip, we have to
> update nouveau_platform_match but might very well forget about your
> function, and will end up with bit 0 being set on all our GPU
> addresses and endless hours of fun figuring out how this happened. :)
>
> While I am at it: how about defining this as a u64 mask to set/unset
> on GPU addresses instead of just a bit? This is more flexible and
> again safer in case someone "omits" to specify the correct bit
for a
> chip.
>

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may
contain
confidential information.  Any unauthorized review, use, disclosure or
distribution
is prohibited.  If you are not the intended recipient, please contact the sender
by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

On 04/16/2015 11:26 PM, Alexandre Courbot wrote:
> Instead of this function, you should probably use the data field of
> struct of_device_id. Define a local struct called, say,
> nouveau_platform_params containing (for now) a single iommu_addr_bit
> field and instanciate one for each entry of nouveau_platform_match.
> Then you can cast match->data and retrieve the field directly instead
> of using strcmp.
>
> I'd say this is then simple enough to do directly in
> nouveau_platform_probe_iommu() instead of having a function dedicated
> to it. It is also safer because when we add a new chip, we have to
> update nouveau_platform_match but might very well forget about your
> function, and will end up with bit 0 being set on all our GPU
> addresses and endless hours of fun figuring out how this happened. :)
>
> While I am at it: how about defining this as a u64 mask to set/unset
> on GPU addresses instead of just a bit? This is more flexible and
> again safer in case someone "omits" to specify the correct bit
for a
> chip.
Wouldn't u64 be as dangerous? We'd be just messing up with address in 
another way.

Another way of expressing this information would be defining number of 
physical(*) address bits. IOMMU bit is the bit after physical address bits.

Terje

* Physical here means after GPU MMU translation, i.e. either physical or 
IOMMU address. Somebody needs to come up with a term to cover that.

On Sat, Apr 18, 2015 at 12:10 AM, Terje Bergstrom <tbergstrom at
nvidia.com> wrote:
>
> On 04/16/2015 11:26 PM, Alexandre Courbot wrote:
>>
>> Instead of this function, you should probably use the data field of
>> struct of_device_id. Define a local struct called, say,
>> nouveau_platform_params containing (for now) a single iommu_addr_bit
>> field and instanciate one for each entry of nouveau_platform_match.
>> Then you can cast match->data and retrieve the field directly
instead
>> of using strcmp.
>>
>> I'd say this is then simple enough to do directly in
>> nouveau_platform_probe_iommu() instead of having a function dedicated
>> to it. It is also safer because when we add a new chip, we have to
>> update nouveau_platform_match but might very well forget about your
>> function, and will end up with bit 0 being set on all our GPU
>> addresses and endless hours of fun figuring out how this happened. :)
>>
>> While I am at it: how about defining this as a u64 mask to set/unset
>> on GPU addresses instead of just a bit? This is more flexible and
>> again safer in case someone "omits" to specify the correct
bit for a
>> chip.
>
> Wouldn't u64 be as dangerous? We'd be just messing up with address
in
> another way.
>
> Another way of expressing this information would be defining number of
> physical(*) address bits. IOMMU bit is the bit after physical address bits.
I don't have a strong opinion on this - the advantage of a bit mask
over a bit index is that it at least gives us the option of not having
a MMU translation bit at all (mask == 0) whereas a bit index of 0 will
set/unset the first bit.

Not that we need this for now, but keeping that possibility open
doesn't sound bad for the future (nothing guarantees the GPU will
always be behind the IOMMU).