Nouveau - Apr 2014 - [PATCH 11/12] drm/nouveau: support GK20A in nouveau_accel_init()

On Wed, Mar 26, 2014 at 1:27 PM, Ben Skeggs <skeggsb at gmail.com>
wrote:
> On Tue, Mar 25, 2014 at 9:10 AM, Thierry Reding
> <thierry.reding at gmail.com> wrote:
>> On Mon, Mar 24, 2014 at 05:42:33PM +0900, Alexandre Courbot wrote:
>>> GK20A does not embed a dedicated COPY engine and thus cannot
allocate
>>> the copy channel that nouveau_accel_init() attempts to create. It
also
>>> lacks any display hardware, so the creation of a software channel
does
>>> not apply neither.
>>
>> Perhaps this should be two separate patches?
>>
>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
>> [...]
>>> +     if (device->chipset == 0xea) {
>>> +             /* gk20a does not have CE0/CE1 */
>>
>> This would be another good candidate for a feature flag.
> There are ways to query this in a chipset-independent way.  However,
> despite reporting it as an error if no copy engines are available, the
> code should continue on without the channel happily.  Perhaps we can
> just punt the relevent error messages to a debug loglevel for now?
Sure, that would be more future-proof as well.
>
>>
>>> +             arg0 = NVE0_CHANNEL_IND_ENGINE_GR;
>>> +             arg1 = 1;
>>> +     } else
>>>       if (device->card_type >= NV_E0) {
>>
>> The formatting here is somewhat weird. From a brief look I couldn't
find
>> any indication that nouveau deviates from the standard coding style, so
>> this should be:
>>
>>         } else if (...) {
> I use the former in a few places, despite it not entirely being
> "correct".. It looks nicer though :)  I don't mind either way
though.
Yeah, I just followed the style of the file here. Whether it needs to
change or not is not my call. :P
>>> +     /* Need to figure out how to handle sw for gk20a */
>>> +     if (device->chipset == 0xea)
>>> +             goto skip_sw_init;
>>
>> The commit message makes it sound like SW isn't needed since gk20a
>> "lacks any display hardware". In that case the comment here
doesn't make
>> much sense.
Correct. As far as I have looked (that is, not very far), SW methods
are used for display-related functions, but there might be other
use-cases too. Maybe someone who knows better can confirm?

On Wed, Apr 2, 2014 at 10:14 AM, Alexandre Courbot <gnurou at gmail.com>
wrote:
>>>> +     /* Need to figure out how to handle sw for gk20a */
>>>> +     if (device->chipset == 0xea)
>>>> +             goto skip_sw_init;
>>>
>>> The commit message makes it sound like SW isn't needed since
gk20a
>>> "lacks any display hardware". In that case the comment
here doesn't make
>>> much sense.
>
> Correct. As far as I have looked (that is, not very far), SW methods
> are used for display-related functions, but there might be other
> use-cases too. Maybe someone who knows better can confirm?
http://cgit.freedesktop.org/~darktama/nouveau/tree/nvkm/engine/software/nvc0.c

Take a look at nvc0_software_mthd_mp_control -- that's used in the
mesa driver to... well, control those things :) They're related to
PGRAPH, which should be available on the GK20A. [Not sure what they do
though. One of them is about turning off error reporting.]

  -ilia

On Thu, Apr 3, 2014 at 12:23 AM, Ilia Mirkin <imirkin at alum.mit.edu>
wrote:
> On Wed, Apr 2, 2014 at 10:14 AM, Alexandre Courbot <gnurou at
gmail.com> wrote:
>>>>> +     /* Need to figure out how to handle sw for gk20a */
>>>>> +     if (device->chipset == 0xea)
>>>>> +             goto skip_sw_init;
>>>>
>>>> The commit message makes it sound like SW isn't needed
since gk20a
>>>> "lacks any display hardware". In that case the
comment here doesn't make
>>>> much sense.
>>
>> Correct. As far as I have looked (that is, not very far), SW methods
>> are used for display-related functions, but there might be other
>> use-cases too. Maybe someone who knows better can confirm?
>
>
http://cgit.freedesktop.org/~darktama/nouveau/tree/nvkm/engine/software/nvc0.c
>
> Take a look at nvc0_software_mthd_mp_control -- that's used in the
> mesa driver to... well, control those things :) They're related to
> PGRAPH, which should be available on the GK20A. [Not sure what they do
> though. One of them is about turning off error reporting.]
Hm, I don't know why I didn't notice that was there for now.. I
wouldn't be entirely surprised if nvidia had reserved method ids in
the 3d object class that were supposed to be used for this, rather
than from a sw object class..
>
>   -ilia