Hi Thomas,
On 11/3/21 12:37 PM, Thomas Zimmermann wrote:
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'.
I Will correct all these minor issues.
+ 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.
@Leandro, do you know if it is possible to have a wb_job without a fb
attached?
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 > So please keep the connector state. I think it's how we do things ATM.to the helpers.
So please keep the connector state. I think it's how we do things ATM.
OK, I will keep then.
Thanks,
Leandro Ribeiro
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);
Please use drm_dgb_kms() instead. There's a 100-character-per-line
limit. The comment probably fits onto a single line.(?)
I will improve that. This code came from the vkms, which follows the 80
chars limit. If I'm not mistaken.
+ 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.
I will change that.
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
Thanks,
---
Igor M. A. Torrente
