| Message ID | 1398410396-23338-7-git-send-email-acourbot@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 25, 2014 at 5:19 PM, Alexandre Courbot <acourbot@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@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@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@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
On Mon, Apr 28, 2014 at 4:57 PM, Thierry Reding <thierry.reding@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@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@gmail.com> wrote: > On Fri, Apr 25, 2014 at 5:19 PM, Alexandre Courbot <acourbot@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@gmail.com> wrote: > On Mon, Apr 28, 2014 at 11:10 AM, Ben Skeggs <skeggsb@gmail.com> wrote: >> On Fri, Apr 25, 2014 at 5:19 PM, Alexandre Courbot <acourbot@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.
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) ||
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. Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> --- drivers/gpu/drm/nouveau/core/engine/graph/nvc0.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)