Re: [PATCH V8 10/43] drm/plane: Add COLOR PIPELINE property
Two nits below, regardless:
Reviewed-by: Simon Ser
> + } else if (property == plane->color_pipeline_property) {
> + /* find DRM colorop object */
> + struct drm_colorop *colorop = NULL;
> +
> + colorop = drm_colorop_find(dev, file_priv, val);
> +
> + if (val && !colorop)
> + return -EACCES;
> +
> + /* set it on drm_plane_state */
> + drm_atomic_set_colorop_for_plane(state, colorop);
Nit: I don't think these comments are especially useful, the names of the
functions are clear enough.
> +int drm_plane_create_color_pipeline_property(struct drm_plane *plane,
> + const struct drm_prop_enum_list
> *pipelines,
> + const int num_pipelines)
Nit: in general we don't mark non-pointer arguments as const: the function
cannot mutate the caller's value anyways.
Re: [PATCH V8 12/43] Documentation/gpu: document drm_colorop
Reviewed-by: Simon Ser
Re: [PATCH V8 11/43] drm/colorop: Introduce DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE
Reviewed-by: Simon Ser
Re: [PATCH V8 30/43] drm/colorop: add BT2020/BT709 OETF and Inverse OETF
Note, this patch adds new values in the middle of the enum. This is in general a breaking uAPI change: all enum values afterwards will get re-numbered. Maybe this patch should come before the PQ 125 one. In this series it shouldn't matter either way because we're also introducing the enum, but (1) who knows what/how distros might end up cherry-picking (2) future patches might take inspiration from this one. With that fixed: Reviewed-by: Simon Ser
Re: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type
Reviewed-by: Simon Ser
Re: [PATCH V8 40/43] drm/colorop: Add 3D LUT support to color pipeline
Reviewed-by: Simon Ser
Re: [PATCH V8 20/43] drm/colorop: pass plane_color_pipeline client cap to atomic check
Reviewed-by: Simon Ser
Re: [PATCH V8 39/43] drm/colorop: allow non-bypass colorops
I'm personally not a fan of such boolean function arguments, especially when caller and callee are far apart. From the caller side, the meaning of the boolean argument is not immediately clear. I would prefer a "flags" argument, which can take a e.g. DRM_COLOROP_FLAG_ALLOW_BYPASS value. But I'm not feeling strongly about this.
Re: [PATCH V8 43/43] drm/colorop: Add destroy functions for color pipeline
I would prefer these functions to be introduced together with the patches adding functions to create objects and adding the new fields. That way it's easier to check the symmetry and at no point in the series there are memory leaks. Additionally, I would avoid using the name "cleanup", which seems to have different semantics: for instance drm_plane_cleanup() doesn't kfree the pointer. "destroy" seems more appropriate here.
Re: [PATCH V8 00/43] Color Pipeline API w/ VKMS
Thanks a lot for sending the updated series, Alex! I've looked at all of the core DRM patches and they all look pretty close to being R-b'ed. I don't think I'd have time to look at vkms or amdgpu patches. Let me know if I missed anything! Simon
