Re: [PATCH v2 1/2] drm/nouveau: Fetch zcull info from device

["Danilo Krummrich" <dakr@kernel.org>]

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 <mhenning@darkrefraction.com>

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

> ---
>  drivers/gpu/drm/nouveau/include/nvkm/engine/gr.h   | 19 +++++++++++++
>  .../gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gr.c   |  9 ++++--
>  .../gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gr.c   | 32 ++++++++++++++++++++--
>  .../drm/nouveau/nvkm/subdev/gsp/rm/r570/nvrm/gr.h  | 19 +++++++++++++
>  drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/rm.h    |  2 +-
>  5 files changed, 75 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/include/nvkm/engine/gr.h b/drivers/gpu/drm/nouveau/include/nvkm/engine/gr.h
> index a2333cfe6955..490ce410f6cb 100644
> --- a/drivers/gpu/drm/nouveau/include/nvkm/engine/gr.h
> +++ b/drivers/gpu/drm/nouveau/include/nvkm/engine/gr.h
> @@ -3,9 +3,28 @@
>  #define __NVKM_GR_H__
>  #include <core/engine.h>
>  
> +struct nvkm_gr_zcull_info {
> +	__u32 width_align_pixels;
> +	__u32 height_align_pixels;
> +	__u32 pixel_squares_by_aliquots;
> +	__u32 aliquot_total;
> +	__u32 zcull_region_byte_multiplier;
> +	__u32 zcull_region_header_size;
> +	__u32 zcull_subregion_header_size;
> +	__u32 subregion_count;
> +	__u32 subregion_width_align_pixels;
> +	__u32 subregion_height_align_pixels;
> +
> +	__u32 ctxsw_size;
> +	__u32 ctxsw_align;
> +};
> +
>  struct nvkm_gr {
>  	const struct nvkm_gr_func *func;
>  	struct nvkm_engine engine;
> +
> +	struct nvkm_gr_zcull_info zcull_info;
> +	bool has_zcull_info;
>  };
>  
>  u64 nvkm_gr_units(struct nvkm_gr *);
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gr.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gr.c
> index ddb57d5e73d6..73844e1e7294 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gr.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gr.c
> @@ -249,7 +249,7 @@ r535_gr_get_ctxbuf_info(struct r535_gr *gr, int i,
>  }
>  
>  static int
> -r535_gr_get_ctxbufs_info(struct r535_gr *gr)
> +r535_gr_get_ctxbufs_and_zcull_info(struct r535_gr *gr)

Why did you combine those two callbacks? Why not extend struct nvkm_rm_api_gr
with another callback?

>  {
>  	NV2080_CTRL_INTERNAL_STATIC_GR_GET_CONTEXT_BUFFERS_INFO_PARAMS *info;
>  	struct nvkm_subdev *subdev = &gr->base.engine.subdev;
> @@ -265,6 +265,9 @@ r535_gr_get_ctxbufs_info(struct r535_gr *gr)
>  		r535_gr_get_ctxbuf_info(gr, i, &info->engineContextBuffersInfo[0].engine[i]);
>  
>  	nvkm_gsp_rm_ctrl_done(&gsp->internal.device.subdevice, info);
> +
> +	gr->base.has_zcull_info = false;
> +
>  	return 0;
>  }
>  
> @@ -312,7 +315,7 @@ r535_gr_oneinit(struct nvkm_gr *base)
>  	 *
>  	 * Also build the information that'll be used to create channel contexts.
>  	 */
> -	ret = rm->api->gr->get_ctxbufs_info(gr);
> +	ret = rm->api->gr->get_ctxbufs_and_zcull_info(gr);
>  	if (ret)
>  		goto done;
>  
> @@ -352,5 +355,5 @@ r535_gr_dtor(struct nvkm_gr *base)
>  
>  const struct nvkm_rm_api_gr
>  r535_gr = {
> -	.get_ctxbufs_info = r535_gr_get_ctxbufs_info,
> +	.get_ctxbufs_and_zcull_info = r535_gr_get_ctxbufs_and_zcull_info,
>  };
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gr.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gr.c
> index b6cced9b8aa1..3e7af2ffece9 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gr.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gr.c
> @@ -164,9 +164,10 @@ r570_gr_scrubber_init(struct r535_gr *gr)
>  }
>  
>  static int
> -r570_gr_get_ctxbufs_info(struct r535_gr *gr)
> +r570_gr_get_ctxbufs_and_zcull_info(struct r535_gr *gr)
>  {
>  	NV2080_CTRL_INTERNAL_STATIC_GR_GET_CONTEXT_BUFFERS_INFO_PARAMS *info;
> +	NV2080_CTRL_GR_GET_ZCULL_INFO_PARAMS *zcull_info;
>  	struct nvkm_subdev *subdev = &gr->base.engine.subdev;
>  	struct nvkm_gsp *gsp = subdev->device->gsp;
>  
> @@ -179,13 +180,40 @@ r570_gr_get_ctxbufs_info(struct r535_gr *gr)
>  	for (int i = 0; i < ARRAY_SIZE(info->engineContextBuffersInfo[0].engine); i++)
>  		r535_gr_get_ctxbuf_info(gr, i, &info->engineContextBuffersInfo[0].engine[i]);
>  
> +	NV2080_CTRL_INTERNAL_ENGINE_CONTEXT_BUFFER_INFO zcull = info->engineContextBuffersInfo[0]
> +		.engine[NV0080_CTRL_FIFO_GET_ENGINE_CONTEXT_PROPERTIES_ENGINE_ID_GRAPHICS_ZCULL];
> +	gr->base.zcull_info.ctxsw_size = zcull.size;
> +	gr->base.zcull_info.ctxsw_align = zcull.alignment;
> +
>  	nvkm_gsp_rm_ctrl_done(&gsp->internal.device.subdevice, info);
> +
> +	zcull_info = nvkm_gsp_rm_ctrl_rd(&gsp->internal.device.subdevice,
> +					 NV2080_CTRL_CMD_GR_GET_ZCULL_INFO,
> +					 sizeof(*zcull_info));
> +	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.

> +		return PTR_ERR(zcull_info);
> +
> +	gr->base.zcull_info.width_align_pixels = zcull_info->widthAlignPixels;
> +	gr->base.zcull_info.height_align_pixels = zcull_info->heightAlignPixels;
> +	gr->base.zcull_info.pixel_squares_by_aliquots = zcull_info->pixelSquaresByAliquots;
> +	gr->base.zcull_info.aliquot_total = zcull_info->aliquotTotal;
> +	gr->base.zcull_info.zcull_region_byte_multiplier = zcull_info->zcullRegionByteMultiplier;
> +	gr->base.zcull_info.zcull_region_header_size = zcull_info->zcullRegionHeaderSize;
> +	gr->base.zcull_info.zcull_subregion_header_size = zcull_info->zcullSubregionHeaderSize;
> +	gr->base.zcull_info.subregion_count = zcull_info->subregionCount;
> +	gr->base.zcull_info.subregion_width_align_pixels = zcull_info->subregionWidthAlignPixels;
> +	gr->base.zcull_info.subregion_height_align_pixels = zcull_info->subregionHeightAlignPixels;
> +
> +	nvkm_gsp_rm_ctrl_done(&gsp->internal.device.subdevice, zcull_info);
> +
> +	gr->base.has_zcull_info = true;
> +
>  	return 0;
>  }