Nouveau - May 2014 - [PATCH v3 6/9] drm/nouveau/graph: enable when using external firmware

On Fri, Apr 25, 2014 at 5:19 PM, Alexandre Courbot <acourbot at
nvidia.com> wrote:
> nvc0_graph_ctor() would only let the graphics engine be enabled if its
> oclass has a proper microcode linked to it. This prevents GR from being
> enabled at all on chips that rely exclusively on external firmware, even
> though such a use-case is valid.
>
> Relax the conditions enabling the GR engine to also include the case
> where an external firmware has also been loaded.
I'm happy to take this patch as-is.  I do wonder if we should do
something like this though:

if (nouveau_boolopt(device->cfgopt, "NvGrUseFW",
oclass->fecs.ucode == NULL))

Which will automatically switch to external firmware if there's no
internal implementation available.

Thoughts?  This could be a separate patch even, if preferred.

Thanks,
Ben.
>
> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
> ---
>  drivers/gpu/drm/nouveau/core/engine/graph/nvc0.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/core/engine/graph/nvc0.c
b/drivers/gpu/drm/nouveau/core/engine/graph/nvc0.c
> index f3c7329da0a0..e5b75f189988 100644
> --- a/drivers/gpu/drm/nouveau/core/engine/graph/nvc0.c
> +++ b/drivers/gpu/drm/nouveau/core/engine/graph/nvc0.c
> @@ -1259,10 +1259,13 @@ nvc0_graph_ctor(struct nouveau_object *parent,
struct nouveau_object *engine,
>         struct nvc0_graph_oclass *oclass = (void *)bclass;
>         struct nouveau_device *device = nv_device(parent);
>         struct nvc0_graph_priv *priv;
> +       bool use_ext_fw, enable;
>         int ret, i;
>
> -       ret = nouveau_graph_create(parent, engine, bclass,
> -                                  (oclass->fecs.ucode != NULL),
&priv);
> +       use_ext_fw = nouveau_boolopt(device->cfgopt,
"NvGrUseFW", false);
> +       enable = use_ext_fw || oclass->fecs.ucode != NULL;
> +
> +       ret = nouveau_graph_create(parent, engine, bclass, enable,
&priv);
>         *pobject = nv_object(priv);
>         if (ret)
>                 return ret;
> @@ -1272,7 +1275,7 @@ nvc0_graph_ctor(struct nouveau_object *parent, struct
nouveau_object *engine,
>
>         priv->base.units = nvc0_graph_units;
>
> -       if (nouveau_boolopt(device->cfgopt, "NvGrUseFW",
false)) {
> +       if (use_ext_fw) {
>                 nv_info(priv, "using external firmware\n");
>                 if (nvc0_graph_ctor_fw(priv, "fuc409c",
&priv->fuc409c) ||
>                     nvc0_graph_ctor_fw(priv, "fuc409d",
&priv->fuc409d) ||
> --
> 1.9.2
>
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau

On Mon, Apr 28, 2014 at 12:10:27PM +1000, Ben Skeggs
wrote:
> On Fri, Apr 25, 2014 at 5:19 PM, Alexandre Courbot <acourbot at
nvidia.com> wrote:
> > nvc0_graph_ctor() would only let the graphics engine be enabled if its
> > oclass has a proper microcode linked to it. This prevents GR from
being
> > enabled at all on chips that rely exclusively on external firmware,
even
> > though such a use-case is valid.
> >
> > Relax the conditions enabling the GR engine to also include the case
> > where an external firmware has also been loaded.
> I'm happy to take this patch as-is.  I do wonder if we should do
> something like this though:
> 
> if (nouveau_boolopt(device->cfgopt, "NvGrUseFW",
oclass->fecs.ucode == NULL))
> 
> Which will automatically switch to external firmware if there's no
> internal implementation available.
I think that makes a lot of sense. Perhaps outputting a warning or so at
runtime when this happens would be helpful in reminding people that the
goal is to make the GPU run with nouveau firmware rather than external
firmware, and hence that there's some work left to do.

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/20140428/936133ac/attachment.sig>

On Mon, Apr 28, 2014 at 4:57 PM, Thierry Reding
<thierry.reding at gmail.com> wrote:
> On Mon, Apr 28, 2014 at 12:10:27PM +1000, Ben Skeggs wrote:
>> On Fri, Apr 25, 2014 at 5:19 PM, Alexandre Courbot <acourbot at
nvidia.com> wrote:
>> > nvc0_graph_ctor() would only let the graphics engine be enabled if
its
>> > oclass has a proper microcode linked to it. This prevents GR from
being
>> > enabled at all on chips that rely exclusively on external
firmware, even
>> > though such a use-case is valid.
>> >
>> > Relax the conditions enabling the GR engine to also include the
case
>> > where an external firmware has also been loaded.
>> I'm happy to take this patch as-is.  I do wonder if we should do
>> something like this though:
>>
>> if (nouveau_boolopt(device->cfgopt, "NvGrUseFW",
oclass->fecs.ucode == NULL))
>>
>> Which will automatically switch to external firmware if there's no
>> internal implementation available.
>
> I think that makes a lot of sense. Perhaps outputting a warning or so at
> runtime when this happens would be helpful in reminding people that the
> goal is to make the GPU run with nouveau firmware rather than external
> firmware, and hence that there's some work left to do.
That already happens with a "using external firmware" notice.
>
> Thierry

On Mon, Apr 28, 2014 at 11:10 AM, Ben Skeggs <skeggsb at gmail.com>
wrote:
> On Fri, Apr 25, 2014 at 5:19 PM, Alexandre Courbot <acourbot at
nvidia.com> wrote:
>> nvc0_graph_ctor() would only let the graphics engine be enabled if its
>> oclass has a proper microcode linked to it. This prevents GR from being
>> enabled at all on chips that rely exclusively on external firmware,
even
>> though such a use-case is valid.
>>
>> Relax the conditions enabling the GR engine to also include the case
>> where an external firmware has also been loaded.
> I'm happy to take this patch as-is.  I do wonder if we should do
> something like this though:
>
> if (nouveau_boolopt(device->cfgopt, "NvGrUseFW",
oclass->fecs.ucode == NULL))
>
> Which will automatically switch to external firmware if there's no
> internal implementation available.
>
> Thoughts?  This could be a separate patch even, if preferred.
Sure, that should work. Do you mind if I come with a follow-up patch
for this so I don't have to re-sent the current patch series? Since
this is the only comment it received so far.

Thanks,
Alex.

On Thu, May 1, 2014 at 2:53 PM, Alexandre Courbot <gnurou at gmail.com>
wrote:
> On Mon, Apr 28, 2014 at 11:10 AM, Ben Skeggs <skeggsb at gmail.com>
wrote:
>> On Fri, Apr 25, 2014 at 5:19 PM, Alexandre Courbot <acourbot at
nvidia.com> wrote:
>>> nvc0_graph_ctor() would only let the graphics engine be enabled if
its
>>> oclass has a proper microcode linked to it. This prevents GR from
being
>>> enabled at all on chips that rely exclusively on external firmware,
even
>>> though such a use-case is valid.
>>>
>>> Relax the conditions enabling the GR engine to also include the
case
>>> where an external firmware has also been loaded.
>> I'm happy to take this patch as-is.  I do wonder if we should do
>> something like this though:
>>
>> if (nouveau_boolopt(device->cfgopt, "NvGrUseFW",
oclass->fecs.ucode == NULL))
>>
>> Which will automatically switch to external firmware if there's no
>> internal implementation available.
>>
>> Thoughts?  This could be a separate patch even, if preferred.
>
> Sure, that should work. Do you mind if I come with a follow-up patch
> for this so I don't have to re-sent the current patch series? Since
> this is the only comment it received so far.
Yeah that's fine.  I suspect I'll merge this tomorrow, assuming no
further objections are raised.

Ben.
>
> Thanks,
> Alex.