Re: [PATCH] drm/atomic: convert drm_atomic_get_{old,new}_colorop_state() into proper functions

[Alex Hung <alex.hung@amd.com>]

Hi Jani,

Some compilation errors with this patch:

drivers/gpu/drm/vkms/vkms_drv.c: In function ‘vkms_destroy’:
drivers/gpu/drm/vkms/vkms_drv.c:261:9: error: implicit declaration of 
function ‘drm_colorop_pipeline_destroy’ 
[-Werror=implicit-function-declaration]
   261 |         drm_colorop_pipeline_destroy(&config->dev->drm);
       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
make[9]: *** [scripts/Makefile.build:287: 
drivers/gpu/drm/vkms/vkms_drv.o] Error 1
make[9]: *** Waiting for unfinished jobs....
drivers/gpu/drm/vkms/vkms_composer.c: In function ‘apply_colorop’:
drivers/gpu/drm/vkms/vkms_composer.c:164:58: error: invalid use of 
undefined type ‘struct drm_colorop’
   164 |         struct drm_colorop_state *colorop_state = colorop->state;
       |                                                          ^~
drivers/gpu/drm/vkms/vkms_composer.c:165:41: error: invalid use of 
undefined type ‘struct drm_colorop’
   165 |         struct drm_device *dev = colorop->dev;
       |                                         ^~
