Nouveau - Apr 2014 - [PATCH 02/12] drm/nouveau/timer: skip calibration on GK20A

On Mon, Mar 24, 2014 at 05:42:24PM +0900, Alexandre Courbot
wrote:
> GK20A's timer is directly attached to the system timer and cannot be
> calibrated. Skip the calibration phase on that chip since the
> corresponding registers do not exist.
> 
> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
> ---
>  drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
> index c0bdd10358d7..822fe0d8a871 100644
> --- a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
> +++ b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
> @@ -185,6 +185,10 @@ nv04_timer_init(struct nouveau_object *object)
>  	if (ret)
>  		return ret;
>  
> +	/* gk20a does not have the calibration registers */
> +	if (device->chipset == 0xea)
> +		goto skip_clk_init;
I'm concerned that this won't scale in the future. Perhaps a better
solution would be to add a "flags" or "features" field to
struct
nouveau_device along with feature bits such as HAS_CALIBRATION or
similar.

That way we don't have to touch this code for every new future Tegra
chip. Unless perhaps if there's a reason to expect things to change in
newer generations.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20140324/16ab7985/attachment.sig>

On Tue, Mar 25, 2014 at 7:54 AM, Thierry Reding
<thierry.reding at gmail.com> wrote:
> On Mon, Mar 24, 2014 at 05:42:24PM +0900, Alexandre Courbot wrote:
>> GK20A's timer is directly attached to the system timer and cannot
be
>> calibrated. Skip the calibration phase on that chip since the
>> corresponding registers do not exist.
>>
>> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
>> ---
>>  drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c | 19
+++++++++++++------
>>  1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
>> index c0bdd10358d7..822fe0d8a871 100644
>> --- a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
>> +++ b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
>> @@ -185,6 +185,10 @@ nv04_timer_init(struct nouveau_object *object)
>>       if (ret)
>>               return ret;
>>
>> +     /* gk20a does not have the calibration registers */
>> +     if (device->chipset == 0xea)
>> +             goto skip_clk_init;
>
> I'm concerned that this won't scale in the future. Perhaps a better
> solution would be to add a "flags" or "features" field
to struct
> nouveau_device along with feature bits such as HAS_CALIBRATION or
> similar.
>
> That way we don't have to touch this code for every new future Tegra
> chip. Unless perhaps if there's a reason to expect things to change in
> newer generations.
I've already handled this in a slightly different way in the tree I'd
previously pointed Alex at (I think!), as I needed to do the same for
GM107.

Should just be able to use that implementation (so, just change the
probe patch) here too.

Thanks,
Ben.
>
> Thierry
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

On Wed, Mar 26, 2014 at 1:19 PM, Ben Skeggs <skeggsb at gmail.com>
wrote:
> On Tue, Mar 25, 2014 at 7:54 AM, Thierry Reding
> <thierry.reding at gmail.com> wrote:
>> On Mon, Mar 24, 2014 at 05:42:24PM +0900, Alexandre Courbot wrote:
>>> GK20A's timer is directly attached to the system timer and
cannot be
>>> calibrated. Skip the calibration phase on that chip since the
>>> corresponding registers do not exist.
>>>
>>> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
>>> ---
>>>  drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c | 19
+++++++++++++------
>>>  1 file changed, 13 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
>>> index c0bdd10358d7..822fe0d8a871 100644
>>> --- a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
>>> +++ b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
>>> @@ -185,6 +185,10 @@ nv04_timer_init(struct nouveau_object *object)
>>>       if (ret)
>>>               return ret;
>>>
>>> +     /* gk20a does not have the calibration registers */
>>> +     if (device->chipset == 0xea)
>>> +             goto skip_clk_init;
>>
>> I'm concerned that this won't scale in the future. Perhaps a
better
>> solution would be to add a "flags" or "features"
field to struct
>> nouveau_device along with feature bits such as HAS_CALIBRATION or
>> similar.
>>
>> That way we don't have to touch this code for every new future
Tegra
>> chip. Unless perhaps if there's a reason to expect things to change
in
>> newer generations.
> I've already handled this in a slightly different way in the tree
I'd
> previously pointed Alex at (I think!), as I needed to do the same for
> GM107.
>
> Should just be able to use that implementation (so, just change the
> probe patch) here too.
I will skip this patch and use your implementation then. Btw,
shouldn't the source file for the GK20A implementation be named nvea.c
instead of gk20a.c?