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