drivers/gpu/drm/vkms/vkms_composer.c:167:20: error: invalid use of 
undefined type ‘struct drm_colorop’
   167 |         if (colorop->type == DRM_COLOROP_1D_CURVE) {
       |                    ^~
drivers/gpu/drm/vkms/vkms_composer.c:168:38: error: invalid use of 
undefined type ‘struct drm_colorop_state’
   168 |                 switch (colorop_state->curve_1d_type) {
       |                                      ^~
drivers/gpu/drm/vkms/vkms_composer.c:169:22: error: 
‘DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF’ undeclared (first use in this function)
   169 |                 case DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF:
       |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/vkms/vkms_composer.c:169:22: note: each undeclared 
identifier is reported only once for each function it appears in
drivers/gpu/drm/vkms/vkms_composer.c:174:22: error: 
‘DRM_COLOROP_1D_CURVE_SRGB_EOTF’ undeclared (first use in this 
function); did you mean ‘DRM_COLOROP_1D_CURVE’?
   174 |                 case DRM_COLOROP_1D_CURVE_SRGB_EOTF:
...


Including the drm_colorop.h in vkms_composer.c and vkms_drv.c fixes them:

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c 
b/drivers/gpu/drm/vkms/vkms_composer.c
index 3cf3f26e0d8e..cd85de4ffd03 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -5,6 +5,7 @@
  #include <drm/drm_atomic.h>
  #include <drm/drm_atomic_helper.h>
  #include <drm/drm_blend.h>
+#include <drm/drm_colorop.h>
  #include <drm/drm_fourcc.h>
  #include <drm/drm_fixed.h>
  #include <drm/drm_gem_framebuffer_helper.h>
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c 
b/drivers/gpu/drm/vkms/vkms_drv.c
index dd1402f43773..434c295f44ba 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -17,6 +17,7 @@
  #include <drm/drm_gem.h>
  #include <drm/drm_atomic.h>
  #include <drm/drm_atomic_helper.h>
+#include <drm/drm_colorop.h>
  #include <drm/drm_drv.h>
  #include <drm/drm_fbdev_shmem.h>
  #include <drm/drm_file.h>


Alex

On 12/18/25 07:15, Jani Nikula wrote:
> There is no real reason to include drm_colorop.h from drm_atomic.h, as
> drm_atomic_get_{old,new}_colorop_state() have no real reason to be
> static inline.
> 
> Convert the static inlines to proper functions, and drop the include to
> reduce the include dependencies and improve data hiding.
> 
> Fixes: cfc27680ee20 ("drm/colorop: Introduce new drm_colorop mode object")
> Cc: Simon Ser <contact@emersion.fr>
> Cc: Alex Hung <alex.hung@amd.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Daniel Stone <daniels@collabora.com>
> Cc: Melissa Wen <mwen@igalia.com>
> Cc: Sebastian Wick <sebastian.wick@redhat.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> Including the massive Cc list because I don't want to keep doing this
> afterwards. This stuff needs to be blocked and fixed during review. Just
> stop including headers from headers. It's a PITA to clean up.
> ---
>   .../amd/display/amdgpu_dm/amdgpu_dm_color.c   |  3 ++
>   drivers/gpu/drm/drm_atomic.c                  | 32 +++++++++++++++
>   drivers/gpu/drm/drm_atomic_helper.c           |  1 +
>   .../drm/i915/display/intel_display_types.h    |  1 +
>   include/drm/drm_atomic.h                      | 39 ++++---------------
>   5 files changed, 45 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> index 1dcc79b35225..20a76d81d532 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> @@ -23,6 +23,9 @@
>    * Authors: AMD
>    *
>    */
> +
> +#include <drm/drm_colorop.h>
> +
>   #include "amdgpu.h"
>   #include "amdgpu_mode.h"
>   #include "amdgpu_dm.h"
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 6d3ea8056b60..52738b80ddbe 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -641,6 +641,38 @@ drm_atomic_get_colorop_state(struct drm_atomic_state *state,
>   }
>   EXPORT_SYMBOL(drm_atomic_get_colorop_state);
>   
> +/**
> + * drm_atomic_get_old_colorop_state - get colorop state, if it exists
> + * @state: global atomic state object
> + * @colorop: colorop to grab
> + *
> + * This function returns the old colorop state for the given colorop, or
> + * NULL if the colorop is not part of the global atomic state.
> + */
> +struct drm_colorop_state *
> +drm_atomic_get_old_colorop_state(struct drm_atomic_state *state,
> +				 struct drm_colorop *colorop)
> +{
> +	return state->colorops[drm_colorop_index(colorop)].old_state;
> +}
> +EXPORT_SYMBOL(drm_atomic_get_old_colorop_state);
> +
> +/**
> + * drm_atomic_get_new_colorop_state - get colorop state, if it exists
> + * @state: global atomic state object
> + * @colorop: colorop to grab
> + *
> + * This function returns the new colorop state for the given colorop, or
> + * NULL if the colorop is not part of the global atomic state.
> + */
> +struct drm_colorop_state *
> +drm_atomic_get_new_colorop_state(struct drm_atomic_state *state,
> +				 struct drm_colorop *colorop)
> +{
> +	return state->colorops[drm_colorop_index(colorop)].new_state;
> +}
> +EXPORT_SYMBOL(drm_atomic_get_new_colorop_state);
> +
>   static bool
>   plane_switching_crtc(const struct drm_plane_state *old_plane_state,
>   		     const struct drm_plane_state *new_plane_state)
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 10adac9397cf..5840e9cc6f66 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -34,6 +34,7 @@
>   #include <drm/drm_atomic_uapi.h>
>   #include <drm/drm_blend.h>
>   #include <drm/drm_bridge.h>
> +#include <drm/drm_colorop.h>
>   #include <drm/drm_damage_helper.h>
>   #include <drm/drm_device.h>
>   #include <drm/drm_drv.h>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 6ff53cd58052..eb2e3f1e83c9 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -34,6 +34,7 @@
>   #include <drm/display/drm_dp_tunnel.h>
>   #include <drm/display/drm_dsc.h>
>   #include <drm/drm_atomic.h>
> +#include <drm/drm_colorop.h>
>   #include <drm/drm_crtc.h>
>   #include <drm/drm_encoder.h>
>   #include <drm/drm_framebuffer.h>
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 74ce26fa8838..178f8f62c80f 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -30,7 +30,6 @@
>   
>   #include <drm/drm_crtc.h>
>   #include <drm/drm_util.h>
> -#include <drm/drm_colorop.h>
>   
>   /**
>    * struct drm_crtc_commit - track modeset commits on a CRTC
> @@ -712,6 +711,14 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
>   struct drm_colorop_state *
>   drm_atomic_get_colorop_state(struct drm_atomic_state *state,
>   			     struct drm_colorop *colorop);
> +
> +struct drm_colorop_state *
> +drm_atomic_get_old_colorop_state(struct drm_atomic_state *state,
> +				 struct drm_colorop *colorop);
> +struct drm_colorop_state *
> +drm_atomic_get_new_colorop_state(struct drm_atomic_state *state,
> +				 struct drm_colorop *colorop);
> +
>   struct drm_connector_state * __must_check
>   drm_atomic_get_connector_state(struct drm_atomic_state *state,
>   			       struct drm_connector *connector);
> @@ -808,36 +815,6 @@ drm_atomic_get_new_plane_state(const struct drm_atomic_state *state,
>   	return state->planes[drm_plane_index(plane)].new_state;
>   }
>   
> -/**
> - * drm_atomic_get_old_colorop_state - get colorop state, if it exists
> - * @state: global atomic state object
> - * @colorop: colorop to grab
> - *
> - * This function returns the old colorop state for the given colorop, or
> - * NULL if the colorop is not part of the global atomic state.
> - */
> -static inline struct drm_colorop_state *
> -drm_atomic_get_old_colorop_state(struct drm_atomic_state *state,
> -				 struct drm_colorop *colorop)
> -{
> -	return state->colorops[drm_colorop_index(colorop)].old_state;
> -}
> -
> -/**
> - * drm_atomic_get_new_colorop_state - get colorop state, if it exists
> - * @state: global atomic state object
> - * @colorop: colorop to grab
> - *
> - * This function returns the new colorop state for the given colorop, or
> - * NULL if the colorop is not part of the global atomic state.
> - */
> -static inline struct drm_colorop_state *
> -drm_atomic_get_new_colorop_state(struct drm_atomic_state *state,
> -				 struct drm_colorop *colorop)
> -{
> -	return state->colorops[drm_colorop_index(colorop)].new_state;
> -}
> -
>   /**
>    * drm_atomic_get_old_connector_state - get connector state, if it exists
>    * @state: global atomic state object