Re: [PATCH RFC v4 3/7] drm/atomic: Move framebuffer checks to helper
From: Jessica Zhang
Date: Fri Jun 30 2023 - 14:00:09 EST
On 6/29/2023 5:43 PM, Dmitry Baryshkov wrote:
On 30/06/2023 03:25, Jessica Zhang wrote:
Currently framebuffer checks happen directly in
drm_atomic_plane_check(). Move these checks into their own helper
method.
Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>
---
drivers/gpu/drm/drm_atomic.c | 130
++++++++++++++++++++++++-------------------
1 file changed, 74 insertions(+), 56 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index b4c6ffc438da..404b984d2d9f 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -580,6 +580,76 @@ plane_switching_crtc(const struct drm_plane_state
*old_plane_state,
return true;
}
+static int drm_atomic_check_fb(const struct drm_plane_state *state)
+{
+ struct drm_plane *plane = state->plane;
+ const struct drm_framebuffer *fb = state->fb;
+ struct drm_mode_rect *clips;
+
+ uint32_t num_clips;
+ unsigned int fb_width, fb_height;
+ int ret;
+
+ /* Check whether this plane supports the fb pixel format. */
+ ret = drm_plane_check_pixel_format(plane, fb->format->format,
+ fb->modifier);
+
+ if (ret) {
+ drm_dbg_atomic(plane->dev,
+ "[PLANE:%d:%s] invalid pixel format %p4cc,
modifier 0x%llx\n",
+ plane->base.id, plane->name,
+ &fb->format->format, fb->modifier);
+ return ret;
+ }
+
+ fb_width = fb->width << 16;
+ fb_height = fb->height << 16;
+
+ /* Make sure source coordinates are inside the fb. */
+ if (state->src_w > fb_width ||
+ state->src_x > fb_width - state->src_w ||
+ state->src_h > fb_height ||
+ state->src_y > fb_height - state->src_h) {
+ drm_dbg_atomic(plane->dev,
+ "[PLANE:%d:%s] invalid source coordinates "
+ "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
+ plane->base.id, plane->name,
+ state->src_w >> 16,
+ ((state->src_w & 0xffff) * 15625) >> 10,
+ state->src_h >> 16,
+ ((state->src_h & 0xffff) * 15625) >> 10,
+ state->src_x >> 16,
+ ((state->src_x & 0xffff) * 15625) >> 10,
+ state->src_y >> 16,
+ ((state->src_y & 0xffff) * 15625) >> 10,
+ fb->width, fb->height);
+ return -ENOSPC;
+ }
+
+ clips = __drm_plane_get_damage_clips(state);
+ num_clips = drm_plane_get_damage_clips_count(state);
+
+ /* Make sure damage clips are valid and inside the fb. */
+ while (num_clips > 0) {
+ if (clips->x1 >= clips->x2 ||
+ clips->y1 >= clips->y2 ||
+ clips->x1 < 0 ||
+ clips->y1 < 0 ||
+ clips->x2 > fb_width ||
+ clips->y2 > fb_height) {
+ drm_dbg_atomic(plane->dev,
+ "[PLANE:%d:%s] invalid damage clip %d %d %d
%d\n",
+ plane->base.id, plane->name, clips->x1,
+ clips->y1, clips->x2, clips->y2);
+ return -EINVAL;
+ }
+ clips++;
+ num_clips--;
+ }
+
+ return 0;
+}
+
/**
* drm_atomic_plane_check - check plane state
* @old_plane_state: old plane state to check
@@ -596,9 +666,6 @@ static int drm_atomic_plane_check(const struct
drm_plane_state *old_plane_state,
struct drm_plane *plane = new_plane_state->plane;
struct drm_crtc *crtc = new_plane_state->crtc;
const struct drm_framebuffer *fb = new_plane_state->fb;
- unsigned int fb_width, fb_height;
- struct drm_mode_rect *clips;
- uint32_t num_clips;
int ret;
/* either *both* CRTC and FB must be set, or neither */
@@ -625,17 +692,6 @@ static int drm_atomic_plane_check(const struct
drm_plane_state *old_plane_state,
return -EINVAL;
}
- /* Check whether this plane supports the fb pixel format. */
- ret = drm_plane_check_pixel_format(plane, fb->format->format,
- fb->modifier);
- if (ret) {
- drm_dbg_atomic(plane->dev,
- "[PLANE:%d:%s] invalid pixel format %p4cc,
modifier 0x%llx\n",
- plane->base.id, plane->name,
- &fb->format->format, fb->modifier);
- return ret;
- }
-
/* Give drivers some help against integer overflows */
if (new_plane_state->crtc_w > INT_MAX ||
new_plane_state->crtc_x > INT_MAX - (int32_t)
new_plane_state->crtc_w ||
@@ -649,49 +705,11 @@ static int drm_atomic_plane_check(const struct
drm_plane_state *old_plane_state,
return -ERANGE;
}
- fb_width = fb->width << 16;
- fb_height = fb->height << 16;
- /* Make sure source coordinates are inside the fb. */
- if (new_plane_state->src_w > fb_width ||
- new_plane_state->src_x > fb_width - new_plane_state->src_w ||
- new_plane_state->src_h > fb_height ||
- new_plane_state->src_y > fb_height - new_plane_state->src_h) {
- drm_dbg_atomic(plane->dev,
- "[PLANE:%d:%s] invalid source coordinates "
- "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
- plane->base.id, plane->name,
- new_plane_state->src_w >> 16,
- ((new_plane_state->src_w & 0xffff) * 15625) >> 10,
- new_plane_state->src_h >> 16,
- ((new_plane_state->src_h & 0xffff) * 15625) >> 10,
- new_plane_state->src_x >> 16,
- ((new_plane_state->src_x & 0xffff) * 15625) >> 10,
- new_plane_state->src_y >> 16,
- ((new_plane_state->src_y & 0xffff) * 15625) >> 10,
- fb->width, fb->height);
- return -ENOSPC;
- }
-
- clips = __drm_plane_get_damage_clips(new_plane_state);
- num_clips = drm_plane_get_damage_clips_count(new_plane_state);
-
- /* Make sure damage clips are valid and inside the fb. */
- while (num_clips > 0) {
- if (clips->x1 >= clips->x2 ||
- clips->y1 >= clips->y2 ||
- clips->x1 < 0 ||
- clips->y1 < 0 ||
- clips->x2 > fb_width ||
- clips->y2 > fb_height) {
- drm_dbg_atomic(plane->dev,
- "[PLANE:%d:%s] invalid damage clip %d %d %d
%d\n",
- plane->base.id, plane->name, clips->x1,
- clips->y1, clips->x2, clips->y2);
- return -EINVAL;
- }
- clips++;
- num_clips--;
+ if (fb) {
This doesn't only move code, but also changes semantics, making the
checks optional if no FB is provided. Consider moving the condition to
the next patch. Otherwise LGTM.
Hi Dmitry,
Sounds good.
Thanks,
Jessica Zhang
+ ret = drm_atomic_check_fb(new_plane_state);
+ if (ret)
+ return ret;
}
if (plane_switching_crtc(old_plane_state, new_plane_state)) {
--
With best wishes
Dmitry