Re: [PATCH i-g-t v2 06/14] lib/igt_kms: Export property blob functions for output/pipe/plane, v2.

[Mika Kahola <mika.kahola@intel.com>]

Reviewed-by: Mika Kahola <mika.kahola@intel.com>

On Thu, 2017-10-12 at 13:54 +0200, Maarten Lankhorst wrote:
> With the replace_prop_blob functions we can safely replace the blob
> for
> any property, without having to care about error handling ourselves.
> 
> This will for example allow overriding color management blobs, or for
> kms_atomic set invalid mode blobs.
> 
> The color management blob functions are removed, they can now be
> replaced by direct calls to replace the properties.
> 
> Changes since v1:
> - Fix chamelium tests.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  lib/igt_kms.c     | 117 +++++++++++++++++++++++++++++++++++++++-----
> ----------
>  lib/igt_kms.h     |  19 +++++++--
>  tests/chamelium.c |   6 +--
>  tests/kms_color.c |  12 +++---
>  4 files changed, 110 insertions(+), 44 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index e7b9fbaba709..5f54021350ac 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -2443,8 +2443,88 @@ static int igt_output_commit(igt_output_t
> *output,
>  	return 0;
>  }
>  
> -static void
> -igt_pipe_replace_blob(igt_pipe_t *pipe, enum
> igt_atomic_crtc_properties prop, void *ptr, size_t length)
> +/**
> + * igt_plane_replace_prop_blob:
> + * @plane: plane to set property on.
> + * @prop: property for which the blob will be replaced.
> + * @ptr: Pointer to contents for the property.
> + * @length: Length of contents.
> + *
> + * This function will destroy the old property blob for the given
> property,
> + * and will create a new property blob with the values passed to
> this function.
> + *
> + * The new property blob will be committed when you call
> igt_display_commit(),
> + * igt_display_commit2() or igt_display_commit_atomic().
> + */
> +void
> +igt_plane_replace_prop_blob(igt_plane_t *plane, enum
> igt_atomic_plane_properties prop, const void *ptr, size_t length)
> +{
> +	igt_display_t *display = plane->pipe->display;
> +	uint64_t *blob = &plane->values[prop];
> +	uint32_t blob_id = 0;
> +
> +	if (*blob != 0)
> +		igt_assert(drmModeDestroyPropertyBlob(display-
> >drm_fd,
> +						      *blob) == 0);
> +
> +	if (length > 0)
> +		igt_assert(drmModeCreatePropertyBlob(display-
> >drm_fd,
> +						     ptr, length,
> &blob_id) == 0);
> +
> +	*blob = blob_id;
> +	igt_plane_set_prop_changed(plane, prop);
> +}
> +
> +/**
> + * igt_output_replace_prop_blob:
> + * @output: output to set property on.
> + * @prop: property for which the blob will be replaced.
> + * @ptr: Pointer to contents for the property.
> + * @length: Length of contents.
> + *
> + * This function will destroy the old property blob for the given
> property,
> + * and will create a new property blob with the values passed to
> this function.
> + *
> + * The new property blob will be committed when you call
> igt_display_commit(),
> + * igt_display_commit2() or igt_display_commit_atomic().
> + */
> +void
> +igt_output_replace_prop_blob(igt_output_t *output, enum
> igt_atomic_connector_properties prop, const void *ptr, size_t length)
> +{
> +	igt_display_t *display = output->display;
> +	uint64_t *blob = &output->values[prop];
> +	uint32_t blob_id = 0;
> +
> +	if (*blob != 0)
> +		igt_assert(drmModeDestroyPropertyBlob(display-
> >drm_fd,
> +						      *blob) == 0);
> +
> +	if (length > 0)
> +		igt_assert(drmModeCreatePropertyBlob(display-
> >drm_fd,
> +						     ptr, length,
> &blob_id) == 0);
> +
> +	*blob = blob_id;
> +	igt_output_set_prop_changed(output, prop);
> +}
> +
> +/**
> + * igt_pipe_obj_replace_prop_blob:
> + * @pipe: pipe to set property on.
> + * @prop: property for which the blob will be replaced.
> + * @ptr: Pointer to contents for the property.
> + * @length: Length of contents.
> + *
> + * This function will destroy the old property blob for the given
> property,
> + * and will create a new property blob with the values passed to
> this function.
> + *
> + * The new property blob will be committed when you call
> igt_display_commit(),
> + * igt_display_commit2() or igt_display_commit_atomic().
> + *
> + * Please use igt_output_override_mode() if you want to set
> #IGT_CRTC_MODE_ID,
> + * it works better with legacy commit.
> + */
> +void
> +igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe, enum
> igt_atomic_crtc_properties prop, const void *ptr, size_t length)
>  {
>  	igt_display_t *display = pipe->display;
>  	uint64_t *blob = &pipe->values[prop];
> @@ -2836,7 +2916,7 @@ void igt_output_override_mode(igt_output_t
> *output, drmModeModeInfo *mode)
>  
>  	if (pipe) {
>  		if (output->display->is_atomic)
> -			igt_pipe_replace_blob(pipe,
> IGT_CRTC_MODE_ID, igt_output_get_mode(output), sizeof(*mode));
> +			igt_pipe_obj_replace_prop_blob(pipe,
> IGT_CRTC_MODE_ID, igt_output_get_mode(output), sizeof(*mode));
>  		else
>  			igt_pipe_obj_set_prop_changed(pipe,
> IGT_CRTC_MODE_ID);
>  	}
> @@ -2865,7 +2945,7 @@ void igt_output_set_pipe(igt_output_t *output,
> enum pipe pipe)
>  		old_output = igt_pipe_get_output(old_pipe);
>  		if (!old_output) {
>  			if (display->is_atomic)
> -				igt_pipe_replace_blob(old_pipe,
> IGT_CRTC_MODE_ID, NULL, 0);
> +				igt_pipe_obj_replace_prop_blob(old_p
> ipe, IGT_CRTC_MODE_ID, NULL, 0);
>  			else
>  				igt_pipe_obj_set_prop_changed(old_pi
> pe, IGT_CRTC_MODE_ID);
>  
> @@ -2879,7 +2959,7 @@ void igt_output_set_pipe(igt_output_t *output,
> enum pipe pipe)
>  
>  	if (pipe_obj) {
>  		if (display->is_atomic)
> -			igt_pipe_replace_blob(pipe_obj,
> IGT_CRTC_MODE_ID, igt_output_get_mode(output),
> sizeof(drmModeModeInfo));
> +			igt_pipe_obj_replace_prop_blob(pipe_obj,
> IGT_CRTC_MODE_ID, igt_output_get_mode(output),
> sizeof(drmModeModeInfo));
>  		else
>  			igt_pipe_obj_set_prop_changed(pipe_obj,
> IGT_CRTC_MODE_ID);
>  
> @@ -2908,18 +2988,11 @@ void igt_pipe_refresh(igt_display_t *display,
> enum pipe pipe, bool force)
>  
>  		pipe_obj->values[IGT_CRTC_MODE_ID] = 0;
>  		if (output)
> -			igt_pipe_replace_blob(pipe_obj,
> IGT_CRTC_MODE_ID, igt_output_get_mode(output),
> sizeof(drmModeModeInfo));
> +			igt_pipe_obj_replace_prop_blob(pipe_obj,
> IGT_CRTC_MODE_ID, igt_output_get_mode(output),
> sizeof(drmModeModeInfo));
>  	} else
>  		igt_pipe_obj_set_prop_changed(pipe_obj,
> IGT_CRTC_MODE_ID);
>  }
>  
> -void igt_output_set_scaling_mode(igt_output_t *output, uint64_t
> scaling_mode)
> -{
> -	igt_output_set_prop_value(output,
> IGT_CONNECTOR_SCALING_MODE, scaling_mode);
> -
> -	igt_require(output->props[IGT_CONNECTOR_SCALING_MODE]);
> -}
> -
>  igt_plane_t *igt_output_get_plane(igt_output_t *output, int
> plane_idx)
>  {
>  	igt_pipe_t *pipe;
> @@ -3119,24 +3192,6 @@ void igt_pipe_request_out_fence(igt_pipe_t
> *pipe)
>  	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_OUT_FENCE_PTR,
> (ptrdiff_t)&pipe->out_fence_fd);
>  }
>  
> -void
> -igt_pipe_set_degamma_lut(igt_pipe_t *pipe, void *ptr, size_t length)
> -{
> -	igt_pipe_replace_blob(pipe, IGT_CRTC_DEGAMMA_LUT, ptr,
> length);
> -}
> -
> -void
> -igt_pipe_set_ctm_matrix(igt_pipe_t *pipe, void *ptr, size_t length)
> -{
> -	igt_pipe_replace_blob(pipe, IGT_CRTC_CTM, ptr, length);
> -}
> -
> -void
> -igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, size_t length)
> -{
> -	igt_pipe_replace_blob(pipe, IGT_CRTC_GAMMA_LUT, ptr,
> length);
> -}
> -
>  /**
>   * igt_crtc_set_background:
>   * @pipe: pipe pointer to which background color to be set
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index b53127ffef5f..2e4c23437f4e 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -366,7 +366,6 @@ const char *igt_output_name(igt_output_t
> *output);
>  drmModeModeInfo *igt_output_get_mode(igt_output_t *output);
>  void igt_output_override_mode(igt_output_t *output, drmModeModeInfo
> *mode);
>  void igt_output_set_pipe(igt_output_t *output, enum pipe pipe);
> -void igt_output_set_scaling_mode(igt_output_t *output, uint64_t
> scaling_mode);
>  igt_plane_t *igt_output_get_plane(igt_output_t *output, int
> plane_idx);
>  igt_plane_t *igt_output_get_plane_type(igt_output_t *output, int
> plane_type);
>  igt_output_t *igt_output_from_connector(igt_display_t *display,
> @@ -381,9 +380,6 @@ static inline bool
> igt_plane_supports_rotation(igt_plane_t *plane)
>  	return plane->props[IGT_PLANE_ROTATION] != 0;
>  }
>  void igt_pipe_request_out_fence(igt_pipe_t *pipe);
> -void igt_pipe_set_degamma_lut(igt_pipe_t *pipe, void *ptr, size_t
> length);
> -void igt_pipe_set_ctm_matrix(igt_pipe_t *pipe, void *ptr, size_t
> length);
> -void igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, size_t
> length);
>  
>  void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb);
>  void igt_plane_set_fence_fd(igt_plane_t *plane, int fence_fd);
> @@ -513,6 +509,10 @@ static inline bool
> igt_output_is_connected(igt_output_t *output)
>  		igt_plane_set_prop_changed(plane, prop); \
>  	} while (0)
>  
> +extern void igt_plane_replace_prop_blob(igt_plane_t *plane,
> +					enum
> igt_atomic_plane_properties prop,
> +					const void *ptr, size_t
> length);
> +
>  #define igt_output_is_prop_changed(output, prop) \
>  	(!!((output)->changed & (1 << (prop))))
>  #define igt_output_set_prop_changed(output, prop) \
> @@ -527,6 +527,10 @@ static inline bool
> igt_output_is_connected(igt_output_t *output)
>  		igt_output_set_prop_changed(output, prop); \
>  	} while (0)
>  
> +extern void igt_output_replace_prop_blob(igt_output_t *output,
> +					 enum
> igt_atomic_connector_properties prop,
> +					 const void *ptr, size_t
> length);
> +
>  #define igt_pipe_obj_is_prop_changed(pipe_obj, prop) \
>  	(!!((pipe_obj)->changed & (1 << (prop))))
>  
> @@ -554,6 +558,13 @@ static inline bool
> igt_output_is_connected(igt_output_t *output)
>  #define igt_pipe_set_prop_value(display, pipe, prop, value) \
>  	igt_pipe_obj_set_prop_value(&(display)->pipes[(pipe)], prop,
> value)
>  
> +extern void igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe_obj,
> +					   enum
> igt_atomic_crtc_properties prop,
> +					   const void *ptr, size_t
> length);
> +
> +#define igt_pipe_replace_prop_blob(display, pipe, prop, ptr, length)
> \
> +	igt_pipe_obj_replace_prop_blob(&(display)->pipes[(pipe)],
> prop, ptr, length)
> +
>  void igt_pipe_refresh(igt_display_t *display, enum pipe pipe, bool
> force);
>  
>  void igt_enable_connectors(void);
> diff --git a/tests/chamelium.c b/tests/chamelium.c
> index e3d81357b32b..ae164920e86e 100644
> --- a/tests/chamelium.c
> +++ b/tests/chamelium.c
> @@ -462,9 +462,9 @@ enable_output(data_t *data,
>  	igt_output_set_pipe(output, output->config.pipe);
>  
>  	/* Clear any color correction values that might be enabled
> */
> -	igt_pipe_set_degamma_lut(primary->pipe, NULL, 0);
> -	igt_pipe_set_gamma_lut(primary->pipe, NULL, 0);
> -	igt_pipe_set_ctm_matrix(primary->pipe, NULL, 0);
> +	igt_pipe_obj_replace_prop_blob(primary->pipe,
> IGT_CRTC_DEGAMMA_LUT, NULL, 0);
> +	igt_pipe_obj_replace_prop_blob(primary->pipe,
> IGT_CRTC_GAMMA_LUT, NULL, 0);
> +	igt_pipe_obj_replace_prop_blob(primary->pipe, IGT_CRTC_CTM,
> NULL, 0);
>  
>  	kmstest_set_connector_broadcast_rgb(display->drm_fd,
> connector,
>  					    BROADCAST_RGB_FULL);
> diff --git a/tests/kms_color.c b/tests/kms_color.c
> index bcd48d89875c..e30e6bf4e409 100644
> --- a/tests/kms_color.c
> +++ b/tests/kms_color.c
> @@ -189,7 +189,7 @@ static void set_degamma(data_t *data,
>  						   data-
> >degamma_lut_size,
>  						   data-
> >color_depth, 0);
>  
> -	igt_pipe_set_degamma_lut(pipe, lut, size);
> +	igt_pipe_obj_replace_prop_blob(pipe, IGT_CRTC_DEGAMMA_LUT,
> lut, size);
>  
>  	free(lut);
>  }
> @@ -204,7 +204,7 @@ static void set_gamma(data_t *data,
>  						   data-
> >gamma_lut_size,
>  						   data-
> >color_depth, 0);
>  
> -	igt_pipe_set_gamma_lut(pipe, lut, size);
> +		igt_pipe_obj_replace_prop_blob(pipe,
> IGT_CRTC_GAMMA_LUT, lut, size);
>  
>  	free(lut);
>  }
> @@ -224,12 +224,12 @@ static void set_ctm(igt_pipe_t *pipe, const
> double *coefficients)
>  				(int64_t) (coefficients[i] *
> ((int64_t) 1L << 32));
>  	}
>  
> -	igt_pipe_set_ctm_matrix(pipe, &ctm, sizeof(ctm));
> +	igt_pipe_obj_replace_prop_blob(pipe, IGT_CRTC_CTM, &ctm,
> sizeof(ctm));
>  }
>  
> -#define disable_degamma(pipe) igt_pipe_set_degamma_lut(pipe, NULL,
> 0)
> -#define disable_gamma(pipe) igt_pipe_set_gamma_lut(pipe, NULL, 0)
> -#define disable_ctm(pipe) igt_pipe_set_ctm_matrix(pipe, NULL, 0)
> +#define disable_degamma(pipe) igt_pipe_obj_replace_prop_blob(pipe,
> IGT_CRTC_DEGAMMA_LUT, NULL, 0)
> +#define disable_gamma(pipe) igt_pipe_obj_replace_prop_blob(pipe,
> IGT_CRTC_GAMMA_LUT, NULL, 0)
> +#define disable_ctm(pipe) igt_pipe_obj_replace_prop_blob(pipe,
> IGT_CRTC_CTM, NULL, 0)
>  
>  static void output_set_property_enum(igt_output_t *output,
>  				     const char *property,
-- 
Mika Kahola - Intel OTC

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx