Hi Am 03.11.21 um 16:11 schrieb Leandro Ribeiro:
Hi, On 11/3/21 12:03, Igor Torrente wrote: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@xxxxxxxxx> --- 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
'writeback' 'an error'
erros if it
'error'
+ * 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;
Just 'nformats'. Please make both variables 'size_t'.
+ 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.
In regular atomic check for planes, there can be planes with no attached framebuffer. Helpers handle this situation. [1] I don't know if this is possible in writeback code, but for consistency, it would make sense to keep this test here. Not sure though.
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.Sure, that makes sense.
We had many helper functions for atomic modesetting that took various arguments for whatever they required. Extending such a function with new functionality/arguments required required touching many drivers and made the parameter list hard to read. At some point, Maxime went through most of the code and unified it all to pass full state to the helpers.
So please keep the connector state. I think it's how we do things ATM.
Thanks, Leandro RibeiroOf 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);
Please use drm_dgb_kms() instead. There's a 100-character-per-line limit. The comment probably fits onto a single line.(?)
+ 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!
Yes, that looks nicer.
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;
We usually use just 'if (ret)' for such test. No need for a less-than. Best regards Thomas[1] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_helper.c#L809
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
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
