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

[Igor Torrente <igormtorrente@gmail.com>]

Hi Leandro,

On 10/28/21 6:38 PM, Leandro Ribeiro wrote:
> 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@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.

Ok.

> 
> 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.

In the Thomas review of v1, he said that maybe other things could be
tested in this helper. I'm not sure what these additional checks could
be, so I tried to design the function signature expecting more things
to be added after his review.

As you can see, the helper is receiving the `drm_encoder` and doing
nothing with it.

If we, eventually, don't find anything else that this helper can do, I
will revert to something very similar (if not equal) to your proposal.
I just want to wait for Thomas's review first.

> 
> 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;
> 

Indeed. Thanks!

> 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,
>>

Thanks,
---
Igor M. A. Torrente