On Tue, 2025-06-24 at 15:01 -0400, M Henning wrote:> We did no such error checking before this series (in fact, most of > these options have almost no error checking). Are you saying you want > to see this added in this patch series?You have a good point, but I think your change, in effect, necessitates my request. Previously, the default was no GSP-RM unless needed. Now it's yes GSP-RM, and the concept of "need" has been removed. So there's no indication any more that some GPUs need GSP-RM and some do not. So to address that, I think it makes sense to add a warning if someone tries disable GSP-RM on a GPU that is not supported in that configuration. Now, whether or not we should ignore NvGspRm=0 on Ada+ is up for debate. If I understand the code correctly, today (and still with your patches), Ada+ would fail to boot. I can't say whether or not that's a good idea. But I think a warning should be printed either way.
On Tue, Jun 24, 2025 at 3:13?PM Timur Tabi <ttabi at nvidia.com> wrote:> You have a good point, but I think your change, in effect, necessitates my request. Previously, the > default was no GSP-RM unless needed. Now it's yes GSP-RM, and the concept of "need" has been > removed. So there's no indication any more that some GPUs need GSP-RM and some do not. > > So to address that, I think it makes sense to add a warning if someone tries disable GSP-RM on a GPU > that is not supported in that configuration. > > Now, whether or not we should ignore NvGspRm=0 on Ada+ is up for debate. If I understand the code > correctly, today (and still with your patches), Ada+ would fail to boot. I can't say whether or not > that's a good idea. But I think a warning should be printed either way.This patch behaves exactly the same as DRM_NOUVEAU_GSP_DEFAULT=y kernels already behave. That being said, I'm not against the additional error checking here and can add it to the next version of this series.
On 6/27/25 03:15, M Henning wrote:> On Tue, Jun 24, 2025 at 3:13?PM Timur Tabi <ttabi at nvidia.com> wrote: >> You have a good point, but I think your change, in effect, necessitates my request. Previously, the >> default was no GSP-RM unless needed. Now it's yes GSP-RM, and the concept of "need" has been >> removed. So there's no indication any more that some GPUs need GSP-RM and some do not. >> >> So to address that, I think it makes sense to add a warning if someone tries disable GSP-RM on a GPU >> that is not supported in that configuration. >> >> Now, whether or not we should ignore NvGspRm=0 on Ada+ is up for debate. If I understand the code >> correctly, today (and still with your patches), Ada+ would fail to boot. I can't say whether or not >> that's a good idea. But I think a warning should be printed either way. > This patch behaves exactly the same as DRM_NOUVEAU_GSP_DEFAULT=y > kernels already behave. > > That being said, I'm not against the additional error checking here > and can add it to the next version of this series.Yeah, the GPUs that don't support GSP-RM can't hit the code that used fwif.enable anyway, so the series should be fine as it is. Feel free to add my: Reviewed-by: Ben Skeggs <bskeggs at nvidia.com>