On Fri, Feb 13, 2026 at 5:22 PM Danilo Krummrich <[email protected]> wrote:
>
> On Fri Feb 13, 2026 at 10:48 PM CET, M Henning wrote:
> > On Fri, Feb 13, 2026 at 12:12 PM Danilo Krummrich <[email protected]> wrote:
> >>
> >> On Thu Feb 5, 2026 at 7:56 PM CET, Mel Henning wrote:
> >> > This information will be exposed to userspace in the following commit.
> >> >
> >> > Signed-off-by: Mel Henning <[email protected]>
> >>
> >> For someone looking at this commit, this commit message is not very useful.
> >>
> >> Please add at least a brief explanation of what the patch does and - even
> >> more
> >> important - why it does it. See also [1].
> >>
> >> [1]
> >> https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> >
> > What I'm struggling with is that I don't know how to do this without
> > repeating myself. If you want, I can copy-paste my explanation of
> > zcull here too and then it will appear three times, once in each
> > commit and once in the cover letter. But that kind of repetition
> > doesn't seem very helpful to me.
>
> Again, the commit message should explain what the commit does and why. For
> instance, I asked you why you did combine those two callbacks below.
>
> The commit message could mention this, e.g. it could be something along the
> lines
> of:
>
> "Add struct nvkm_gr_zcull_info, which serves as abstraction layer between the
> corresponding uAPI (added in a subsequent patch) and the firmware (version
> specific) structure.
>
> This is needed in order to not leak the uAPI layer into nvkm. Also note that
> we
> are bypassing the nvif layer, since ...
>
> Also note that we reuse the get_ctxbufs_info() callback, since ..."
>
> I.e. make it obvious to maintainers what's going on and what's the motivation
> for the patch and it's implementation details.
Okay, I'll give it another go.
> > Because of this, I decided that it was simplest to combine them in a
> > single call, which avoids repeated rpc calls to the gpu without the
> > complexity of handling partially complete states.
>
> Ok, that seems reasonable.
>
> >> > + if (WARN_ON(IS_ERR(zcull_info)))
> >>
> >> What justifies this WARN_ON()? To me this seems like normal error
> >> handling, i.e.
> >> it is not a violation of some API invariant, etc. Also, this is in the
> >> driver's
> >> probe() path.
> >
> > I was just copying the error handling that already exists in this function.
> >
> > I do think these are weird error cases though - they mean that the gpu
> > was partially but not fully initialized which shouldn't happen during
> > normal usage. The only cases I can think of that would trigger this
> > warning are a kernel bug or an intermittent PCI link, which I think
> > are both reasonable to warn on.
>
> It could also be that the firmware is buggy, etc. In any case, I don't see
> that
> a WARN_ON() is justified. Please use dev_err() instead.
Okay. I've now written this as:
if (IS_ERR(zcull_info)) {
nvdev_error(gr->base.engine.subdev.device, "could not fetch zcull info\n");
return PTR_ERR(zcull_info);
}
since nouveau seems to use its own nvdev_error() macro over dev_err()
