[PATCH v2 5/8] drm: drm_atomic_helper: Add a new helper to deal with the writeback connector validation

[Leandro Ribeiro]

Hi,

On 10/26/21 08:34, Igor Torrente wrote:
> Add a helper function to validate the connector configuration receive in
> the encoder atomic_check by the drivers.
> 
> So the drivers don't need do these common validations themselves.
> 
> Signed-off-by: Igor Torrente <igormtorrente at gmail.com>
> ---
> V2: Move the format verification to a new helper at the drm_atomic_helper.c
>     (Thomas Zimmermann).
> ---
>  drivers/gpu/drm/drm_atomic_helper.c   | 47 +++++++++++++++++++++++++++
>  drivers/gpu/drm/vkms/vkms_writeback.c |  9 +++--
>  include/drm/drm_atomic_helper.h       |  3 ++
>  3 files changed, 54 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 2c0c6ec92820..c2653b9824b5 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -766,6 +766,53 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
>  
> +/**
> + * drm_atomic_helper_check_wb_connector_state() - Check writeback encoder state
> + * @encoder: encoder state to check
> + * @conn_state: connector state to check
> + *
> + * Checks if the wriback connector state is valid, and returns a erros if it
> + * isn't.
> + *
> + * RETURNS:
> + * Zero for success or -errno
> + */
> +int
> +drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
> +					 struct drm_connector_state *conn_state)
> +{
> +	struct drm_writeback_job *wb_job = conn_state->writeback_job;
> +	struct drm_property_blob *pixel_format_blob;
> +	bool format_supported = false;
> +	struct drm_framebuffer *fb;
> +	int i, n_formats;
> +	u32 *formats;
> +
> +	if (!wb_job || !wb_job->fb)
> +		return 0;

I think that this should be removed and that this functions should
assume that (wb_job && wb_job->fb) == true.

Actually, it's weird to have conn_state as argument and only use it to
get the wb_job. Instead, this function could receive wb_job directly.

Of course, its name/description would have to change.

> +
> +	pixel_format_blob = wb_job->connector->pixel_formats_blob_ptr;
> +	n_formats = pixel_format_blob->length / sizeof(u32);
> +	formats = pixel_format_blob->data;
> +	fb = wb_job->fb;
> +
> +	for (i = 0; i < n_formats; i++) {
> +		if (fb->format->format == formats[i]) {
> +			format_supported = true;
> +			break;
> +		}
> +	}
> +
> +	if (!format_supported) {
> +		DRM_DEBUG_KMS("Invalid pixel format %p4cc\n",
> +			      &fb->format->format);
> +		return -EINVAL;
> +	}
> +
> +	return 0;

If you do this, you can get rid of the format_supported flag:

	for(...) {
		if (fb->format->format == formats[i])
			return 0;
	}


	DRM_DEBUG_KMS(...);
	return -EINVAL;

Thanks,
Leandro Ribeiro

> +}
> +EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state);
> +
>  /**
>   * drm_atomic_helper_check_plane_state() - Check plane state for validity
>   * @plane_state: plane state to check
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index 32734cdbf6c2..42f3396c523a 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -30,6 +30,7 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
>  {
>  	struct drm_framebuffer *fb;
>  	const struct drm_display_mode *mode = &crtc_state->mode;
> +	int ret;
>  
>  	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
>  		return 0;
> @@ -41,11 +42,9 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
>  		return -EINVAL;
>  	}
>  
> -	if (fb->format->format != vkms_wb_formats[0]) {
> -		DRM_DEBUG_KMS("Invalid pixel format %p4cc\n",
> -			      &fb->format->format);
> -		return -EINVAL;
> -	}
> +	ret = drm_atomic_helper_check_wb_encoder_state(encoder, conn_state);
> +	if (ret < 0)
> +		return ret;
>  
>  	return 0;
>  }
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 4045e2507e11..3fbf695da60f 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -40,6 +40,9 @@ struct drm_private_state;
>  
>  int drm_atomic_helper_check_modeset(struct drm_device *dev,
>  				struct drm_atomic_state *state);
> +int
> +drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
> +					 struct drm_connector_state *conn_state);
>  int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
>  					const struct drm_crtc_state *crtc_state,
>  					int min_scale,
>