Hi All,
This is extension to Lukasz Spintzyk earlier draft of damage interface for drm.
Bascially a new plane property is added called "DAMAGE_CLIPS" which is simply
an array of drm_rect (exported to userspace as drm_mode_rect). The clips
represents damage in framebuffer coordinate of attached fb to the plane.
Helper iterator is added to traverse the damage rectangles and get the damage
clips in framebuffer, plane or crtc coordinates as need by driver
implementation. Finally a helper to reset damage in case need full update is
required. Drivers interested in page-flip with damage should call this from
atomic_check hook.
With the RFC for atomic implementation of dirtyfb ioctl I was thinking
should we need to consider dirty_fb flags, especially
DRM_MODE_FB_DIRTY_ANNOTATE_COPY to be passed with atomic
DAMAGE_CLIPS property blob? I didn't considered that untill now. If no driver
uses that in my opinion for simplicity this can be ignored?
About overlaping of damage rectangles is also not finalized. This really
depends on driver specific implementation and can be left open-ended?
My knowledge is limited to vmwgfx so would like to hear about other driver use
cases and this can be modified in keeping other drivers need.
Going forward driver implementation for vmwgfx and user-space implementation
of kmscube/weston will be next step to test the changes.
Thanks,
Deepak
Deepak Rawat (2):
drm: Add helper iterator functions to iterate over plane damage.
drm: Add helper to validate damage during modeset_check
Lukasz Spintzyk (1):
drm: Add DAMAGE_CLIPS property to plane
drivers/gpu/drm/drm_atomic.c | 42 +++++++++
drivers/gpu/drm/drm_atomic_helper.c | 173 ++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/drm_mode_config.c | 5 ++
drivers/gpu/drm/drm_plane.c | 12 +++
include/drm/drm_atomic_helper.h | 41 +++++++++
include/drm/drm_mode_config.h | 15 ++++
include/drm/drm_plane.h | 16 ++++
include/uapi/drm/drm_mode.h | 15 ++++
8 files changed, 319 insertions(+)
--
2.7.4
This patch adds a helper which should be called by driver which enable
damage (by calling drm_plane_enable_damage_clips) from atomic_check
hook. This helper for now set the damage to NULL for the planes on crtc
which need full modeset.
The driver also need to check for other crtc properties which can
affect damage in atomic_check hook, like gamma, ctm, etc. Plane related
properties which affect damage can be handled in damage iterator.
Signed-off-by: Deepak Rawat <[email protected]>
---
drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++++++++++++++++++++++++
include/drm/drm_atomic_helper.h | 2 ++
2 files changed, 49 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 355b514..23f44ab 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3987,3 +3987,50 @@ drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,
return true;
}
EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
+
+/**
+ * drm_atomic_helper_check_damage - validate state object for damage changes
+ * @dev: DRM device
+ * @state: the driver state object
+ *
+ * Check the state object if damage is required or not. In case damage is not
+ * required e.g. need modeset, the damage blob is set to NULL.
+ *
+ * NOTE: This helper is not called by core. Those driver which enable damage
+ * using drm_plane_enable_damage_clips should call this from their @atomic_check
+ * hook.
+ */
+int drm_atomic_helper_check_damage(struct drm_device *dev,
+ struct drm_atomic_state *state)
+{
+ struct drm_crtc *crtc;
+ struct drm_plane *plane;
+ struct drm_crtc_state *crtc_state;
+ unsigned plane_mask;
+ int i;
+
+ for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+ if (!drm_atomic_crtc_needs_modeset(crtc_state))
+ continue;
+
+ plane_mask = crtc_state->plane_mask;
+ plane_mask |= crtc->state->plane_mask;
+
+ drm_for_each_plane_mask(plane, dev, plane_mask) {
+ struct drm_plane_state *plane_state =
+ drm_atomic_get_plane_state(state, plane);
+
+ if (IS_ERR(plane_state))
+ return PTR_ERR(plane_state);
+
+ if (plane_state->damage_clips) {
+ drm_property_blob_put(plane_state->damage_clips);
+ plane_state->damage_clips = NULL;
+ plane_state->num_clips = 0;
+ }
+ }
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_atomic_helper_check_damage);
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index ebd4b66..b12335c 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -224,6 +224,8 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
bool
drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,
struct drm_rect *rect);
+int drm_atomic_helper_check_damage(struct drm_device *dev,
+ struct drm_atomic_state *state);
/**
* drm_atomic_crtc_for_each_plane - iterate over planes currently attached to CRTC
--
2.7.4
With damage property in drm_plane_state, this patch adds helper iterator
to traverse the damage clips. Iterator will return the damage rectangles
in framebuffer, plane or crtc coordinates as need by driver
implementation.
Signed-off-by: Deepak Rawat <[email protected]>
---
drivers/gpu/drm/drm_atomic_helper.c | 122 ++++++++++++++++++++++++++++++++++++
include/drm/drm_atomic_helper.h | 39 ++++++++++++
2 files changed, 161 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 55b44e3..355b514 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3865,3 +3865,125 @@ void __drm_atomic_helper_private_obj_duplicate_state(struct drm_private_obj *obj
memcpy(state, obj->state, sizeof(*state));
}
EXPORT_SYMBOL(__drm_atomic_helper_private_obj_duplicate_state);
+
+/**
+ * drm_atomic_helper_damage_iter_init - initialize the damage iterator
+ * @iter: The iterator to initialize.
+ * @type: Coordinate type caller is interested in.
+ * @state: plane_state from which to iterate the damage clips.
+ * @hdisplay: Width of crtc on which plane is scanned out.
+ * @vdisplay: Height of crtc on which plane is scanned out.
+ *
+ * Initialize an iterator that is used to translate and clip a set of damage
+ * rectangles in framebuffer coordinates to plane and crtc coordinates. The type
+ * argument specify which type of coordinate to iterate in.
+ *
+ * Returns: 0 on success and negative error code on error. If an error code is
+ * returned then it means the plane state should not update.
+ */
+int
+drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
+ enum drm_atomic_helper_damage_clip_type type,
+ const struct drm_plane_state *state,
+ uint32_t hdisplay, uint32_t vdisplay)
+{
+ if (!state || !state->crtc || !state->fb)
+ return -EINVAL;
+
+ memset(iter, 0, sizeof(*iter));
+ iter->clips = (struct drm_rect *)state->damage_clips->data;
+ iter->num_clips = state->num_clips;
+ iter->type = type;
+
+ /*
+ * Full update in case of scaling or rotation. In future support for
+ * scaling/rotating damage clips can be added
+ */
+ if (state->crtc_w != (state->src_w >> 16) ||
+ state->crtc_h != state->src_h >> 16 || state->rotation != 0) {
+ iter->curr_clip = iter->num_clips;
+ return 0;
+ }
+
+ iter->fb_src.x1 = 0;
+ iter->fb_src.y1 = 0;
+ iter->fb_src.x2 = state->fb->width;
+ iter->fb_src.y2 = state->fb->height;
+
+ iter->plane_src.x1 = state->src_x >> 16;
+ iter->plane_src.y1 = state->src_y >> 16;
+ iter->plane_src.x2 = iter->plane_src.x1 + (state->src_w >> 16);
+ iter->plane_src.y2 = iter->plane_src.y1 + (state->src_h >> 16);
+ iter->translate_plane_x = -iter->plane_src.x1;
+ iter->translate_plane_y = -iter->plane_src.y1;
+
+ /* Clip plane src rect to fb dimensions */
+ drm_rect_intersect(&iter->plane_src, &iter->fb_src);
+
+ iter->crtc_src.x1 = 0;
+ iter->crtc_src.y1 = 0;
+ iter->crtc_src.x2 = hdisplay;
+ iter->crtc_src.y2 = vdisplay;
+ iter->translate_crtc_x = -(iter->plane_src.x1 - state->crtc_x);
+ iter->translate_crtc_x = -(iter->plane_src.y1 - state->crtc_y);
+
+ /* Clip crtc src rect to plane dimensions */
+ drm_rect_translate(&iter->crtc_src, -iter->translate_crtc_x,
+ -iter->translate_crtc_x);
+ drm_rect_intersect(&iter->crtc_src, &iter->plane_src);
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init);
+
+/**
+ * drm_atomic_helper_damage_iter_next - advance the damage iterator
+ * @iter: The iterator to advance.
+ * @rect: Return a rectangle in coordinate specified during iterator init.
+ *
+ * Returns: true if the output is valid, false if we've reached the end of the
+ * rectangle list. If the first call return false, means need full update.
+ */
+bool
+drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,
+ struct drm_rect *rect)
+{
+ const struct drm_rect *curr_clip;
+
+next_clip:
+ if (iter->curr_clip >= iter->num_clips)
+ return false;
+
+ curr_clip = &iter->clips[iter->curr_clip];
+ iter->curr_clip++;
+
+ rect->x1 = curr_clip->x1;
+ rect->x2 = curr_clip->x2;
+ rect->y1 = curr_clip->y1;
+ rect->y2 = curr_clip->y2;
+
+ /* Clip damage rect within fb limit */
+ if (!drm_rect_intersect(rect, &iter->fb_src))
+ goto next_clip;
+ else if (iter->type & DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_FB)
+ return true;
+
+ /* Clip damage rect within plane limit */
+ if (!drm_rect_intersect(rect, &iter->plane_src))
+ goto next_clip;
+ else if (iter->type & DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_PLANE) {
+ drm_rect_translate(rect, iter->translate_plane_x,
+ iter->translate_plane_y);
+ return true;
+ }
+
+ /* Clip damage rect within crtc limit */
+ if (!drm_rect_intersect(rect, &iter->crtc_src))
+ goto next_clip;
+
+ drm_rect_translate(rect, iter->translate_crtc_x,
+ iter->translate_crtc_y);
+
+ return true;
+}
+EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 26aaba5..ebd4b66 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -36,6 +36,37 @@ struct drm_atomic_state;
struct drm_private_obj;
struct drm_private_state;
+/**
+ * enum drm_atomic_helper_damage_clip_type - type of clips to iterator over
+ *
+ * While using drm_atomic_helper_damage_iter the type of clip coordinates caller
+ * is interested in.
+ */
+enum drm_atomic_helper_damage_clip_type {
+ DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_FB = 0x0,
+ DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_PLANE = 0x1,
+ DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_CRTC = 0x2,
+};
+
+/**
+ * struct drm_atomic_helper_damage_iter - damage clip iterator
+ *
+ * This iterator tracks state needed to walk the list of damage clips.
+ */
+struct drm_atomic_helper_damage_iter {
+ enum drm_atomic_helper_damage_clip_type type;
+ const struct drm_rect *clips;
+ uint32_t num_clips;
+ uint32_t curr_clip;
+ struct drm_rect fb_src;
+ int translate_plane_x;
+ int translate_plane_y;
+ struct drm_rect plane_src;
+ int translate_crtc_x;
+ int translate_crtc_y;
+ struct drm_rect crtc_src;
+};
+
int drm_atomic_helper_check_modeset(struct drm_device *dev,
struct drm_atomic_state *state);
int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
@@ -185,6 +216,14 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
struct drm_modeset_acquire_ctx *ctx);
void __drm_atomic_helper_private_obj_duplicate_state(struct drm_private_obj *obj,
struct drm_private_state *state);
+int
+drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
+ enum drm_atomic_helper_damage_clip_type type,
+ const struct drm_plane_state *state,
+ uint32_t hdisplay, uint32_t vdisplay);
+bool
+drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,
+ struct drm_rect *rect);
/**
* drm_atomic_crtc_for_each_plane - iterate over planes currently attached to CRTC
--
2.7.4
From: Lukasz Spintzyk <[email protected]>
Optional plane property to mark damaged regions on the plane in
framebuffer coordinates of the framebuffer attached to the plane.
The layout of blob data is simply an array of drm_mode_rect with maximum
array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
coordinates, damage clips are not in 16.16 fixed point.
Damage clips are a hint to kernel as which area of framebuffer has
changed since last page-flip. This should be helpful for some drivers
especially for virtual devices where each framebuffer change needs to
be transmitted over network, usb, etc.
Driver which are interested in enabling DAMAGE_CLIPS property for a
plane should enable this property using drm_plane_enable_damage_clips.
Signed-off-by: Lukasz Spintzyk <[email protected]>
Signed-off-by: Deepak Rawat <[email protected]>
---
drivers/gpu/drm/drm_atomic.c | 42 +++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
drivers/gpu/drm/drm_mode_config.c | 5 +++++
drivers/gpu/drm/drm_plane.c | 12 +++++++++++
include/drm/drm_mode_config.h | 15 +++++++++++++
include/drm/drm_plane.h | 16 ++++++++++++++
include/uapi/drm/drm_mode.h | 15 +++++++++++++
7 files changed, 109 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7d25c42..9226d24 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p,
}
/**
+ * drm_atomic_set_damage_for_plane - sets the damage clips property to plane
+ * @state: plane state
+ * @blob: damage clips in framebuffer coordinates
+ *
+ * Returns:
+ *
+ * Zero on success, error code on failure.
+ */
+static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
+ struct drm_property_blob *blob)
+{
+ if (blob == state->damage_clips)
+ return 0;
+
+ drm_property_blob_put(state->damage_clips);
+ state->damage_clips = NULL;
+
+ if (blob) {
+ uint32_t count = blob->length/sizeof(struct drm_rect);
+
+ if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
+ return -EINVAL;
+
+ state->damage_clips = drm_property_blob_get(blob);
+ state->num_clips = count;
+ } else {
+ state->damage_clips = NULL;
+ state->num_clips = 0;
+ }
+
+ return 0;
+}
+
+/**
* drm_atomic_get_plane_state - get plane state
* @state: global atomic state object
* @plane: plane to get state object for
@@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
state->color_encoding = val;
} else if (property == plane->color_range_property) {
state->color_range = val;
+ } else if (property == config->prop_damage_clips) {
+ struct drm_property_blob *blob =
+ drm_property_lookup_blob(dev, val);
+ int ret = drm_atomic_set_damage_for_plane(state, blob);
+ drm_property_blob_put(blob);
+ return ret;
} else if (plane->funcs->atomic_set_property) {
return plane->funcs->atomic_set_property(plane, state,
property, val);
@@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
*val = state->color_encoding;
} else if (property == plane->color_range_property) {
*val = state->color_range;
+ } else if (property == config->prop_damage_clips) {
+ *val = (state->damage_clips) ? state->damage_clips->base.id : 0;
} else if (plane->funcs->atomic_get_property) {
return plane->funcs->atomic_get_property(plane, state, property, val);
} else {
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index c356545..55b44e3 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
state->fence = NULL;
state->commit = NULL;
+ state->damage_clips = NULL;
+ state->num_clips = 0;
}
EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
@@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
if (state->commit)
drm_crtc_commit_put(state->commit);
+
+ drm_property_blob_put(state->damage_clips);
}
EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index e5c6533..e93b127 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -293,6 +293,11 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
return -ENOMEM;
dev->mode_config.prop_crtc_id = prop;
+ prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, "DAMAGE_CLIPS", 0);
+ if (!prop)
+ return -ENOMEM;
+ dev->mode_config.prop_damage_clips = prop;
+
prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
"ACTIVE");
if (!prop)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 6d2a6e4..071221b 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
return ret;
}
+
+/**
+ * drm_plane_enable_damage_clips - enable damage clips property
+ * @plane: plane on which this property to enable.
+ */
+void drm_plane_enable_damage_clips(struct drm_plane *plane)
+{
+ struct drm_device *dev = plane->dev;
+ struct drm_mode_config *config = &dev->mode_config;
+
+ drm_object_attach_property(&plane->base, config->prop_damage_clips, 0);
+}
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 7569f22..d8767da 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -628,6 +628,21 @@ struct drm_mode_config {
*/
struct drm_property *prop_crtc_id;
/**
+ * @prop_damage_clips: Optional plane property to mark damaged regions
+ * on the plane in framebuffer coordinates of the framebuffer attached
+ * to the plane.
+ *
+ * The layout of blob data is simply an array of drm_mode_rect with
+ * maximum array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike
+ * plane src coordinates, damage clips are not in 16.16 fixed point.
+ *
+ * Damage clips are a hint to kernel as which area of framebuffer has
+ * changed since last page-flip. This should be helpful
+ * for some drivers especially for virtual devices where each
+ * framebuffer change needs to be transmitted over network, usb, etc.
+ */
+ struct drm_property *prop_damage_clips;
+ /**
* @prop_active: Default atomic CRTC property to control the active
* state, which is the simplified implementation for DPMS in atomic
* drivers.
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index f7bf4a4..9f24548 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -146,6 +146,21 @@ struct drm_plane_state {
*/
struct drm_crtc_commit *commit;
+ /*
+ * @damage_clips
+ *
+ * blob property with damage as array of drm_rect in framebuffer
+ * coodinates.
+ */
+ struct drm_property_blob *damage_clips;
+
+ /*
+ * @num_clips
+ *
+ * Number of drm_rect in @damage_clips.
+ */
+ uint32_t num_clips;
+
struct drm_atomic_state *state;
};
@@ -611,6 +626,7 @@ int drm_plane_init(struct drm_device *dev,
const uint32_t *formats, unsigned int format_count,
bool is_primary);
void drm_plane_cleanup(struct drm_plane *plane);
+void drm_plane_enable_damage_clips(struct drm_plane *plane);
/**
* drm_plane_index - find the index of a registered plane
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 50bcf42..0ad0d5b 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -873,6 +873,21 @@ struct drm_mode_revoke_lease {
__u32 lessee_id;
};
+/**
+ * struct drm_mode_rect - two dimensional rectangle drm_rect exported to
+ * user-space.
+ * @x1: horizontal starting coordinate (inclusive)
+ * @y1: vertical starting coordinate (inclusive)
+ * @x2: horizontal ending coordinate (exclusive)
+ * @y2: vertical ending coordinate (exclusive)
+ */
+struct drm_mode_rect {
+ __s32 x1;
+ __s32 y1;
+ __s32 x2;
+ __s32 y2;
+};
+
#if defined(__cplusplus)
}
#endif
--
2.7.4
On Wed, Apr 04, 2018 at 04:49:05PM -0700, Deepak Rawat wrote:
> Hi All,
>
> This is extension to Lukasz Spintzyk earlier draft of damage interface for drm.
> Bascially a new plane property is added called "DAMAGE_CLIPS" which is simply
> an array of drm_rect (exported to userspace as drm_mode_rect). The clips
> represents damage in framebuffer coordinate of attached fb to the plane.
>
> Helper iterator is added to traverse the damage rectangles and get the damage
> clips in framebuffer, plane or crtc coordinates as need by driver
> implementation. Finally a helper to reset damage in case need full update is
> required. Drivers interested in page-flip with damage should call this from
> atomic_check hook.
>
> With the RFC for atomic implementation of dirtyfb ioctl I was thinking
> should we need to consider dirty_fb flags, especially
> DRM_MODE_FB_DIRTY_ANNOTATE_COPY to be passed with atomic
> DAMAGE_CLIPS property blob? I didn't considered that untill now. If no driver
> uses that in my opinion for simplicity this can be ignored?
Last time I've checked no driver nor userspace really used this. I'd drop
it for the new uapi like you've done.
> About overlaping of damage rectangles is also not finalized. This really
> depends on driver specific implementation and can be left open-ended?
Overlapping is userspace being stupid imo :-) I guess we should say that
drivers should at least not fall over overlapping rects, and if they can't
handle them, either coalesce into one big rect, or split them on their own
somehow.
> My knowledge is limited to vmwgfx so would like to hear about other driver use
> cases and this can be modified in keeping other drivers need.
>
> Going forward driver implementation for vmwgfx and user-space implementation
> of kmscube/weston will be next step to test the changes.
I think an implementation for -modesetting and weston would be perfect.
Kmscube has very littel value for damage tracking purposes (it's not a
full compositor, just renders new frame each scene). And for kms I'm not a
fan of using vendor-specific drivers to demonstrate new generic uapi - way
too big chances you're making some vendor driver specific hardcoded
assumption that doesn't hold anywhere else. Also makes it harder for
others to test-implement your uapi in their drivers.
-Daniel
>
> Thanks,
> Deepak
>
> Deepak Rawat (2):
> drm: Add helper iterator functions to iterate over plane damage.
> drm: Add helper to validate damage during modeset_check
>
> Lukasz Spintzyk (1):
> drm: Add DAMAGE_CLIPS property to plane
>
> drivers/gpu/drm/drm_atomic.c | 42 +++++++++
> drivers/gpu/drm/drm_atomic_helper.c | 173 ++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/drm_mode_config.c | 5 ++
> drivers/gpu/drm/drm_plane.c | 12 +++
> include/drm/drm_atomic_helper.h | 41 +++++++++
> include/drm/drm_mode_config.h | 15 ++++
> include/drm/drm_plane.h | 16 ++++
> include/uapi/drm/drm_mode.h | 15 ++++
> 8 files changed, 319 insertions(+)
>
> --
> 2.7.4
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
> From: Lukasz Spintzyk <[email protected]>
>
> Optional plane property to mark damaged regions on the plane in
> framebuffer coordinates of the framebuffer attached to the plane.
>
> The layout of blob data is simply an array of drm_mode_rect with maximum
> array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
> coordinates, damage clips are not in 16.16 fixed point.
>
> Damage clips are a hint to kernel as which area of framebuffer has
> changed since last page-flip. This should be helpful for some drivers
> especially for virtual devices where each framebuffer change needs to
> be transmitted over network, usb, etc.
>
> Driver which are interested in enabling DAMAGE_CLIPS property for a
> plane should enable this property using drm_plane_enable_damage_clips.
>
> Signed-off-by: Lukasz Spintzyk <[email protected]>
> Signed-off-by: Deepak Rawat <[email protected]>
The property uapi section is missing, see:
https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-properties
Plane composition feels like the best place to put this. Please use that
section to tie all the various bits together, including the helpers you're
adding in the following patches for drivers to use.
Bunch of nitpicks below, but overall I'm agreeing now with just going with
fb coordinate damage rects.
Like you say, the thing needed here now is userspace + driver actually
implementing this. I'd also say the compat helper to map the legacy
fb->dirty to this new atomic way of doing things should be included here
(gives us a lot more testing for these new paths).
Icing on the cake would be an igt to make sure kernel rejects invalid clip
rects correctly.
> ---
> drivers/gpu/drm/drm_atomic.c | 42 +++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
> drivers/gpu/drm/drm_mode_config.c | 5 +++++
> drivers/gpu/drm/drm_plane.c | 12 +++++++++++
> include/drm/drm_mode_config.h | 15 +++++++++++++
> include/drm/drm_plane.h | 16 ++++++++++++++
> include/uapi/drm/drm_mode.h | 15 +++++++++++++
> 7 files changed, 109 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7d25c42..9226d24 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p,
> }
>
> /**
> + * drm_atomic_set_damage_for_plane - sets the damage clips property to plane
> + * @state: plane state
> + * @blob: damage clips in framebuffer coordinates
> + *
> + * Returns:
> + *
> + * Zero on success, error code on failure.
> + */
> +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
> + struct drm_property_blob *blob)
> +{
> + if (blob == state->damage_clips)
> + return 0;
> +
> + drm_property_blob_put(state->damage_clips);
> + state->damage_clips = NULL;
> +
> + if (blob) {
> + uint32_t count = blob->length/sizeof(struct drm_rect);
> +
> + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
> + return -EINVAL;
> +
> + state->damage_clips = drm_property_blob_get(blob);
> + state->num_clips = count;
> + } else {
> + state->damage_clips = NULL;
> + state->num_clips = 0;
> + }
> +
> + return 0;
> +}
> +
> +/**
> * drm_atomic_get_plane_state - get plane state
> * @state: global atomic state object
> * @plane: plane to get state object for
> @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
> state->color_encoding = val;
> } else if (property == plane->color_range_property) {
> state->color_range = val;
> + } else if (property == config->prop_damage_clips) {
> + struct drm_property_blob *blob =
> + drm_property_lookup_blob(dev, val);
> + int ret = drm_atomic_set_damage_for_plane(state, blob);
There's already a helper with size-checking built-in, see
drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd
just provide a little inline helper that does the
blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).
> + drm_property_blob_put(blob);
> + return ret;
> } else if (plane->funcs->atomic_set_property) {
> return plane->funcs->atomic_set_property(plane, state,
> property, val);
> @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> *val = state->color_encoding;
> } else if (property == plane->color_range_property) {
> *val = state->color_range;
> + } else if (property == config->prop_damage_clips) {
> + *val = (state->damage_clips) ? state->damage_clips->base.id : 0;
> } else if (plane->funcs->atomic_get_property) {
> return plane->funcs->atomic_get_property(plane, state, property, val);
> } else {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index c356545..55b44e3 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>
> state->fence = NULL;
> state->commit = NULL;
> + state->damage_clips = NULL;
> + state->num_clips = 0;
> }
> EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
>
> @@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>
> if (state->commit)
> drm_crtc_commit_put(state->commit);
> +
> + drm_property_blob_put(state->damage_clips);
> }
> EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index e5c6533..e93b127 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -293,6 +293,11 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
> return -ENOMEM;
> dev->mode_config.prop_crtc_id = prop;
>
> + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, "DAMAGE_CLIPS", 0);
Bit a bikeshed, but since the coordinates are in fb pixels, not plane
pixels, I'd call this "FB_DAMAGE_CLIPS".
> + if (!prop)
> + return -ENOMEM;
> + dev->mode_config.prop_damage_clips = prop;
> +
> prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
> "ACTIVE");
> if (!prop)
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 6d2a6e4..071221b 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>
> return ret;
> }
> +
> +/**
> + * drm_plane_enable_damage_clips - enable damage clips property
> + * @plane: plane on which this property to enable.
> + */
> +void drm_plane_enable_damage_clips(struct drm_plane *plane)
> +{
> + struct drm_device *dev = plane->dev;
> + struct drm_mode_config *config = &dev->mode_config;
> +
> + drm_object_attach_property(&plane->base, config->prop_damage_clips, 0);
> +}
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 7569f22..d8767da 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -628,6 +628,21 @@ struct drm_mode_config {
> */
> struct drm_property *prop_crtc_id;
> /**
> + * @prop_damage_clips: Optional plane property to mark damaged regions
> + * on the plane in framebuffer coordinates of the framebuffer attached
> + * to the plane.
Why should we make this optional? Looks like just another thing drivers
might screw up, since we have multiple callbacks and things to set up for
proper dirty tracking.
One option I'm seeing is that if this is set, and it's an atomic driver,
then we just directly call into the default atomic fb->dirty
implementation. That way there's only 1 thing drivers need to do to set up
dirty rect tracking, and they'll get all of it.
> + *
> + * The layout of blob data is simply an array of drm_mode_rect with
> + * maximum array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike
> + * plane src coordinates, damage clips are not in 16.16 fixed point.
I honestly have no idea where this limit is from. Worth keeping? I can
easily imagine that userspace could trip over this - it's fairly high, but
not unlimited.
> + *
> + * Damage clips are a hint to kernel as which area of framebuffer has
> + * changed since last page-flip. This should be helpful
> + * for some drivers especially for virtual devices where each
> + * framebuffer change needs to be transmitted over network, usb, etc.
I'd also clarify that userspace still must render the entire screen, i.e.
make it more clear that it's really just a hint and not mandatory to only
scan out the damaged parts.
> + */
> + struct drm_property *prop_damage_clips;
> + /**
> * @prop_active: Default atomic CRTC property to control the active
> * state, which is the simplified implementation for DPMS in atomic
> * drivers.
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index f7bf4a4..9f24548 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -146,6 +146,21 @@ struct drm_plane_state {
> */
> struct drm_crtc_commit *commit;
>
> + /*
> + * @damage_clips
> + *
> + * blob property with damage as array of drm_rect in framebuffer
&drm_rect gives you a nice hyperlink in the generated docs.
> + * coodinates.
> + */
> + struct drm_property_blob *damage_clips;
> +
> + /*
> + * @num_clips
> + *
> + * Number of drm_rect in @damage_clips.
> + */
> + uint32_t num_clips;
> +
> struct drm_atomic_state *state;
> };
>
> @@ -611,6 +626,7 @@ int drm_plane_init(struct drm_device *dev,
> const uint32_t *formats, unsigned int format_count,
> bool is_primary);
> void drm_plane_cleanup(struct drm_plane *plane);
> +void drm_plane_enable_damage_clips(struct drm_plane *plane);
>
> /**
> * drm_plane_index - find the index of a registered plane
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 50bcf42..0ad0d5b 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -873,6 +873,21 @@ struct drm_mode_revoke_lease {
> __u32 lessee_id;
> };
>
> +/**
> + * struct drm_mode_rect - two dimensional rectangle drm_rect exported to
> + * user-space.
> + * @x1: horizontal starting coordinate (inclusive)
> + * @y1: vertical starting coordinate (inclusive)
> + * @x2: horizontal ending coordinate (exclusive)
> + * @y2: vertical ending coordinate (exclusive)
> + */
> +struct drm_mode_rect {
> + __s32 x1;
> + __s32 y1;
> + __s32 x2;
> + __s32 y2;
Why signed? Negative damage rects on an fb don't make sense to me. Also,
please specify what this is exactly (to avoid confusion with the 16.16
fixed point src rects), and maybe mention in the commit message why we're
not using drm_clip_rect (going to proper uapi types and 32bit makes sense
to me).
Cheers, Daniel
> +};
> +
> #if defined(__cplusplus)
> }
> #endif
> --
> 2.7.4
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Wed, Apr 04, 2018 at 04:49:07PM -0700, Deepak Rawat wrote:
> With damage property in drm_plane_state, this patch adds helper iterator
> to traverse the damage clips. Iterator will return the damage rectangles
> in framebuffer, plane or crtc coordinates as need by driver
> implementation.
>
> Signed-off-by: Deepak Rawat <[email protected]>
I'd really like selftest/unittests for this stuff. There's an awful lot of
cornercases in this here (for any of the transformed iterators at least),
and unit tests is the best way to make sure we handle them all correctly.
Bonus points if you integrate the new selftests into igt so intel CI can
run them, seel igt/tests/drm_mm.c for an example. drm_mm selftest is also
the framework I'd copy for this stuff.
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 122 ++++++++++++++++++++++++++++++++++++
> include/drm/drm_atomic_helper.h | 39 ++++++++++++
> 2 files changed, 161 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 55b44e3..355b514 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3865,3 +3865,125 @@ void __drm_atomic_helper_private_obj_duplicate_state(struct drm_private_obj *obj
> memcpy(state, obj->state, sizeof(*state));
> }
> EXPORT_SYMBOL(__drm_atomic_helper_private_obj_duplicate_state);
> +
> +/**
> + * drm_atomic_helper_damage_iter_init - initialize the damage iterator
> + * @iter: The iterator to initialize.
> + * @type: Coordinate type caller is interested in.
> + * @state: plane_state from which to iterate the damage clips.
> + * @hdisplay: Width of crtc on which plane is scanned out.
> + * @vdisplay: Height of crtc on which plane is scanned out.
> + *
> + * Initialize an iterator that is used to translate and clip a set of damage
> + * rectangles in framebuffer coordinates to plane and crtc coordinates. The type
> + * argument specify which type of coordinate to iterate in.
> + *
> + * Returns: 0 on success and negative error code on error. If an error code is
> + * returned then it means the plane state should not update.
> + */
> +int
> +drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
> + enum drm_atomic_helper_damage_clip_type type,
> + const struct drm_plane_state *state,
> + uint32_t hdisplay, uint32_t vdisplay)
> +{
> + if (!state || !state->crtc || !state->fb)
> + return -EINVAL;
> +
> + memset(iter, 0, sizeof(*iter));
> + iter->clips = (struct drm_rect *)state->damage_clips->data;
> + iter->num_clips = state->num_clips;
> + iter->type = type;
> +
> + /*
> + * Full update in case of scaling or rotation. In future support for
> + * scaling/rotating damage clips can be added
> + */
> + if (state->crtc_w != (state->src_w >> 16) ||
> + state->crtc_h != state->src_h >> 16 || state->rotation != 0) {
> + iter->curr_clip = iter->num_clips;
> + return 0;
Given there's no user of this I have no idea how this manages to provoke a
full clip rect. selftest code would be perfect for stuff like this.
Also, I think we should provide a full clip for the case of num_clips ==
0, so that driver code can simply iterate over all clips and doesn't ever
have to handle the "no clip rects provided" case as a special case itself.
> + }
> +
> + iter->fb_src.x1 = 0;
> + iter->fb_src.y1 = 0;
> + iter->fb_src.x2 = state->fb->width;
> + iter->fb_src.y2 = state->fb->height;
> +
> + iter->plane_src.x1 = state->src_x >> 16;
> + iter->plane_src.y1 = state->src_y >> 16;
> + iter->plane_src.x2 = iter->plane_src.x1 + (state->src_w >> 16);
> + iter->plane_src.y2 = iter->plane_src.y1 + (state->src_h >> 16);
> + iter->translate_plane_x = -iter->plane_src.x1;
> + iter->translate_plane_y = -iter->plane_src.y1;
> +
> + /* Clip plane src rect to fb dimensions */
> + drm_rect_intersect(&iter->plane_src, &iter->fb_src);
This smells like driver bug. Also, see Ville's recent efforts to improve
the atomic plane clipping, I think drm_plane_state already has all the
clip rects you want.
> +
> + iter->crtc_src.x1 = 0;
> + iter->crtc_src.y1 = 0;
> + iter->crtc_src.x2 = hdisplay;
> + iter->crtc_src.y2 = vdisplay;
> + iter->translate_crtc_x = -(iter->plane_src.x1 - state->crtc_x);
> + iter->translate_crtc_x = -(iter->plane_src.y1 - state->crtc_y);
> +
> + /* Clip crtc src rect to plane dimensions */
> + drm_rect_translate(&iter->crtc_src, -iter->translate_crtc_x,
> + -iter->translate_crtc_x);
We can also scale.
> + drm_rect_intersect(&iter->crtc_src, &iter->plane_src);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init);
> +
> +/**
> + * drm_atomic_helper_damage_iter_next - advance the damage iterator
> + * @iter: The iterator to advance.
> + * @rect: Return a rectangle in coordinate specified during iterator init.
> + *
> + * Returns: true if the output is valid, false if we've reached the end of the
> + * rectangle list. If the first call return false, means need full update.
> + */
> +bool
> +drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,
> + struct drm_rect *rect)
> +{
> + const struct drm_rect *curr_clip;
> +
> +next_clip:
> + if (iter->curr_clip >= iter->num_clips)
> + return false;
> +
> + curr_clip = &iter->clips[iter->curr_clip];
> + iter->curr_clip++;
> +
> + rect->x1 = curr_clip->x1;
> + rect->x2 = curr_clip->x2;
> + rect->y1 = curr_clip->y1;
> + rect->y2 = curr_clip->y2;
> +
> + /* Clip damage rect within fb limit */
> + if (!drm_rect_intersect(rect, &iter->fb_src))
> + goto next_clip;
> + else if (iter->type & DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_FB)
> + return true;
> +
> + /* Clip damage rect within plane limit */
> + if (!drm_rect_intersect(rect, &iter->plane_src))
> + goto next_clip;
> + else if (iter->type & DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_PLANE) {
> + drm_rect_translate(rect, iter->translate_plane_x,
> + iter->translate_plane_y);
> + return true;
> + }
> +
> + /* Clip damage rect within crtc limit */
> + if (!drm_rect_intersect(rect, &iter->crtc_src))
> + goto next_clip;
> +
> + drm_rect_translate(rect, iter->translate_crtc_x,
> + iter->translate_crtc_y);
> +
> + return true;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 26aaba5..ebd4b66 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -36,6 +36,37 @@ struct drm_atomic_state;
> struct drm_private_obj;
> struct drm_private_state;
>
> +/**
> + * enum drm_atomic_helper_damage_clip_type - type of clips to iterator over
> + *
> + * While using drm_atomic_helper_damage_iter the type of clip coordinates caller
> + * is interested in.
> + */
> +enum drm_atomic_helper_damage_clip_type {
> + DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_FB = 0x0,
> + DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_PLANE = 0x1,
> + DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_CRTC = 0x2,
I'm confused with what exactly these different types of iterators are
supposed to achieve. TYPE_FB makes sense, that's what vmwgfx and other
virtual drivers need to figure out which parts of a buffer to upload to
the host.
TYPE_PLANE I have no idea who needs that. I suggest we just drop it.
TYPE_CRTC is what I'd want to use for manual upload hw, were instead of
compositing the entire screen we can limit the uploaded area to 1 or 2
rectangles (depending upon how the hw works). But those drivers want all
the crtc clip rects for _all_ the planes combined, not for each plane
individually.
My suggestion is to drop TYPE_CRTC until someone needs it for a driver.
And most likely the only iterator we want for TYPE_CRTC is one that gives
you the overall damage area, including alpha/ctm/gamma/everything else,
coalesced into just 1 clip rect. So probably an entirely different
function.
Summarizing all this, I'd simplify the iterator to:
drm_atomic_helper_damage_iter_init(iter, plane_state);
And leave the other 2 use-cases to the next patch series. For crtc damage
we probably want:
drm_atomic_helper_crtc_damage(drm_atomic_state, rect)
Which internally loops over all planes and also takes all the other state
changes into account. That way you also don't have to fix the scaling
issue, since your current code only handles translation.
Another bit: drm_atomic_helper.c is huge, I'd vote to put all this stuff
into a new drm_damage_helper.[hc], including new section in drm-kms.rst
and all that. Splitting up drm_atomic_helper.[hc] is somewhere on my todo
...
Cheers, Daniel
> +};
> +
> +/**
> + * struct drm_atomic_helper_damage_iter - damage clip iterator
> + *
> + * This iterator tracks state needed to walk the list of damage clips.
> + */
> +struct drm_atomic_helper_damage_iter {
> + enum drm_atomic_helper_damage_clip_type type;
> + const struct drm_rect *clips;
> + uint32_t num_clips;
> + uint32_t curr_clip;
> + struct drm_rect fb_src;
> + int translate_plane_x;
> + int translate_plane_y;
> + struct drm_rect plane_src;
> + int translate_crtc_x;
> + int translate_crtc_y;
> + struct drm_rect crtc_src;
> +};
> +
> int drm_atomic_helper_check_modeset(struct drm_device *dev,
> struct drm_atomic_state *state);
> int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
> @@ -185,6 +216,14 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> struct drm_modeset_acquire_ctx *ctx);
> void __drm_atomic_helper_private_obj_duplicate_state(struct drm_private_obj *obj,
> struct drm_private_state *state);
> +int
> +drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
> + enum drm_atomic_helper_damage_clip_type type,
> + const struct drm_plane_state *state,
> + uint32_t hdisplay, uint32_t vdisplay);
> +bool
> +drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,
> + struct drm_rect *rect);
>
> /**
> * drm_atomic_crtc_for_each_plane - iterate over planes currently attached to CRTC
> --
> 2.7.4
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Wed, Apr 04, 2018 at 04:49:08PM -0700, Deepak Rawat wrote:
> This patch adds a helper which should be called by driver which enable
> damage (by calling drm_plane_enable_damage_clips) from atomic_check
> hook. This helper for now set the damage to NULL for the planes on crtc
> which need full modeset.
>
> The driver also need to check for other crtc properties which can
> affect damage in atomic_check hook, like gamma, ctm, etc. Plane related
> properties which affect damage can be handled in damage iterator.
>
> Signed-off-by: Deepak Rawat <[email protected]>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++++++++++++++++++++++++
> include/drm/drm_atomic_helper.h | 2 ++
> 2 files changed, 49 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 355b514..23f44ab 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3987,3 +3987,50 @@ drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,
> return true;
> }
> EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
> +
> +/**
> + * drm_atomic_helper_check_damage - validate state object for damage changes
> + * @dev: DRM device
> + * @state: the driver state object
> + *
> + * Check the state object if damage is required or not. In case damage is not
> + * required e.g. need modeset, the damage blob is set to NULL.
Why is that needed?
I can imagine that drivers unconditionally upload everything for a
modeset, and hence don't need special damage tracking. But for that it's
imo better to have a clear_damage() helper.
But even for modesets (e.g. resolution changes) I'd expect that virtual
drivers don't want to upload unecessary amounts of data. Manual upload
hw drivers probably need to upload everything, because the panel will have
forgotten all the old data once power cycled.
And if you think this is really the right thing, then we need to rename
the function to tell what it does, e.g.
drm_damage_helper_clear_damage_on_modeset()
drm_damage_helper because I think we should stuff this all into
drm_damage_helper.c, see previous patch.
But I think a
drm_damage_helper_clear_damage(crtc_state)
which you can use in your crtc->atomic_check function like
crtc_atomic_check(state)
{
if (drm_atomic_crtc_needs_modeset(state))
drm_damage_helper_clear_damage(state);
}
is more flexible and useful for drivers. There might be other cases where
clearing damage is the right thing to do. Also, there's the question of
whether no damage clips == full damage or not, so maybe we should call
this helper full_damage() instead.
-Daniel
> + *
> + * NOTE: This helper is not called by core. Those driver which enable damage
> + * using drm_plane_enable_damage_clips should call this from their @atomic_check
> + * hook.
> + */
> +int drm_atomic_helper_check_damage(struct drm_device *dev,
> + struct drm_atomic_state *state)
> +{
> + struct drm_crtc *crtc;
> + struct drm_plane *plane;
> + struct drm_crtc_state *crtc_state;
> + unsigned plane_mask;
> + int i;
> +
> + for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> + if (!drm_atomic_crtc_needs_modeset(crtc_state))
> + continue;
> +
> + plane_mask = crtc_state->plane_mask;
> + plane_mask |= crtc->state->plane_mask;
> +
> + drm_for_each_plane_mask(plane, dev, plane_mask) {
> + struct drm_plane_state *plane_state =
> + drm_atomic_get_plane_state(state, plane);
> +
> + if (IS_ERR(plane_state))
> + return PTR_ERR(plane_state);
> +
> + if (plane_state->damage_clips) {
> + drm_property_blob_put(plane_state->damage_clips);
> + plane_state->damage_clips = NULL;
> + plane_state->num_clips = 0;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_check_damage);
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index ebd4b66..b12335c 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -224,6 +224,8 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
> bool
> drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,
> struct drm_rect *rect);
> +int drm_atomic_helper_check_damage(struct drm_device *dev,
> + struct drm_atomic_state *state);
>
> /**
> * drm_atomic_crtc_for_each_plane - iterate over planes currently attached to CRTC
> --
> 2.7.4
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On 04/05/2018 09:52 AM, Daniel Vetter wrote:
> On Wed, Apr 04, 2018 at 04:49:07PM -0700, Deepak Rawat wrote:
>> With damage property in drm_plane_state, this patch adds helper iterator
>> to traverse the damage clips. Iterator will return the damage rectangles
>> in framebuffer, plane or crtc coordinates as need by driver
>> implementation.
>>
>> Signed-off-by: Deepak Rawat <[email protected]>
> I'd really like selftest/unittests for this stuff. There's an awful lot of
> cornercases in this here (for any of the transformed iterators at least),
> and unit tests is the best way to make sure we handle them all correctly.
>
> Bonus points if you integrate the new selftests into igt so intel CI can
> run them, seel igt/tests/drm_mm.c for an example. drm_mm selftest is also
> the framework I'd copy for this stuff.
>
>> ---
>> drivers/gpu/drm/drm_atomic_helper.c | 122 ++++++++++++++++++++++++++++++++++++
>> include/drm/drm_atomic_helper.h | 39 ++++++++++++
>> 2 files changed, 161 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 55b44e3..355b514 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -3865,3 +3865,125 @@ void __drm_atomic_helper_private_obj_duplicate_state(struct drm_private_obj *obj
>> memcpy(state, obj->state, sizeof(*state));
>> }
>> EXPORT_SYMBOL(__drm_atomic_helper_private_obj_duplicate_state);
>> +
>> +/**
>> + * drm_atomic_helper_damage_iter_init - initialize the damage iterator
>> + * @iter: The iterator to initialize.
>> + * @type: Coordinate type caller is interested in.
>> + * @state: plane_state from which to iterate the damage clips.
>> + * @hdisplay: Width of crtc on which plane is scanned out.
>> + * @vdisplay: Height of crtc on which plane is scanned out.
>> + *
>> + * Initialize an iterator that is used to translate and clip a set of damage
>> + * rectangles in framebuffer coordinates to plane and crtc coordinates. The type
>> + * argument specify which type of coordinate to iterate in.
>> + *
>> + * Returns: 0 on success and negative error code on error. If an error code is
>> + * returned then it means the plane state should not update.
>> + */
>> +int
>> +drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
>> + enum drm_atomic_helper_damage_clip_type type,
>> + const struct drm_plane_state *state,
>> + uint32_t hdisplay, uint32_t vdisplay)
>> +{
>> + if (!state || !state->crtc || !state->fb)
>> + return -EINVAL;
>> +
>> + memset(iter, 0, sizeof(*iter));
>> + iter->clips = (struct drm_rect *)state->damage_clips->data;
>> + iter->num_clips = state->num_clips;
>> + iter->type = type;
>> +
>> + /*
>> + * Full update in case of scaling or rotation. In future support for
>> + * scaling/rotating damage clips can be added
>> + */
>> + if (state->crtc_w != (state->src_w >> 16) ||
>> + state->crtc_h != state->src_h >> 16 || state->rotation != 0) {
>> + iter->curr_clip = iter->num_clips;
>> + return 0;
> Given there's no user of this I have no idea how this manages to provoke a
> full clip rect. selftest code would be perfect for stuff like this.
>
> Also, I think we should provide a full clip for the case of num_clips ==
> 0, so that driver code can simply iterate over all clips and doesn't ever
> have to handle the "no clip rects provided" case as a special case itself.
>
>> + }
>> +
>> + iter->fb_src.x1 = 0;
>> + iter->fb_src.y1 = 0;
>> + iter->fb_src.x2 = state->fb->width;
>> + iter->fb_src.y2 = state->fb->height;
>> +
>> + iter->plane_src.x1 = state->src_x >> 16;
>> + iter->plane_src.y1 = state->src_y >> 16;
>> + iter->plane_src.x2 = iter->plane_src.x1 + (state->src_w >> 16);
>> + iter->plane_src.y2 = iter->plane_src.y1 + (state->src_h >> 16);
>> + iter->translate_plane_x = -iter->plane_src.x1;
>> + iter->translate_plane_y = -iter->plane_src.y1;
>> +
>> + /* Clip plane src rect to fb dimensions */
>> + drm_rect_intersect(&iter->plane_src, &iter->fb_src);
> This smells like driver bug. Also, see Ville's recent efforts to improve
> the atomic plane clipping, I think drm_plane_state already has all the
> clip rects you want.
>
>> +
>> + iter->crtc_src.x1 = 0;
>> + iter->crtc_src.y1 = 0;
>> + iter->crtc_src.x2 = hdisplay;
>> + iter->crtc_src.y2 = vdisplay;
>> + iter->translate_crtc_x = -(iter->plane_src.x1 - state->crtc_x);
>> + iter->translate_crtc_x = -(iter->plane_src.y1 - state->crtc_y);
>> +
>> + /* Clip crtc src rect to plane dimensions */
>> + drm_rect_translate(&iter->crtc_src, -iter->translate_crtc_x,
>> + -iter->translate_crtc_x);
> We can also scale.
I suggest we leave out scaling for now until someone actually needs it.
>
>> + drm_rect_intersect(&iter->crtc_src, &iter->plane_src);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init);
>> +
>> +/**
>> + * drm_atomic_helper_damage_iter_next - advance the damage iterator
>> + * @iter: The iterator to advance.
>> + * @rect: Return a rectangle in coordinate specified during iterator init.
>> + *
>> + * Returns: true if the output is valid, false if we've reached the end of the
>> + * rectangle list. If the first call return false, means need full update.
>> + */
>> +bool
>> +drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,
>> + struct drm_rect *rect)
>> +{
>> + const struct drm_rect *curr_clip;
>> +
>> +next_clip:
>> + if (iter->curr_clip >= iter->num_clips)
>> + return false;
>> +
>> + curr_clip = &iter->clips[iter->curr_clip];
>> + iter->curr_clip++;
>> +
>> + rect->x1 = curr_clip->x1;
>> + rect->x2 = curr_clip->x2;
>> + rect->y1 = curr_clip->y1;
>> + rect->y2 = curr_clip->y2;
>> +
>> + /* Clip damage rect within fb limit */
>> + if (!drm_rect_intersect(rect, &iter->fb_src))
>> + goto next_clip;
>> + else if (iter->type & DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_FB)
>> + return true;
>> +
>> + /* Clip damage rect within plane limit */
>> + if (!drm_rect_intersect(rect, &iter->plane_src))
>> + goto next_clip;
>> + else if (iter->type & DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_PLANE) {
>> + drm_rect_translate(rect, iter->translate_plane_x,
>> + iter->translate_plane_y);
>> + return true;
>> + }
>> +
>> + /* Clip damage rect within crtc limit */
>> + if (!drm_rect_intersect(rect, &iter->crtc_src))
>> + goto next_clip;
>> +
>> + drm_rect_translate(rect, iter->translate_crtc_x,
>> + iter->translate_crtc_y);
>> +
>> + return true;
>> +}
>> +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
>> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
>> index 26aaba5..ebd4b66 100644
>> --- a/include/drm/drm_atomic_helper.h
>> +++ b/include/drm/drm_atomic_helper.h
>> @@ -36,6 +36,37 @@ struct drm_atomic_state;
>> struct drm_private_obj;
>> struct drm_private_state;
>>
>> +/**
>> + * enum drm_atomic_helper_damage_clip_type - type of clips to iterator over
>> + *
>> + * While using drm_atomic_helper_damage_iter the type of clip coordinates caller
>> + * is interested in.
>> + */
>> +enum drm_atomic_helper_damage_clip_type {
>> + DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_FB = 0x0,
>> + DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_PLANE = 0x1,
>> + DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_CRTC = 0x2,
> I'm confused with what exactly these different types of iterators are
> supposed to achieve. TYPE_FB makes sense, that's what vmwgfx and other
> virtual drivers need to figure out which parts of a buffer to upload to
> the host.
>
> TYPE_PLANE I have no idea who needs that. I suggest we just drop it.
>
> TYPE_CRTC is what I'd want to use for manual upload hw, were instead of
> compositing the entire screen we can limit the uploaded area to 1 or 2
> rectangles (depending upon how the hw works). But those drivers want all
> the crtc clip rects for _all_ the planes combined, not for each plane
> individually.
>
> My suggestion is to drop TYPE_CRTC until someone needs it for a driver.
> And most likely the only iterator we want for TYPE_CRTC is one that gives
> you the overall damage area, including alpha/ctm/gamma/everything else,
> coalesced into just 1 clip rect. So probably an entirely different
> function.
Actually for vmwgfx, the display part of the hardware comes in different
versions and we'd typically expect both the FB coordinates and the CRTC
coordinates. In the latest version the surface backing the screen needs
to exactly fit the CRTC scanout area, and if the FB doesn't do that, we
need to create a surface that does and copy in the kernel driver. After
that, as a second step, we need to notify the display system of the
damaged area in CRTC coordinates.
> Summarizing all this, I'd simplify the iterator to:
>
> drm_atomic_helper_damage_iter_init(iter, plane_state);
>
> And leave the other 2 use-cases to the next patch series. For crtc damage
> we probably want:
>
> drm_atomic_helper_crtc_damage(drm_atomic_state, rect)
>
> Which internally loops over all planes and also takes all the other state
> changes into account. That way you also don't have to fix the scaling
> issue, since your current code only handles translation.
>
> Another bit: drm_atomic_helper.c is huge, I'd vote to put all this stuff
> into a new drm_damage_helper.[hc], including new section in drm-kms.rst
> and all that. Splitting up drm_atomic_helper.[hc] is somewhere on my todo
> ...
>
> Cheers, Daniel
/Thomas
On 04/05/2018 09:52 AM, Daniel Vetter wrote:
>
> TYPE_PLANE I have no idea who needs that. I suggest we just drop it.
I'm assuming CRTC plane coordinates here. They are used for uploading
contents of hardware planes. Like, in the simplest case, cursor images.
/Thomas
On 04/05/2018 09:35 AM, Daniel Vetter wrote:
> On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
>> From: Lukasz Spintzyk <[email protected]>
>>
>> Optional plane property to mark damaged regions on the plane in
>> framebuffer coordinates of the framebuffer attached to the plane.
>>
>> The layout of blob data is simply an array of drm_mode_rect with maximum
>> array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
>> coordinates, damage clips are not in 16.16 fixed point.
>>
>> Damage clips are a hint to kernel as which area of framebuffer has
>> changed since last page-flip. This should be helpful for some drivers
>> especially for virtual devices where each framebuffer change needs to
>> be transmitted over network, usb, etc.
>>
>> Driver which are interested in enabling DAMAGE_CLIPS property for a
>> plane should enable this property using drm_plane_enable_damage_clips.
>>
>> Signed-off-by: Lukasz Spintzyk <[email protected]>
>> Signed-off-by: Deepak Rawat <[email protected]>
> The property uapi section is missing, see:
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-2Dcomposition-2Dproperties&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ&m=J31YNV1uz3IVRaorM-eo48msi9__sQurqRZrig2UE1s&s=vzsuquF1agbOw54HSC_18Kk2k9j83m8RusmDCtPAFWQ&e=
>
> Plane composition feels like the best place to put this. Please use that
> section to tie all the various bits together, including the helpers you're
> adding in the following patches for drivers to use.
>
> Bunch of nitpicks below, but overall I'm agreeing now with just going with
> fb coordinate damage rects.
>
> Like you say, the thing needed here now is userspace + driver actually
> implementing this. I'd also say the compat helper to map the legacy
> fb->dirty to this new atomic way of doing things should be included here
> (gives us a lot more testing for these new paths).
>
> Icing on the cake would be an igt to make sure kernel rejects invalid clip
> rects correctly.
>
>> ---
>> drivers/gpu/drm/drm_atomic.c | 42 +++++++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
>> drivers/gpu/drm/drm_mode_config.c | 5 +++++
>> drivers/gpu/drm/drm_plane.c | 12 +++++++++++
>> include/drm/drm_mode_config.h | 15 +++++++++++++
>> include/drm/drm_plane.h | 16 ++++++++++++++
>> include/uapi/drm/drm_mode.h | 15 +++++++++++++
>> 7 files changed, 109 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 7d25c42..9226d24 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p,
>> }
>>
>> /**
>> + * drm_atomic_set_damage_for_plane - sets the damage clips property to plane
>> + * @state: plane state
>> + * @blob: damage clips in framebuffer coordinates
>> + *
>> + * Returns:
>> + *
>> + * Zero on success, error code on failure.
>> + */
>> +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
>> + struct drm_property_blob *blob)
>> +{
>> + if (blob == state->damage_clips)
>> + return 0;
>> +
>> + drm_property_blob_put(state->damage_clips);
>> + state->damage_clips = NULL;
>> +
>> + if (blob) {
>> + uint32_t count = blob->length/sizeof(struct drm_rect);
>> +
>> + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
>> + return -EINVAL;
>> +
>> + state->damage_clips = drm_property_blob_get(blob);
>> + state->num_clips = count;
>> + } else {
>> + state->damage_clips = NULL;
>> + state->num_clips = 0;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> * drm_atomic_get_plane_state - get plane state
>> * @state: global atomic state object
>> * @plane: plane to get state object for
>> @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>> state->color_encoding = val;
>> } else if (property == plane->color_range_property) {
>> state->color_range = val;
>> + } else if (property == config->prop_damage_clips) {
>> + struct drm_property_blob *blob =
>> + drm_property_lookup_blob(dev, val);
>> + int ret = drm_atomic_set_damage_for_plane(state, blob);
> There's already a helper with size-checking built-in, see
> drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd
> just provide a little inline helper that does the
> blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).
>
>> + drm_property_blob_put(blob);
>> + return ret;
>> } else if (plane->funcs->atomic_set_property) {
>> return plane->funcs->atomic_set_property(plane, state,
>> property, val);
>> @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>> *val = state->color_encoding;
>> } else if (property == plane->color_range_property) {
>> *val = state->color_range;
>> + } else if (property == config->prop_damage_clips) {
>> + *val = (state->damage_clips) ? state->damage_clips->base.id : 0;
>> } else if (plane->funcs->atomic_get_property) {
>> return plane->funcs->atomic_get_property(plane, state, property, val);
>> } else {
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index c356545..55b44e3 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>>
>> state->fence = NULL;
>> state->commit = NULL;
>> + state->damage_clips = NULL;
>> + state->num_clips = 0;
>> }
>> EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
>>
>> @@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>>
>> if (state->commit)
>> drm_crtc_commit_put(state->commit);
>> +
>> + drm_property_blob_put(state->damage_clips);
>> }
>> EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>>
>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
>> index e5c6533..e93b127 100644
>> --- a/drivers/gpu/drm/drm_mode_config.c
>> +++ b/drivers/gpu/drm/drm_mode_config.c
>> @@ -293,6 +293,11 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>> return -ENOMEM;
>> dev->mode_config.prop_crtc_id = prop;
>>
>> + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, "DAMAGE_CLIPS", 0);
> Bit a bikeshed, but since the coordinates are in fb pixels, not plane
> pixels, I'd call this "FB_DAMAGE_CLIPS".
>
>> + if (!prop)
>> + return -ENOMEM;
>> + dev->mode_config.prop_damage_clips = prop;
>> +
>> prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
>> "ACTIVE");
>> if (!prop)
>> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>> index 6d2a6e4..071221b 100644
>> --- a/drivers/gpu/drm/drm_plane.c
>> +++ b/drivers/gpu/drm/drm_plane.c
>> @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>>
>> return ret;
>> }
>> +
>> +/**
>> + * drm_plane_enable_damage_clips - enable damage clips property
>> + * @plane: plane on which this property to enable.
>> + */
>> +void drm_plane_enable_damage_clips(struct drm_plane *plane)
>> +{
>> + struct drm_device *dev = plane->dev;
>> + struct drm_mode_config *config = &dev->mode_config;
>> +
>> + drm_object_attach_property(&plane->base, config->prop_damage_clips, 0);
>> +}
>> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
>> index 7569f22..d8767da 100644
>> --- a/include/drm/drm_mode_config.h
>> +++ b/include/drm/drm_mode_config.h
>> @@ -628,6 +628,21 @@ struct drm_mode_config {
>> */
>> struct drm_property *prop_crtc_id;
>> /**
>> + * @prop_damage_clips: Optional plane property to mark damaged regions
>> + * on the plane in framebuffer coordinates of the framebuffer attached
>> + * to the plane.
> Why should we make this optional? Looks like just another thing drivers
> might screw up, since we have multiple callbacks and things to set up for
> proper dirty tracking.
>
> One option I'm seeing is that if this is set, and it's an atomic driver,
> then we just directly call into the default atomic fb->dirty
> implementation. That way there's only 1 thing drivers need to do to set up
> dirty rect tracking, and they'll get all of it.
>
>> + *
>> + * The layout of blob data is simply an array of drm_mode_rect with
>> + * maximum array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike
>> + * plane src coordinates, damage clips are not in 16.16 fixed point.
> I honestly have no idea where this limit is from. Worth keeping? I can
> easily imagine that userspace could trip over this - it's fairly high, but
> not unlimited.
>
>> + *
>> + * Damage clips are a hint to kernel as which area of framebuffer has
>> + * changed since last page-flip. This should be helpful
>> + * for some drivers especially for virtual devices where each
>> + * framebuffer change needs to be transmitted over network, usb, etc.
> I'd also clarify that userspace still must render the entire screen, i.e.
> make it more clear that it's really just a hint and not mandatory to only
> scan out the damaged parts.
>
>> + */
>> + struct drm_property *prop_damage_clips;
>> + /**
>> * @prop_active: Default atomic CRTC property to control the active
>> * state, which is the simplified implementation for DPMS in atomic
>> * drivers.
>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>> index f7bf4a4..9f24548 100644
>> --- a/include/drm/drm_plane.h
>> +++ b/include/drm/drm_plane.h
>> @@ -146,6 +146,21 @@ struct drm_plane_state {
>> */
>> struct drm_crtc_commit *commit;
>>
>> + /*
>> + * @damage_clips
>> + *
>> + * blob property with damage as array of drm_rect in framebuffer
> &drm_rect gives you a nice hyperlink in the generated docs.
>
>> + * coodinates.
>> + */
>> + struct drm_property_blob *damage_clips;
>> +
>> + /*
>> + * @num_clips
>> + *
>> + * Number of drm_rect in @damage_clips.
>> + */
>> + uint32_t num_clips;
>> +
>> struct drm_atomic_state *state;
>> };
>>
>> @@ -611,6 +626,7 @@ int drm_plane_init(struct drm_device *dev,
>> const uint32_t *formats, unsigned int format_count,
>> bool is_primary);
>> void drm_plane_cleanup(struct drm_plane *plane);
>> +void drm_plane_enable_damage_clips(struct drm_plane *plane);
>>
>> /**
>> * drm_plane_index - find the index of a registered plane
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index 50bcf42..0ad0d5b 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -873,6 +873,21 @@ struct drm_mode_revoke_lease {
>> __u32 lessee_id;
>> };
>>
>> +/**
>> + * struct drm_mode_rect - two dimensional rectangle drm_rect exported to
>> + * user-space.
>> + * @x1: horizontal starting coordinate (inclusive)
>> + * @y1: vertical starting coordinate (inclusive)
>> + * @x2: horizontal ending coordinate (exclusive)
>> + * @y2: vertical ending coordinate (exclusive)
>> + */
>> +struct drm_mode_rect {
>> + __s32 x1;
>> + __s32 y1;
>> + __s32 x2;
>> + __s32 y2;
> Why signed? Negative damage rects on an fb don't make sense to me. Also,
> please specify what this is exactly (to avoid confusion with the 16.16
> fixed point src rects), and maybe mention in the commit message why we're
> not using drm_clip_rect (going to proper uapi types and 32bit makes sense
> to me).
IMO, while we don't expect negative damage coordinates,
to avoid yet another drm uapi rect in the future when we actually need
negative numbers signed is a good choice...
/Thomas
On Thu, Apr 05, 2018 at 11:00:15AM +0200, Thomas Hellstrom wrote:
> On 04/05/2018 09:35 AM, Daniel Vetter wrote:
> > On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
> > > From: Lukasz Spintzyk <[email protected]>
> > >
> > > Optional plane property to mark damaged regions on the plane in
> > > framebuffer coordinates of the framebuffer attached to the plane.
> > >
> > > The layout of blob data is simply an array of drm_mode_rect with maximum
> > > array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
> > > coordinates, damage clips are not in 16.16 fixed point.
> > >
> > > Damage clips are a hint to kernel as which area of framebuffer has
> > > changed since last page-flip. This should be helpful for some drivers
> > > especially for virtual devices where each framebuffer change needs to
> > > be transmitted over network, usb, etc.
> > >
> > > Driver which are interested in enabling DAMAGE_CLIPS property for a
> > > plane should enable this property using drm_plane_enable_damage_clips.
> > >
> > > Signed-off-by: Lukasz Spintzyk <[email protected]>
> > > Signed-off-by: Deepak Rawat <[email protected]>
> > The property uapi section is missing, see:
> >
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-2Dcomposition-2Dproperties&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ&m=J31YNV1uz3IVRaorM-eo48msi9__sQurqRZrig2UE1s&s=vzsuquF1agbOw54HSC_18Kk2k9j83m8RusmDCtPAFWQ&e=
> >
> > Plane composition feels like the best place to put this. Please use that
> > section to tie all the various bits together, including the helpers you're
> > adding in the following patches for drivers to use.
> >
> > Bunch of nitpicks below, but overall I'm agreeing now with just going with
> > fb coordinate damage rects.
> >
> > Like you say, the thing needed here now is userspace + driver actually
> > implementing this. I'd also say the compat helper to map the legacy
> > fb->dirty to this new atomic way of doing things should be included here
> > (gives us a lot more testing for these new paths).
> >
> > Icing on the cake would be an igt to make sure kernel rejects invalid clip
> > rects correctly.
> >
> > > ---
> > > drivers/gpu/drm/drm_atomic.c | 42 +++++++++++++++++++++++++++++++++++++
> > > drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
> > > drivers/gpu/drm/drm_mode_config.c | 5 +++++
> > > drivers/gpu/drm/drm_plane.c | 12 +++++++++++
> > > include/drm/drm_mode_config.h | 15 +++++++++++++
> > > include/drm/drm_plane.h | 16 ++++++++++++++
> > > include/uapi/drm/drm_mode.h | 15 +++++++++++++
> > > 7 files changed, 109 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index 7d25c42..9226d24 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p,
> > > }
> > > /**
> > > + * drm_atomic_set_damage_for_plane - sets the damage clips property to plane
> > > + * @state: plane state
> > > + * @blob: damage clips in framebuffer coordinates
> > > + *
> > > + * Returns:
> > > + *
> > > + * Zero on success, error code on failure.
> > > + */
> > > +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
> > > + struct drm_property_blob *blob)
> > > +{
> > > + if (blob == state->damage_clips)
> > > + return 0;
> > > +
> > > + drm_property_blob_put(state->damage_clips);
> > > + state->damage_clips = NULL;
> > > +
> > > + if (blob) {
> > > + uint32_t count = blob->length/sizeof(struct drm_rect);
> > > +
> > > + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
> > > + return -EINVAL;
> > > +
> > > + state->damage_clips = drm_property_blob_get(blob);
> > > + state->num_clips = count;
> > > + } else {
> > > + state->damage_clips = NULL;
> > > + state->num_clips = 0;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > * drm_atomic_get_plane_state - get plane state
> > > * @state: global atomic state object
> > > * @plane: plane to get state object for
> > > @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
> > > state->color_encoding = val;
> > > } else if (property == plane->color_range_property) {
> > > state->color_range = val;
> > > + } else if (property == config->prop_damage_clips) {
> > > + struct drm_property_blob *blob =
> > > + drm_property_lookup_blob(dev, val);
> > > + int ret = drm_atomic_set_damage_for_plane(state, blob);
> > There's already a helper with size-checking built-in, see
> > drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd
> > just provide a little inline helper that does the
> > blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).
> >
> > > + drm_property_blob_put(blob);
> > > + return ret;
> > > } else if (plane->funcs->atomic_set_property) {
> > > return plane->funcs->atomic_set_property(plane, state,
> > > property, val);
> > > @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> > > *val = state->color_encoding;
> > > } else if (property == plane->color_range_property) {
> > > *val = state->color_range;
> > > + } else if (property == config->prop_damage_clips) {
> > > + *val = (state->damage_clips) ? state->damage_clips->base.id : 0;
> > > } else if (plane->funcs->atomic_get_property) {
> > > return plane->funcs->atomic_get_property(plane, state, property, val);
> > > } else {
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index c356545..55b44e3 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
> > > state->fence = NULL;
> > > state->commit = NULL;
> > > + state->damage_clips = NULL;
> > > + state->num_clips = 0;
> > > }
> > > EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
> > > @@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
> > > if (state->commit)
> > > drm_crtc_commit_put(state->commit);
> > > +
> > > + drm_property_blob_put(state->damage_clips);
> > > }
> > > EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
> > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > > index e5c6533..e93b127 100644
> > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > @@ -293,6 +293,11 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
> > > return -ENOMEM;
> > > dev->mode_config.prop_crtc_id = prop;
> > > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, "DAMAGE_CLIPS", 0);
> > Bit a bikeshed, but since the coordinates are in fb pixels, not plane
> > pixels, I'd call this "FB_DAMAGE_CLIPS".
> >
> > > + if (!prop)
> > > + return -ENOMEM;
> > > + dev->mode_config.prop_damage_clips = prop;
> > > +
> > > prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
> > > "ACTIVE");
> > > if (!prop)
> > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > index 6d2a6e4..071221b 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > > return ret;
> > > }
> > > +
> > > +/**
> > > + * drm_plane_enable_damage_clips - enable damage clips property
> > > + * @plane: plane on which this property to enable.
> > > + */
> > > +void drm_plane_enable_damage_clips(struct drm_plane *plane)
> > > +{
> > > + struct drm_device *dev = plane->dev;
> > > + struct drm_mode_config *config = &dev->mode_config;
> > > +
> > > + drm_object_attach_property(&plane->base, config->prop_damage_clips, 0);
> > > +}
> > > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > > index 7569f22..d8767da 100644
> > > --- a/include/drm/drm_mode_config.h
> > > +++ b/include/drm/drm_mode_config.h
> > > @@ -628,6 +628,21 @@ struct drm_mode_config {
> > > */
> > > struct drm_property *prop_crtc_id;
> > > /**
> > > + * @prop_damage_clips: Optional plane property to mark damaged regions
> > > + * on the plane in framebuffer coordinates of the framebuffer attached
> > > + * to the plane.
> > Why should we make this optional? Looks like just another thing drivers
> > might screw up, since we have multiple callbacks and things to set up for
> > proper dirty tracking.
> >
> > One option I'm seeing is that if this is set, and it's an atomic driver,
> > then we just directly call into the default atomic fb->dirty
> > implementation. That way there's only 1 thing drivers need to do to set up
> > dirty rect tracking, and they'll get all of it.
> >
> > > + *
> > > + * The layout of blob data is simply an array of drm_mode_rect with
> > > + * maximum array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike
> > > + * plane src coordinates, damage clips are not in 16.16 fixed point.
> > I honestly have no idea where this limit is from. Worth keeping? I can
> > easily imagine that userspace could trip over this - it's fairly high, but
> > not unlimited.
> >
> > > + *
> > > + * Damage clips are a hint to kernel as which area of framebuffer has
> > > + * changed since last page-flip. This should be helpful
> > > + * for some drivers especially for virtual devices where each
> > > + * framebuffer change needs to be transmitted over network, usb, etc.
> > I'd also clarify that userspace still must render the entire screen, i.e.
> > make it more clear that it's really just a hint and not mandatory to only
> > scan out the damaged parts.
> >
> > > + */
> > > + struct drm_property *prop_damage_clips;
> > > + /**
> > > * @prop_active: Default atomic CRTC property to control the active
> > > * state, which is the simplified implementation for DPMS in atomic
> > > * drivers.
> > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > > index f7bf4a4..9f24548 100644
> > > --- a/include/drm/drm_plane.h
> > > +++ b/include/drm/drm_plane.h
> > > @@ -146,6 +146,21 @@ struct drm_plane_state {
> > > */
> > > struct drm_crtc_commit *commit;
> > > + /*
> > > + * @damage_clips
> > > + *
> > > + * blob property with damage as array of drm_rect in framebuffer
> > &drm_rect gives you a nice hyperlink in the generated docs.
> >
> > > + * coodinates.
> > > + */
> > > + struct drm_property_blob *damage_clips;
> > > +
> > > + /*
> > > + * @num_clips
> > > + *
> > > + * Number of drm_rect in @damage_clips.
> > > + */
> > > + uint32_t num_clips;
> > > +
> > > struct drm_atomic_state *state;
> > > };
> > > @@ -611,6 +626,7 @@ int drm_plane_init(struct drm_device *dev,
> > > const uint32_t *formats, unsigned int format_count,
> > > bool is_primary);
> > > void drm_plane_cleanup(struct drm_plane *plane);
> > > +void drm_plane_enable_damage_clips(struct drm_plane *plane);
> > > /**
> > > * drm_plane_index - find the index of a registered plane
> > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > > index 50bcf42..0ad0d5b 100644
> > > --- a/include/uapi/drm/drm_mode.h
> > > +++ b/include/uapi/drm/drm_mode.h
> > > @@ -873,6 +873,21 @@ struct drm_mode_revoke_lease {
> > > __u32 lessee_id;
> > > };
> > > +/**
> > > + * struct drm_mode_rect - two dimensional rectangle drm_rect exported to
> > > + * user-space.
> > > + * @x1: horizontal starting coordinate (inclusive)
> > > + * @y1: vertical starting coordinate (inclusive)
> > > + * @x2: horizontal ending coordinate (exclusive)
> > > + * @y2: vertical ending coordinate (exclusive)
> > > + */
> > > +struct drm_mode_rect {
> > > + __s32 x1;
> > > + __s32 y1;
> > > + __s32 x2;
> > > + __s32 y2;
> > Why signed? Negative damage rects on an fb don't make sense to me. Also,
> > please specify what this is exactly (to avoid confusion with the 16.16
> > fixed point src rects), and maybe mention in the commit message why we're
> > not using drm_clip_rect (going to proper uapi types and 32bit makes sense
> > to me).
>
> IMO, while we don't expect negative damage coordinates,
> to avoid yet another drm uapi rect in the future when we actually need
> negative numbers signed is a good choice...
Makes sense. Another thing I realized: Since src rect are 16.16 fixed
point, do we really need s32? drm_clip_rect is a bit meh, but it gives us
s16 already. That would avoid having to sprinkle the code with tons of
overflow checks for input validation.
On the topic of input validation: Should the kernel check in shared code
that all the clip rects are sensible, i.e. within the dimensions of the
fb? Relying on drivers for that seems like a bad idea.
That could be done in core code in drm_atomic_plane_check().
-Daniel
>
> /Thomas
>
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Thu, Apr 05, 2018 at 10:49:09AM +0200, Thomas Hellstrom wrote:
> On 04/05/2018 09:52 AM, Daniel Vetter wrote:
> > On Wed, Apr 04, 2018 at 04:49:07PM -0700, Deepak Rawat wrote:
> > > With damage property in drm_plane_state, this patch adds helper iterator
> > > to traverse the damage clips. Iterator will return the damage rectangles
> > > in framebuffer, plane or crtc coordinates as need by driver
> > > implementation.
> > >
> > > Signed-off-by: Deepak Rawat <[email protected]>
> > I'd really like selftest/unittests for this stuff. There's an awful lot of
> > cornercases in this here (for any of the transformed iterators at least),
> > and unit tests is the best way to make sure we handle them all correctly.
> >
> > Bonus points if you integrate the new selftests into igt so intel CI can
> > run them, seel igt/tests/drm_mm.c for an example. drm_mm selftest is also
> > the framework I'd copy for this stuff.
> >
> > > ---
> > > drivers/gpu/drm/drm_atomic_helper.c | 122 ++++++++++++++++++++++++++++++++++++
> > > include/drm/drm_atomic_helper.h | 39 ++++++++++++
> > > 2 files changed, 161 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 55b44e3..355b514 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -3865,3 +3865,125 @@ void __drm_atomic_helper_private_obj_duplicate_state(struct drm_private_obj *obj
> > > memcpy(state, obj->state, sizeof(*state));
> > > }
> > > EXPORT_SYMBOL(__drm_atomic_helper_private_obj_duplicate_state);
> > > +
> > > +/**
> > > + * drm_atomic_helper_damage_iter_init - initialize the damage iterator
> > > + * @iter: The iterator to initialize.
> > > + * @type: Coordinate type caller is interested in.
> > > + * @state: plane_state from which to iterate the damage clips.
> > > + * @hdisplay: Width of crtc on which plane is scanned out.
> > > + * @vdisplay: Height of crtc on which plane is scanned out.
> > > + *
> > > + * Initialize an iterator that is used to translate and clip a set of damage
> > > + * rectangles in framebuffer coordinates to plane and crtc coordinates. The type
> > > + * argument specify which type of coordinate to iterate in.
> > > + *
> > > + * Returns: 0 on success and negative error code on error. If an error code is
> > > + * returned then it means the plane state should not update.
> > > + */
> > > +int
> > > +drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
> > > + enum drm_atomic_helper_damage_clip_type type,
> > > + const struct drm_plane_state *state,
> > > + uint32_t hdisplay, uint32_t vdisplay)
> > > +{
> > > + if (!state || !state->crtc || !state->fb)
> > > + return -EINVAL;
> > > +
> > > + memset(iter, 0, sizeof(*iter));
> > > + iter->clips = (struct drm_rect *)state->damage_clips->data;
> > > + iter->num_clips = state->num_clips;
> > > + iter->type = type;
> > > +
> > > + /*
> > > + * Full update in case of scaling or rotation. In future support for
> > > + * scaling/rotating damage clips can be added
> > > + */
> > > + if (state->crtc_w != (state->src_w >> 16) ||
> > > + state->crtc_h != state->src_h >> 16 || state->rotation != 0) {
> > > + iter->curr_clip = iter->num_clips;
> > > + return 0;
> > Given there's no user of this I have no idea how this manages to provoke a
> > full clip rect. selftest code would be perfect for stuff like this.
> >
> > Also, I think we should provide a full clip for the case of num_clips ==
> > 0, so that driver code can simply iterate over all clips and doesn't ever
> > have to handle the "no clip rects provided" case as a special case itself.
> >
> > > + }
> > > +
> > > + iter->fb_src.x1 = 0;
> > > + iter->fb_src.y1 = 0;
> > > + iter->fb_src.x2 = state->fb->width;
> > > + iter->fb_src.y2 = state->fb->height;
> > > +
> > > + iter->plane_src.x1 = state->src_x >> 16;
> > > + iter->plane_src.y1 = state->src_y >> 16;
> > > + iter->plane_src.x2 = iter->plane_src.x1 + (state->src_w >> 16);
> > > + iter->plane_src.y2 = iter->plane_src.y1 + (state->src_h >> 16);
> > > + iter->translate_plane_x = -iter->plane_src.x1;
> > > + iter->translate_plane_y = -iter->plane_src.y1;
> > > +
> > > + /* Clip plane src rect to fb dimensions */
> > > + drm_rect_intersect(&iter->plane_src, &iter->fb_src);
> > This smells like driver bug. Also, see Ville's recent efforts to improve
> > the atomic plane clipping, I think drm_plane_state already has all the
> > clip rects you want.
> >
> > > +
> > > + iter->crtc_src.x1 = 0;
> > > + iter->crtc_src.y1 = 0;
> > > + iter->crtc_src.x2 = hdisplay;
> > > + iter->crtc_src.y2 = vdisplay;
> > > + iter->translate_crtc_x = -(iter->plane_src.x1 - state->crtc_x);
> > > + iter->translate_crtc_x = -(iter->plane_src.y1 - state->crtc_y);
> > > +
> > > + /* Clip crtc src rect to plane dimensions */
> > > + drm_rect_translate(&iter->crtc_src, -iter->translate_crtc_x,
> > > + -iter->translate_crtc_x);
> > We can also scale.
>
> I suggest we leave out scaling for now until someone actually needs it.
In that case we need to WARN_ON and bail out if there's scaling going on.
I'm totally fine with not solving the world here, but please no trapdoors
for following driver's use-cases.
>
> >
> > > + drm_rect_intersect(&iter->crtc_src, &iter->plane_src);
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init);
> > > +
> > > +/**
> > > + * drm_atomic_helper_damage_iter_next - advance the damage iterator
> > > + * @iter: The iterator to advance.
> > > + * @rect: Return a rectangle in coordinate specified during iterator init.
> > > + *
> > > + * Returns: true if the output is valid, false if we've reached the end of the
> > > + * rectangle list. If the first call return false, means need full update.
> > > + */
> > > +bool
> > > +drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,
> > > + struct drm_rect *rect)
> > > +{
> > > + const struct drm_rect *curr_clip;
> > > +
> > > +next_clip:
> > > + if (iter->curr_clip >= iter->num_clips)
> > > + return false;
> > > +
> > > + curr_clip = &iter->clips[iter->curr_clip];
> > > + iter->curr_clip++;
> > > +
> > > + rect->x1 = curr_clip->x1;
> > > + rect->x2 = curr_clip->x2;
> > > + rect->y1 = curr_clip->y1;
> > > + rect->y2 = curr_clip->y2;
> > > +
> > > + /* Clip damage rect within fb limit */
> > > + if (!drm_rect_intersect(rect, &iter->fb_src))
> > > + goto next_clip;
> > > + else if (iter->type & DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_FB)
> > > + return true;
> > > +
> > > + /* Clip damage rect within plane limit */
> > > + if (!drm_rect_intersect(rect, &iter->plane_src))
> > > + goto next_clip;
> > > + else if (iter->type & DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_PLANE) {
> > > + drm_rect_translate(rect, iter->translate_plane_x,
> > > + iter->translate_plane_y);
> > > + return true;
> > > + }
> > > +
> > > + /* Clip damage rect within crtc limit */
> > > + if (!drm_rect_intersect(rect, &iter->crtc_src))
> > > + goto next_clip;
> > > +
> > > + drm_rect_translate(rect, iter->translate_crtc_x,
> > > + iter->translate_crtc_y);
> > > +
> > > + return true;
> > > +}
> > > +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
> > > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> > > index 26aaba5..ebd4b66 100644
> > > --- a/include/drm/drm_atomic_helper.h
> > > +++ b/include/drm/drm_atomic_helper.h
> > > @@ -36,6 +36,37 @@ struct drm_atomic_state;
> > > struct drm_private_obj;
> > > struct drm_private_state;
> > > +/**
> > > + * enum drm_atomic_helper_damage_clip_type - type of clips to iterator over
> > > + *
> > > + * While using drm_atomic_helper_damage_iter the type of clip coordinates caller
> > > + * is interested in.
> > > + */
> > > +enum drm_atomic_helper_damage_clip_type {
> > > + DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_FB = 0x0,
> > > + DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_PLANE = 0x1,
> > > + DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_CRTC = 0x2,
> > I'm confused with what exactly these different types of iterators are
> > supposed to achieve. TYPE_FB makes sense, that's what vmwgfx and other
> > virtual drivers need to figure out which parts of a buffer to upload to
> > the host.
> >
> > TYPE_PLANE I have no idea who needs that. I suggest we just drop it.
> >
> > TYPE_CRTC is what I'd want to use for manual upload hw, were instead of
> > compositing the entire screen we can limit the uploaded area to 1 or 2
> > rectangles (depending upon how the hw works). But those drivers want all
> > the crtc clip rects for _all_ the planes combined, not for each plane
> > individually.
> >
> > My suggestion is to drop TYPE_CRTC until someone needs it for a driver.
> > And most likely the only iterator we want for TYPE_CRTC is one that gives
> > you the overall damage area, including alpha/ctm/gamma/everything else,
> > coalesced into just 1 clip rect. So probably an entirely different
> > function.
>
> Actually for vmwgfx, the display part of the hardware comes in different
> versions and we'd typically expect both the FB coordinates and the CRTC
> coordinates. In the latest version the surface backing the screen needs to
> exactly fit the CRTC scanout area, and if the FB doesn't do that, we need to
> create a surface that does and copy in the kernel driver. After that, as a
> second step, we need to notify the display system of the damaged area in
> CRTC coordinates.
Hm, that's an unexpected use-case. I'm leaning towards just having a fb
coordination iterator and adjusting the static offset in vmwgfx code. Your
use-case is rather different from manual upload screens I think, so having
a CRTC damage iterator which doesn't actually do what most other drivers
want will be confusing.
And since it's just a static drm_rect_translate for you it'll be only 1
additional line in vmwgfx. Imo that's worth the safer/cleaner semantics
for the core helpers.
Manual upload hw driver use case here means:
- Multiple planes, positioned all over the screen.
- Definitely scaling involved.
- Alpha, gamma, blending all matter (since it's only the final composited
pixel value we decide to upload or not upload), and the hw with psr/dsi
panels tend to all have rather fancy hw composition engines.
- Just 1 or 2 rectangles in CRTC coordinates.
Your CRTC coordination use case otoh is simply fallout from your
requirement that you can't position the framebuffer anywhere else than at
0,0, with size exactly matching the screen, plus the reasonable decision
to handle that impendence mismatch in your driver (since userspace will be
unhappy about this, at least generic one).
I think even as far as virtual hardware guess, that's fairly special, and
probably not the case we should optimize helpers for.
Thoughts?
Thanks, Daniel
> > Summarizing all this, I'd simplify the iterator to:
> >
> > drm_atomic_helper_damage_iter_init(iter, plane_state);
> >
> > And leave the other 2 use-cases to the next patch series. For crtc damage
> > we probably want:
> >
> > drm_atomic_helper_crtc_damage(drm_atomic_state, rect)
> >
> > Which internally loops over all planes and also takes all the other state
> > changes into account. That way you also don't have to fix the scaling
> > issue, since your current code only handles translation.
> >
> > Another bit: drm_atomic_helper.c is huge, I'd vote to put all this stuff
> > into a new drm_damage_helper.[hc], including new section in drm-kms.rst
> > and all that. Splitting up drm_atomic_helper.[hc] is somewhere on my todo
> > ...
> >
> > Cheers, Daniel
>
> /Thomas
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On 04/05/2018 12:03 PM, Daniel Vetter wrote:
> On Thu, Apr 05, 2018 at 11:00:15AM +0200, Thomas Hellstrom wrote:
>> On 04/05/2018 09:35 AM, Daniel Vetter wrote:
>>> On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
>>>> From: Lukasz Spintzyk <[email protected]>
>>>>
>>>> Optional plane property to mark damaged regions on the plane in
>>>> framebuffer coordinates of the framebuffer attached to the plane.
>>>>
>>>> The layout of blob data is simply an array of drm_mode_rect with maximum
>>>> array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
>>>> coordinates, damage clips are not in 16.16 fixed point.
>>>>
>>>> Damage clips are a hint to kernel as which area of framebuffer has
>>>> changed since last page-flip. This should be helpful for some drivers
>>>> especially for virtual devices where each framebuffer change needs to
>>>> be transmitted over network, usb, etc.
>>>>
>>>> Driver which are interested in enabling DAMAGE_CLIPS property for a
>>>> plane should enable this property using drm_plane_enable_damage_clips.
>>>>
>>>> Signed-off-by: Lukasz Spintzyk <[email protected]>
>>>> Signed-off-by: Deepak Rawat <[email protected]>
>>> The property uapi section is missing, see:
>>>
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-2Dcomposition-2Dproperties&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ&m=J31YNV1uz3IVRaorM-eo48msi9__sQurqRZrig2UE1s&s=vzsuquF1agbOw54HSC_18Kk2k9j83m8RusmDCtPAFWQ&e=
>>>
>>> Plane composition feels like the best place to put this. Please use that
>>> section to tie all the various bits together, including the helpers you're
>>> adding in the following patches for drivers to use.
>>>
>>> Bunch of nitpicks below, but overall I'm agreeing now with just going with
>>> fb coordinate damage rects.
>>>
>>> Like you say, the thing needed here now is userspace + driver actually
>>> implementing this. I'd also say the compat helper to map the legacy
>>> fb->dirty to this new atomic way of doing things should be included here
>>> (gives us a lot more testing for these new paths).
>>>
>>> Icing on the cake would be an igt to make sure kernel rejects invalid clip
>>> rects correctly.
>>>
>>>> ---
>>>> drivers/gpu/drm/drm_atomic.c | 42 +++++++++++++++++++++++++++++++++++++
>>>> drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
>>>> drivers/gpu/drm/drm_mode_config.c | 5 +++++
>>>> drivers/gpu/drm/drm_plane.c | 12 +++++++++++
>>>> include/drm/drm_mode_config.h | 15 +++++++++++++
>>>> include/drm/drm_plane.h | 16 ++++++++++++++
>>>> include/uapi/drm/drm_mode.h | 15 +++++++++++++
>>>> 7 files changed, 109 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>>> index 7d25c42..9226d24 100644
>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>> @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p,
>>>> }
>>>> /**
>>>> + * drm_atomic_set_damage_for_plane - sets the damage clips property to plane
>>>> + * @state: plane state
>>>> + * @blob: damage clips in framebuffer coordinates
>>>> + *
>>>> + * Returns:
>>>> + *
>>>> + * Zero on success, error code on failure.
>>>> + */
>>>> +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
>>>> + struct drm_property_blob *blob)
>>>> +{
>>>> + if (blob == state->damage_clips)
>>>> + return 0;
>>>> +
>>>> + drm_property_blob_put(state->damage_clips);
>>>> + state->damage_clips = NULL;
>>>> +
>>>> + if (blob) {
>>>> + uint32_t count = blob->length/sizeof(struct drm_rect);
>>>> +
>>>> + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
>>>> + return -EINVAL;
>>>> +
>>>> + state->damage_clips = drm_property_blob_get(blob);
>>>> + state->num_clips = count;
>>>> + } else {
>>>> + state->damage_clips = NULL;
>>>> + state->num_clips = 0;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> * drm_atomic_get_plane_state - get plane state
>>>> * @state: global atomic state object
>>>> * @plane: plane to get state object for
>>>> @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>>>> state->color_encoding = val;
>>>> } else if (property == plane->color_range_property) {
>>>> state->color_range = val;
>>>> + } else if (property == config->prop_damage_clips) {
>>>> + struct drm_property_blob *blob =
>>>> + drm_property_lookup_blob(dev, val);
>>>> + int ret = drm_atomic_set_damage_for_plane(state, blob);
>>> There's already a helper with size-checking built-in, see
>>> drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd
>>> just provide a little inline helper that does the
>>> blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).
>>>
>>>> + drm_property_blob_put(blob);
>>>> + return ret;
>>>> } else if (plane->funcs->atomic_set_property) {
>>>> return plane->funcs->atomic_set_property(plane, state,
>>>> property, val);
>>>> @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>>>> *val = state->color_encoding;
>>>> } else if (property == plane->color_range_property) {
>>>> *val = state->color_range;
>>>> + } else if (property == config->prop_damage_clips) {
>>>> + *val = (state->damage_clips) ? state->damage_clips->base.id : 0;
>>>> } else if (plane->funcs->atomic_get_property) {
>>>> return plane->funcs->atomic_get_property(plane, state, property, val);
>>>> } else {
>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>> index c356545..55b44e3 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>> @@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>>>> state->fence = NULL;
>>>> state->commit = NULL;
>>>> + state->damage_clips = NULL;
>>>> + state->num_clips = 0;
>>>> }
>>>> EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
>>>> @@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>>>> if (state->commit)
>>>> drm_crtc_commit_put(state->commit);
>>>> +
>>>> + drm_property_blob_put(state->damage_clips);
>>>> }
>>>> EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
>>>> index e5c6533..e93b127 100644
>>>> --- a/drivers/gpu/drm/drm_mode_config.c
>>>> +++ b/drivers/gpu/drm/drm_mode_config.c
>>>> @@ -293,6 +293,11 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>>>> return -ENOMEM;
>>>> dev->mode_config.prop_crtc_id = prop;
>>>> + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, "DAMAGE_CLIPS", 0);
>>> Bit a bikeshed, but since the coordinates are in fb pixels, not plane
>>> pixels, I'd call this "FB_DAMAGE_CLIPS".
>>>
>>>> + if (!prop)
>>>> + return -ENOMEM;
>>>> + dev->mode_config.prop_damage_clips = prop;
>>>> +
>>>> prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
>>>> "ACTIVE");
>>>> if (!prop)
>>>> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>>>> index 6d2a6e4..071221b 100644
>>>> --- a/drivers/gpu/drm/drm_plane.c
>>>> +++ b/drivers/gpu/drm/drm_plane.c
>>>> @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>>>> return ret;
>>>> }
>>>> +
>>>> +/**
>>>> + * drm_plane_enable_damage_clips - enable damage clips property
>>>> + * @plane: plane on which this property to enable.
>>>> + */
>>>> +void drm_plane_enable_damage_clips(struct drm_plane *plane)
>>>> +{
>>>> + struct drm_device *dev = plane->dev;
>>>> + struct drm_mode_config *config = &dev->mode_config;
>>>> +
>>>> + drm_object_attach_property(&plane->base, config->prop_damage_clips, 0);
>>>> +}
>>>> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
>>>> index 7569f22..d8767da 100644
>>>> --- a/include/drm/drm_mode_config.h
>>>> +++ b/include/drm/drm_mode_config.h
>>>> @@ -628,6 +628,21 @@ struct drm_mode_config {
>>>> */
>>>> struct drm_property *prop_crtc_id;
>>>> /**
>>>> + * @prop_damage_clips: Optional plane property to mark damaged regions
>>>> + * on the plane in framebuffer coordinates of the framebuffer attached
>>>> + * to the plane.
>>> Why should we make this optional? Looks like just another thing drivers
>>> might screw up, since we have multiple callbacks and things to set up for
>>> proper dirty tracking.
>>>
>>> One option I'm seeing is that if this is set, and it's an atomic driver,
>>> then we just directly call into the default atomic fb->dirty
>>> implementation. That way there's only 1 thing drivers need to do to set up
>>> dirty rect tracking, and they'll get all of it.
>>>
>>>> + *
>>>> + * The layout of blob data is simply an array of drm_mode_rect with
>>>> + * maximum array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike
>>>> + * plane src coordinates, damage clips are not in 16.16 fixed point.
>>> I honestly have no idea where this limit is from. Worth keeping? I can
>>> easily imagine that userspace could trip over this - it's fairly high, but
>>> not unlimited.
>>>
>>>> + *
>>>> + * Damage clips are a hint to kernel as which area of framebuffer has
>>>> + * changed since last page-flip. This should be helpful
>>>> + * for some drivers especially for virtual devices where each
>>>> + * framebuffer change needs to be transmitted over network, usb, etc.
>>> I'd also clarify that userspace still must render the entire screen, i.e.
>>> make it more clear that it's really just a hint and not mandatory to only
>>> scan out the damaged parts.
>>>
>>>> + */
>>>> + struct drm_property *prop_damage_clips;
>>>> + /**
>>>> * @prop_active: Default atomic CRTC property to control the active
>>>> * state, which is the simplified implementation for DPMS in atomic
>>>> * drivers.
>>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>>>> index f7bf4a4..9f24548 100644
>>>> --- a/include/drm/drm_plane.h
>>>> +++ b/include/drm/drm_plane.h
>>>> @@ -146,6 +146,21 @@ struct drm_plane_state {
>>>> */
>>>> struct drm_crtc_commit *commit;
>>>> + /*
>>>> + * @damage_clips
>>>> + *
>>>> + * blob property with damage as array of drm_rect in framebuffer
>>> &drm_rect gives you a nice hyperlink in the generated docs.
>>>
>>>> + * coodinates.
>>>> + */
>>>> + struct drm_property_blob *damage_clips;
>>>> +
>>>> + /*
>>>> + * @num_clips
>>>> + *
>>>> + * Number of drm_rect in @damage_clips.
>>>> + */
>>>> + uint32_t num_clips;
>>>> +
>>>> struct drm_atomic_state *state;
>>>> };
>>>> @@ -611,6 +626,7 @@ int drm_plane_init(struct drm_device *dev,
>>>> const uint32_t *formats, unsigned int format_count,
>>>> bool is_primary);
>>>> void drm_plane_cleanup(struct drm_plane *plane);
>>>> +void drm_plane_enable_damage_clips(struct drm_plane *plane);
>>>> /**
>>>> * drm_plane_index - find the index of a registered plane
>>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>>>> index 50bcf42..0ad0d5b 100644
>>>> --- a/include/uapi/drm/drm_mode.h
>>>> +++ b/include/uapi/drm/drm_mode.h
>>>> @@ -873,6 +873,21 @@ struct drm_mode_revoke_lease {
>>>> __u32 lessee_id;
>>>> };
>>>> +/**
>>>> + * struct drm_mode_rect - two dimensional rectangle drm_rect exported to
>>>> + * user-space.
>>>> + * @x1: horizontal starting coordinate (inclusive)
>>>> + * @y1: vertical starting coordinate (inclusive)
>>>> + * @x2: horizontal ending coordinate (exclusive)
>>>> + * @y2: vertical ending coordinate (exclusive)
>>>> + */
>>>> +struct drm_mode_rect {
>>>> + __s32 x1;
>>>> + __s32 y1;
>>>> + __s32 x2;
>>>> + __s32 y2;
>>> Why signed? Negative damage rects on an fb don't make sense to me. Also,
>>> please specify what this is exactly (to avoid confusion with the 16.16
>>> fixed point src rects), and maybe mention in the commit message why we're
>>> not using drm_clip_rect (going to proper uapi types and 32bit makes sense
>>> to me).
>> IMO, while we don't expect negative damage coordinates,
>> to avoid yet another drm uapi rect in the future when we actually need
>> negative numbers signed is a good choice...
> Makes sense. Another thing I realized: Since src rect are 16.16 fixed
> point, do we really need s32? drm_clip_rect is a bit meh, but it gives us
> s16 already. That would avoid having to sprinkle the code with tons of
> overflow checks for input validation.
IMHO, sooner or later we're going to run into the s16 limitation, and I
think we should start
making user-space APIs > 15 bit coordinate safe. I agree this could lead
to some validation
grief in the short run, but less to fix up later.
How difficult is it to make the kernel-internal 16.16 fixed point 48.16?
/Thomas
>
> On the topic of input validation: Should the kernel check in shared code
> that all the clip rects are sensible, i.e. within the dimensions of the
> fb? Relying on drivers for that seems like a bad idea.
>
> That could be done in core code in drm_atomic_plane_check().
> -Daniel
>> /Thomas
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 04/05/2018 12:03 PM, Daniel Vetter wrote:
>
> On the topic of input validation: Should the kernel check in shared code
> that all the clip rects are sensible, i.e. within the dimensions of the
> fb? Relying on drivers for that seems like a bad idea.
I guess we could do that.
There seems to be different needs for different kinds of iterators, but
otherwise
I was thinking that an iterator could just skip invalid rects if found.
Then we
don't need a separate validation step, but OTOH user-space won't get
notified if
that was intended.
I'm not 100% sure user-space would do anything sensible with any error
information, though.
/Thomas
>
> That could be done in core code in drm_atomic_plane_check().
> -Daniel
>> /Thomas
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> [email protected]
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ&m=JV7fhQ4zTiyiKsY8M0Zlf81l7szYePUke8kwSvQv1T8&s=HEcbr-8aoWqRvWGY6RcL1QeAtEyxMRLHbsOtdJFk78I&e=
On 04/05/2018 12:10 PM, Daniel Vetter wrote:
> On Thu, Apr 05, 2018 at 10:49:09AM +0200, Thomas Hellstrom wrote:
>> On 04/05/2018 09:52 AM, Daniel Vetter wrote:
>>> On Wed, Apr 04, 2018 at 04:49:07PM -0700, Deepak Rawat wrote:
>>>> With damage property in drm_plane_state, this patch adds helper iterator
>>>> to traverse the damage clips. Iterator will return the damage rectangles
>>>> in framebuffer, plane or crtc coordinates as need by driver
>>>> implementation.
>>>>
>>>> Signed-off-by: Deepak Rawat <[email protected]>
>>> I'd really like selftest/unittests for this stuff. There's an awful lot of
>>> cornercases in this here (for any of the transformed iterators at least),
>>> and unit tests is the best way to make sure we handle them all correctly.
>>>
>>> Bonus points if you integrate the new selftests into igt so intel CI can
>>> run them, seel igt/tests/drm_mm.c for an example. drm_mm selftest is also
>>> the framework I'd copy for this stuff.
>>>
>>>> ---
>>>> drivers/gpu/drm/drm_atomic_helper.c | 122 ++++++++++++++++++++++++++++++++++++
>>>> include/drm/drm_atomic_helper.h | 39 ++++++++++++
>>>> 2 files changed, 161 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>> index 55b44e3..355b514 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>> @@ -3865,3 +3865,125 @@ void __drm_atomic_helper_private_obj_duplicate_state(struct drm_private_obj *obj
>>>> memcpy(state, obj->state, sizeof(*state));
>>>> }
>>>> EXPORT_SYMBOL(__drm_atomic_helper_private_obj_duplicate_state);
>>>> +
>>>> +/**
>>>> + * drm_atomic_helper_damage_iter_init - initialize the damage iterator
>>>> + * @iter: The iterator to initialize.
>>>> + * @type: Coordinate type caller is interested in.
>>>> + * @state: plane_state from which to iterate the damage clips.
>>>> + * @hdisplay: Width of crtc on which plane is scanned out.
>>>> + * @vdisplay: Height of crtc on which plane is scanned out.
>>>> + *
>>>> + * Initialize an iterator that is used to translate and clip a set of damage
>>>> + * rectangles in framebuffer coordinates to plane and crtc coordinates. The type
>>>> + * argument specify which type of coordinate to iterate in.
>>>> + *
>>>> + * Returns: 0 on success and negative error code on error. If an error code is
>>>> + * returned then it means the plane state should not update.
>>>> + */
>>>> +int
>>>> +drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
>>>> + enum drm_atomic_helper_damage_clip_type type,
>>>> + const struct drm_plane_state *state,
>>>> + uint32_t hdisplay, uint32_t vdisplay)
>>>> +{
>>>> + if (!state || !state->crtc || !state->fb)
>>>> + return -EINVAL;
>>>> +
>>>> + memset(iter, 0, sizeof(*iter));
>>>> + iter->clips = (struct drm_rect *)state->damage_clips->data;
>>>> + iter->num_clips = state->num_clips;
>>>> + iter->type = type;
>>>> +
>>>> + /*
>>>> + * Full update in case of scaling or rotation. In future support for
>>>> + * scaling/rotating damage clips can be added
>>>> + */
>>>> + if (state->crtc_w != (state->src_w >> 16) ||
>>>> + state->crtc_h != state->src_h >> 16 || state->rotation != 0) {
>>>> + iter->curr_clip = iter->num_clips;
>>>> + return 0;
>>> Given there's no user of this I have no idea how this manages to provoke a
>>> full clip rect. selftest code would be perfect for stuff like this.
>>>
>>> Also, I think we should provide a full clip for the case of num_clips ==
>>> 0, so that driver code can simply iterate over all clips and doesn't ever
>>> have to handle the "no clip rects provided" case as a special case itself.
>>>
>>>> + }
>>>> +
>>>> + iter->fb_src.x1 = 0;
>>>> + iter->fb_src.y1 = 0;
>>>> + iter->fb_src.x2 = state->fb->width;
>>>> + iter->fb_src.y2 = state->fb->height;
>>>> +
>>>> + iter->plane_src.x1 = state->src_x >> 16;
>>>> + iter->plane_src.y1 = state->src_y >> 16;
>>>> + iter->plane_src.x2 = iter->plane_src.x1 + (state->src_w >> 16);
>>>> + iter->plane_src.y2 = iter->plane_src.y1 + (state->src_h >> 16);
>>>> + iter->translate_plane_x = -iter->plane_src.x1;
>>>> + iter->translate_plane_y = -iter->plane_src.y1;
>>>> +
>>>> + /* Clip plane src rect to fb dimensions */
>>>> + drm_rect_intersect(&iter->plane_src, &iter->fb_src);
>>> This smells like driver bug. Also, see Ville's recent efforts to improve
>>> the atomic plane clipping, I think drm_plane_state already has all the
>>> clip rects you want.
>>>
>>>> +
>>>> + iter->crtc_src.x1 = 0;
>>>> + iter->crtc_src.y1 = 0;
>>>> + iter->crtc_src.x2 = hdisplay;
>>>> + iter->crtc_src.y2 = vdisplay;
>>>> + iter->translate_crtc_x = -(iter->plane_src.x1 - state->crtc_x);
>>>> + iter->translate_crtc_x = -(iter->plane_src.y1 - state->crtc_y);
>>>> +
>>>> + /* Clip crtc src rect to plane dimensions */
>>>> + drm_rect_translate(&iter->crtc_src, -iter->translate_crtc_x,
>>>> + -iter->translate_crtc_x);
>>> We can also scale.
>> I suggest we leave out scaling for now until someone actually needs it.
> In that case we need to WARN_ON and bail out if there's scaling going on.
> I'm totally fine with not solving the world here, but please no trapdoors
> for following driver's use-cases.
>
Agreed.
>>>> + drm_rect_intersect(&iter->crtc_src, &iter->plane_src);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init);
>>>> +
>>>> +/**
>>>> + * drm_atomic_helper_damage_iter_next - advance the damage iterator
>>>> + * @iter: The iterator to advance.
>>>> + * @rect: Return a rectangle in coordinate specified during iterator init.
>>>> + *
>>>> + * Returns: true if the output is valid, false if we've reached the end of the
>>>> + * rectangle list. If the first call return false, means need full update.
>>>> + */
>>>> +bool
>>>> +drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,
>>>> + struct drm_rect *rect)
>>>> +{
>>>> + const struct drm_rect *curr_clip;
>>>> +
>>>> +next_clip:
>>>> + if (iter->curr_clip >= iter->num_clips)
>>>> + return false;
>>>> +
>>>> + curr_clip = &iter->clips[iter->curr_clip];
>>>> + iter->curr_clip++;
>>>> +
>>>> + rect->x1 = curr_clip->x1;
>>>> + rect->x2 = curr_clip->x2;
>>>> + rect->y1 = curr_clip->y1;
>>>> + rect->y2 = curr_clip->y2;
>>>> +
>>>> + /* Clip damage rect within fb limit */
>>>> + if (!drm_rect_intersect(rect, &iter->fb_src))
>>>> + goto next_clip;
>>>> + else if (iter->type & DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_FB)
>>>> + return true;
>>>> +
>>>> + /* Clip damage rect within plane limit */
>>>> + if (!drm_rect_intersect(rect, &iter->plane_src))
>>>> + goto next_clip;
>>>> + else if (iter->type & DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_PLANE) {
>>>> + drm_rect_translate(rect, iter->translate_plane_x,
>>>> + iter->translate_plane_y);
>>>> + return true;
>>>> + }
>>>> +
>>>> + /* Clip damage rect within crtc limit */
>>>> + if (!drm_rect_intersect(rect, &iter->crtc_src))
>>>> + goto next_clip;
>>>> +
>>>> + drm_rect_translate(rect, iter->translate_crtc_x,
>>>> + iter->translate_crtc_y);
>>>> +
>>>> + return true;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
>>>> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
>>>> index 26aaba5..ebd4b66 100644
>>>> --- a/include/drm/drm_atomic_helper.h
>>>> +++ b/include/drm/drm_atomic_helper.h
>>>> @@ -36,6 +36,37 @@ struct drm_atomic_state;
>>>> struct drm_private_obj;
>>>> struct drm_private_state;
>>>> +/**
>>>> + * enum drm_atomic_helper_damage_clip_type - type of clips to iterator over
>>>> + *
>>>> + * While using drm_atomic_helper_damage_iter the type of clip coordinates caller
>>>> + * is interested in.
>>>> + */
>>>> +enum drm_atomic_helper_damage_clip_type {
>>>> + DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_FB = 0x0,
>>>> + DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_PLANE = 0x1,
>>>> + DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_CRTC = 0x2,
>>> I'm confused with what exactly these different types of iterators are
>>> supposed to achieve. TYPE_FB makes sense, that's what vmwgfx and other
>>> virtual drivers need to figure out which parts of a buffer to upload to
>>> the host.
>>>
>>> TYPE_PLANE I have no idea who needs that. I suggest we just drop it.
>>>
>>> TYPE_CRTC is what I'd want to use for manual upload hw, were instead of
>>> compositing the entire screen we can limit the uploaded area to 1 or 2
>>> rectangles (depending upon how the hw works). But those drivers want all
>>> the crtc clip rects for _all_ the planes combined, not for each plane
>>> individually.
>>>
>>> My suggestion is to drop TYPE_CRTC until someone needs it for a driver.
>>> And most likely the only iterator we want for TYPE_CRTC is one that gives
>>> you the overall damage area, including alpha/ctm/gamma/everything else,
>>> coalesced into just 1 clip rect. So probably an entirely different
>>> function.
>> Actually for vmwgfx, the display part of the hardware comes in different
>> versions and we'd typically expect both the FB coordinates and the CRTC
>> coordinates. In the latest version the surface backing the screen needs to
>> exactly fit the CRTC scanout area, and if the FB doesn't do that, we need to
>> create a surface that does and copy in the kernel driver. After that, as a
>> second step, we need to notify the display system of the damaged area in
>> CRTC coordinates.
> Hm, that's an unexpected use-case. I'm leaning towards just having a fb
> coordination iterator and adjusting the static offset in vmwgfx code. Your
> use-case is rather different from manual upload screens I think, so having
> a CRTC damage iterator which doesn't actually do what most other drivers
> want will be confusing.
>
> And since it's just a static drm_rect_translate for you it'll be only 1
> additional line in vmwgfx. Imo that's worth the safer/cleaner semantics
> for the core helpers.
Sounds reasonable. We could have an fb coord iterator only, and do the
translation to crtc coordinates outside. But I still think we should do
the plane clipping in
the iterator, if needed with a big WARN_ON for transformed planes.
/Thomas
On Thu, Apr 05, 2018 at 01:35:25PM +0200, Thomas Hellstrom wrote:
> On 04/05/2018 12:03 PM, Daniel Vetter wrote:
> > On Thu, Apr 05, 2018 at 11:00:15AM +0200, Thomas Hellstrom wrote:
> > > On 04/05/2018 09:35 AM, Daniel Vetter wrote:
> > > > On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
> > > > > From: Lukasz Spintzyk <[email protected]>
> > > > >
> > > > > Optional plane property to mark damaged regions on the plane in
> > > > > framebuffer coordinates of the framebuffer attached to the plane.
> > > > >
> > > > > The layout of blob data is simply an array of drm_mode_rect with maximum
> > > > > array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
> > > > > coordinates, damage clips are not in 16.16 fixed point.
> > > > >
> > > > > Damage clips are a hint to kernel as which area of framebuffer has
> > > > > changed since last page-flip. This should be helpful for some drivers
> > > > > especially for virtual devices where each framebuffer change needs to
> > > > > be transmitted over network, usb, etc.
> > > > >
> > > > > Driver which are interested in enabling DAMAGE_CLIPS property for a
> > > > > plane should enable this property using drm_plane_enable_damage_clips.
> > > > >
> > > > > Signed-off-by: Lukasz Spintzyk <[email protected]>
> > > > > Signed-off-by: Deepak Rawat <[email protected]>
> > > > The property uapi section is missing, see:
> > > >
> > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-2Dcomposition-2Dproperties&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ&m=J31YNV1uz3IVRaorM-eo48msi9__sQurqRZrig2UE1s&s=vzsuquF1agbOw54HSC_18Kk2k9j83m8RusmDCtPAFWQ&e=
> > > >
> > > > Plane composition feels like the best place to put this. Please use that
> > > > section to tie all the various bits together, including the helpers you're
> > > > adding in the following patches for drivers to use.
> > > >
> > > > Bunch of nitpicks below, but overall I'm agreeing now with just going with
> > > > fb coordinate damage rects.
> > > >
> > > > Like you say, the thing needed here now is userspace + driver actually
> > > > implementing this. I'd also say the compat helper to map the legacy
> > > > fb->dirty to this new atomic way of doing things should be included here
> > > > (gives us a lot more testing for these new paths).
> > > >
> > > > Icing on the cake would be an igt to make sure kernel rejects invalid clip
> > > > rects correctly.
> > > >
> > > > > ---
> > > > > drivers/gpu/drm/drm_atomic.c | 42 +++++++++++++++++++++++++++++++++++++
> > > > > drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
> > > > > drivers/gpu/drm/drm_mode_config.c | 5 +++++
> > > > > drivers/gpu/drm/drm_plane.c | 12 +++++++++++
> > > > > include/drm/drm_mode_config.h | 15 +++++++++++++
> > > > > include/drm/drm_plane.h | 16 ++++++++++++++
> > > > > include/uapi/drm/drm_mode.h | 15 +++++++++++++
> > > > > 7 files changed, 109 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > > > index 7d25c42..9226d24 100644
> > > > > --- a/drivers/gpu/drm/drm_atomic.c
> > > > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > > > @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p,
> > > > > }
> > > > > /**
> > > > > + * drm_atomic_set_damage_for_plane - sets the damage clips property to plane
> > > > > + * @state: plane state
> > > > > + * @blob: damage clips in framebuffer coordinates
> > > > > + *
> > > > > + * Returns:
> > > > > + *
> > > > > + * Zero on success, error code on failure.
> > > > > + */
> > > > > +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
> > > > > + struct drm_property_blob *blob)
> > > > > +{
> > > > > + if (blob == state->damage_clips)
> > > > > + return 0;
> > > > > +
> > > > > + drm_property_blob_put(state->damage_clips);
> > > > > + state->damage_clips = NULL;
> > > > > +
> > > > > + if (blob) {
> > > > > + uint32_t count = blob->length/sizeof(struct drm_rect);
> > > > > +
> > > > > + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + state->damage_clips = drm_property_blob_get(blob);
> > > > > + state->num_clips = count;
> > > > > + } else {
> > > > > + state->damage_clips = NULL;
> > > > > + state->num_clips = 0;
> > > > > + }
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > * drm_atomic_get_plane_state - get plane state
> > > > > * @state: global atomic state object
> > > > > * @plane: plane to get state object for
> > > > > @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
> > > > > state->color_encoding = val;
> > > > > } else if (property == plane->color_range_property) {
> > > > > state->color_range = val;
> > > > > + } else if (property == config->prop_damage_clips) {
> > > > > + struct drm_property_blob *blob =
> > > > > + drm_property_lookup_blob(dev, val);
> > > > > + int ret = drm_atomic_set_damage_for_plane(state, blob);
> > > > There's already a helper with size-checking built-in, see
> > > > drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd
> > > > just provide a little inline helper that does the
> > > > blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).
> > > >
> > > > > + drm_property_blob_put(blob);
> > > > > + return ret;
> > > > > } else if (plane->funcs->atomic_set_property) {
> > > > > return plane->funcs->atomic_set_property(plane, state,
> > > > > property, val);
> > > > > @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> > > > > *val = state->color_encoding;
> > > > > } else if (property == plane->color_range_property) {
> > > > > *val = state->color_range;
> > > > > + } else if (property == config->prop_damage_clips) {
> > > > > + *val = (state->damage_clips) ? state->damage_clips->base.id : 0;
> > > > > } else if (plane->funcs->atomic_get_property) {
> > > > > return plane->funcs->atomic_get_property(plane, state, property, val);
> > > > > } else {
> > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > index c356545..55b44e3 100644
> > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > @@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
> > > > > state->fence = NULL;
> > > > > state->commit = NULL;
> > > > > + state->damage_clips = NULL;
> > > > > + state->num_clips = 0;
> > > > > }
> > > > > EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
> > > > > @@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
> > > > > if (state->commit)
> > > > > drm_crtc_commit_put(state->commit);
> > > > > +
> > > > > + drm_property_blob_put(state->damage_clips);
> > > > > }
> > > > > EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
> > > > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > > > > index e5c6533..e93b127 100644
> > > > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > > > @@ -293,6 +293,11 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
> > > > > return -ENOMEM;
> > > > > dev->mode_config.prop_crtc_id = prop;
> > > > > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, "DAMAGE_CLIPS", 0);
> > > > Bit a bikeshed, but since the coordinates are in fb pixels, not plane
> > > > pixels, I'd call this "FB_DAMAGE_CLIPS".
> > > >
> > > > > + if (!prop)
> > > > > + return -ENOMEM;
> > > > > + dev->mode_config.prop_damage_clips = prop;
> > > > > +
> > > > > prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
> > > > > "ACTIVE");
> > > > > if (!prop)
> > > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > > > index 6d2a6e4..071221b 100644
> > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > > > > return ret;
> > > > > }
> > > > > +
> > > > > +/**
> > > > > + * drm_plane_enable_damage_clips - enable damage clips property
> > > > > + * @plane: plane on which this property to enable.
> > > > > + */
> > > > > +void drm_plane_enable_damage_clips(struct drm_plane *plane)
> > > > > +{
> > > > > + struct drm_device *dev = plane->dev;
> > > > > + struct drm_mode_config *config = &dev->mode_config;
> > > > > +
> > > > > + drm_object_attach_property(&plane->base, config->prop_damage_clips, 0);
> > > > > +}
> > > > > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > > > > index 7569f22..d8767da 100644
> > > > > --- a/include/drm/drm_mode_config.h
> > > > > +++ b/include/drm/drm_mode_config.h
> > > > > @@ -628,6 +628,21 @@ struct drm_mode_config {
> > > > > */
> > > > > struct drm_property *prop_crtc_id;
> > > > > /**
> > > > > + * @prop_damage_clips: Optional plane property to mark damaged regions
> > > > > + * on the plane in framebuffer coordinates of the framebuffer attached
> > > > > + * to the plane.
> > > > Why should we make this optional? Looks like just another thing drivers
> > > > might screw up, since we have multiple callbacks and things to set up for
> > > > proper dirty tracking.
> > > >
> > > > One option I'm seeing is that if this is set, and it's an atomic driver,
> > > > then we just directly call into the default atomic fb->dirty
> > > > implementation. That way there's only 1 thing drivers need to do to set up
> > > > dirty rect tracking, and they'll get all of it.
> > > >
> > > > > + *
> > > > > + * The layout of blob data is simply an array of drm_mode_rect with
> > > > > + * maximum array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike
> > > > > + * plane src coordinates, damage clips are not in 16.16 fixed point.
> > > > I honestly have no idea where this limit is from. Worth keeping? I can
> > > > easily imagine that userspace could trip over this - it's fairly high, but
> > > > not unlimited.
> > > >
> > > > > + *
> > > > > + * Damage clips are a hint to kernel as which area of framebuffer has
> > > > > + * changed since last page-flip. This should be helpful
> > > > > + * for some drivers especially for virtual devices where each
> > > > > + * framebuffer change needs to be transmitted over network, usb, etc.
> > > > I'd also clarify that userspace still must render the entire screen, i.e.
> > > > make it more clear that it's really just a hint and not mandatory to only
> > > > scan out the damaged parts.
> > > >
> > > > > + */
> > > > > + struct drm_property *prop_damage_clips;
> > > > > + /**
> > > > > * @prop_active: Default atomic CRTC property to control the active
> > > > > * state, which is the simplified implementation for DPMS in atomic
> > > > > * drivers.
> > > > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > > > > index f7bf4a4..9f24548 100644
> > > > > --- a/include/drm/drm_plane.h
> > > > > +++ b/include/drm/drm_plane.h
> > > > > @@ -146,6 +146,21 @@ struct drm_plane_state {
> > > > > */
> > > > > struct drm_crtc_commit *commit;
> > > > > + /*
> > > > > + * @damage_clips
> > > > > + *
> > > > > + * blob property with damage as array of drm_rect in framebuffer
> > > > &drm_rect gives you a nice hyperlink in the generated docs.
> > > >
> > > > > + * coodinates.
> > > > > + */
> > > > > + struct drm_property_blob *damage_clips;
> > > > > +
> > > > > + /*
> > > > > + * @num_clips
> > > > > + *
> > > > > + * Number of drm_rect in @damage_clips.
> > > > > + */
> > > > > + uint32_t num_clips;
> > > > > +
> > > > > struct drm_atomic_state *state;
> > > > > };
> > > > > @@ -611,6 +626,7 @@ int drm_plane_init(struct drm_device *dev,
> > > > > const uint32_t *formats, unsigned int format_count,
> > > > > bool is_primary);
> > > > > void drm_plane_cleanup(struct drm_plane *plane);
> > > > > +void drm_plane_enable_damage_clips(struct drm_plane *plane);
> > > > > /**
> > > > > * drm_plane_index - find the index of a registered plane
> > > > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > > > > index 50bcf42..0ad0d5b 100644
> > > > > --- a/include/uapi/drm/drm_mode.h
> > > > > +++ b/include/uapi/drm/drm_mode.h
> > > > > @@ -873,6 +873,21 @@ struct drm_mode_revoke_lease {
> > > > > __u32 lessee_id;
> > > > > };
> > > > > +/**
> > > > > + * struct drm_mode_rect - two dimensional rectangle drm_rect exported to
> > > > > + * user-space.
> > > > > + * @x1: horizontal starting coordinate (inclusive)
> > > > > + * @y1: vertical starting coordinate (inclusive)
> > > > > + * @x2: horizontal ending coordinate (exclusive)
> > > > > + * @y2: vertical ending coordinate (exclusive)
> > > > > + */
> > > > > +struct drm_mode_rect {
> > > > > + __s32 x1;
> > > > > + __s32 y1;
> > > > > + __s32 x2;
> > > > > + __s32 y2;
> > > > Why signed? Negative damage rects on an fb don't make sense to me. Also,
> > > > please specify what this is exactly (to avoid confusion with the 16.16
> > > > fixed point src rects), and maybe mention in the commit message why we're
> > > > not using drm_clip_rect (going to proper uapi types and 32bit makes sense
> > > > to me).
> > > IMO, while we don't expect negative damage coordinates,
> > > to avoid yet another drm uapi rect in the future when we actually need
> > > negative numbers signed is a good choice...
> > Makes sense. Another thing I realized: Since src rect are 16.16 fixed
> > point, do we really need s32? drm_clip_rect is a bit meh, but it gives us
> > s16 already. That would avoid having to sprinkle the code with tons of
> > overflow checks for input validation.
>
> IMHO, sooner or later we're going to run into the s16 limitation, and I
> think we should start
> making user-space APIs > 15 bit coordinate safe. I agree this could lead to
> some validation
> grief in the short run, but less to fix up later.
If all the corner-case validation comes with selftests I'm happy to review
them for completeness. I'm not good enough to figure out whether there's
missing stuff by just looking at the validation code generally.
> How difficult is it to make the kernel-internal 16.16 fixed point 48.16?
Since we range-limit properties it should work out. But that means easy
bitshifting will much more likely overflow, and I kinda don't want to
audit all those places for something we don't yet need. Even 8k is still a
bit off from the limit.
Once we have hw that can scan out 32k buffers we'll probably have to fix
up everything :-/
-Daniel
>
> /Thomas
>
>
> >
> > On the topic of input validation: Should the kernel check in shared code
> > that all the clip rects are sensible, i.e. within the dimensions of the
> > fb? Relying on drivers for that seems like a bad idea.
> >
> > That could be done in core code in drm_atomic_plane_check().
> > -Daniel
> > > /Thomas
> > >
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > [email protected]
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Thu, Apr 05, 2018 at 01:42:11PM +0200, Thomas Hellstrom wrote:
> On 04/05/2018 12:03 PM, Daniel Vetter wrote:
> >
> > On the topic of input validation: Should the kernel check in shared code
> > that all the clip rects are sensible, i.e. within the dimensions of the
> > fb? Relying on drivers for that seems like a bad idea.
>
> I guess we could do that.
>
> There seems to be different needs for different kinds of iterators, but
> otherwise
> I was thinking that an iterator could just skip invalid rects if found. Then
> we
> don't need a separate validation step, but OTOH user-space won't get
> notified if
> that was intended.
>
> I'm not 100% sure user-space would do anything sensible with any error
> information, though.
Fix userspace, and ime letting userspace get away with improper uapi usage
is a path full of regrets. That's why I'm advising that we check any
unused field for 0, and any unused bits also. Plus anything else that
userspace could get wrong. Because if it's not checked, someone will get
it wrong, and then we have to keep it working forever.
-Daniel
>
> /Thomas
>
>
> >
> > That could be done in core code in drm_atomic_plane_check().
> > -Daniel
> > > /Thomas
> > >
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > [email protected]
> > > https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ&m=JV7fhQ4zTiyiKsY8M0Zlf81l7szYePUke8kwSvQv1T8&s=HEcbr-8aoWqRvWGY6RcL1QeAtEyxMRLHbsOtdJFk78I&e=
>
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Thu, Apr 05, 2018 at 01:51:49PM +0200, Thomas Hellstrom wrote:
> On 04/05/2018 12:10 PM, Daniel Vetter wrote:
> > On Thu, Apr 05, 2018 at 10:49:09AM +0200, Thomas Hellstrom wrote:
> > > On 04/05/2018 09:52 AM, Daniel Vetter wrote:
> > > > On Wed, Apr 04, 2018 at 04:49:07PM -0700, Deepak Rawat wrote:
> > > > > With damage property in drm_plane_state, this patch adds helper iterator
> > > > > to traverse the damage clips. Iterator will return the damage rectangles
> > > > > in framebuffer, plane or crtc coordinates as need by driver
> > > > > implementation.
> > > > >
> > > > > Signed-off-by: Deepak Rawat <[email protected]>
> > > > I'd really like selftest/unittests for this stuff. There's an awful lot of
> > > > cornercases in this here (for any of the transformed iterators at least),
> > > > and unit tests is the best way to make sure we handle them all correctly.
> > > >
> > > > Bonus points if you integrate the new selftests into igt so intel CI can
> > > > run them, seel igt/tests/drm_mm.c for an example. drm_mm selftest is also
> > > > the framework I'd copy for this stuff.
> > > >
> > > > > ---
> > > > > drivers/gpu/drm/drm_atomic_helper.c | 122 ++++++++++++++++++++++++++++++++++++
> > > > > include/drm/drm_atomic_helper.h | 39 ++++++++++++
> > > > > 2 files changed, 161 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > index 55b44e3..355b514 100644
> > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > @@ -3865,3 +3865,125 @@ void __drm_atomic_helper_private_obj_duplicate_state(struct drm_private_obj *obj
> > > > > memcpy(state, obj->state, sizeof(*state));
> > > > > }
> > > > > EXPORT_SYMBOL(__drm_atomic_helper_private_obj_duplicate_state);
> > > > > +
> > > > > +/**
> > > > > + * drm_atomic_helper_damage_iter_init - initialize the damage iterator
> > > > > + * @iter: The iterator to initialize.
> > > > > + * @type: Coordinate type caller is interested in.
> > > > > + * @state: plane_state from which to iterate the damage clips.
> > > > > + * @hdisplay: Width of crtc on which plane is scanned out.
> > > > > + * @vdisplay: Height of crtc on which plane is scanned out.
> > > > > + *
> > > > > + * Initialize an iterator that is used to translate and clip a set of damage
> > > > > + * rectangles in framebuffer coordinates to plane and crtc coordinates. The type
> > > > > + * argument specify which type of coordinate to iterate in.
> > > > > + *
> > > > > + * Returns: 0 on success and negative error code on error. If an error code is
> > > > > + * returned then it means the plane state should not update.
> > > > > + */
> > > > > +int
> > > > > +drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
> > > > > + enum drm_atomic_helper_damage_clip_type type,
> > > > > + const struct drm_plane_state *state,
> > > > > + uint32_t hdisplay, uint32_t vdisplay)
> > > > > +{
> > > > > + if (!state || !state->crtc || !state->fb)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + memset(iter, 0, sizeof(*iter));
> > > > > + iter->clips = (struct drm_rect *)state->damage_clips->data;
> > > > > + iter->num_clips = state->num_clips;
> > > > > + iter->type = type;
> > > > > +
> > > > > + /*
> > > > > + * Full update in case of scaling or rotation. In future support for
> > > > > + * scaling/rotating damage clips can be added
> > > > > + */
> > > > > + if (state->crtc_w != (state->src_w >> 16) ||
> > > > > + state->crtc_h != state->src_h >> 16 || state->rotation != 0) {
> > > > > + iter->curr_clip = iter->num_clips;
> > > > > + return 0;
> > > > Given there's no user of this I have no idea how this manages to provoke a
> > > > full clip rect. selftest code would be perfect for stuff like this.
> > > >
> > > > Also, I think we should provide a full clip for the case of num_clips ==
> > > > 0, so that driver code can simply iterate over all clips and doesn't ever
> > > > have to handle the "no clip rects provided" case as a special case itself.
> > > >
> > > > > + }
> > > > > +
> > > > > + iter->fb_src.x1 = 0;
> > > > > + iter->fb_src.y1 = 0;
> > > > > + iter->fb_src.x2 = state->fb->width;
> > > > > + iter->fb_src.y2 = state->fb->height;
> > > > > +
> > > > > + iter->plane_src.x1 = state->src_x >> 16;
> > > > > + iter->plane_src.y1 = state->src_y >> 16;
> > > > > + iter->plane_src.x2 = iter->plane_src.x1 + (state->src_w >> 16);
> > > > > + iter->plane_src.y2 = iter->plane_src.y1 + (state->src_h >> 16);
> > > > > + iter->translate_plane_x = -iter->plane_src.x1;
> > > > > + iter->translate_plane_y = -iter->plane_src.y1;
> > > > > +
> > > > > + /* Clip plane src rect to fb dimensions */
> > > > > + drm_rect_intersect(&iter->plane_src, &iter->fb_src);
> > > > This smells like driver bug. Also, see Ville's recent efforts to improve
> > > > the atomic plane clipping, I think drm_plane_state already has all the
> > > > clip rects you want.
> > > >
> > > > > +
> > > > > + iter->crtc_src.x1 = 0;
> > > > > + iter->crtc_src.y1 = 0;
> > > > > + iter->crtc_src.x2 = hdisplay;
> > > > > + iter->crtc_src.y2 = vdisplay;
> > > > > + iter->translate_crtc_x = -(iter->plane_src.x1 - state->crtc_x);
> > > > > + iter->translate_crtc_x = -(iter->plane_src.y1 - state->crtc_y);
> > > > > +
> > > > > + /* Clip crtc src rect to plane dimensions */
> > > > > + drm_rect_translate(&iter->crtc_src, -iter->translate_crtc_x,
> > > > > + -iter->translate_crtc_x);
> > > > We can also scale.
> > > I suggest we leave out scaling for now until someone actually needs it.
> > In that case we need to WARN_ON and bail out if there's scaling going on.
> > I'm totally fine with not solving the world here, but please no trapdoors
> > for following driver's use-cases.
> >
>
> Agreed.
>
> > > > > + drm_rect_intersect(&iter->crtc_src, &iter->plane_src);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init);
> > > > > +
> > > > > +/**
> > > > > + * drm_atomic_helper_damage_iter_next - advance the damage iterator
> > > > > + * @iter: The iterator to advance.
> > > > > + * @rect: Return a rectangle in coordinate specified during iterator init.
> > > > > + *
> > > > > + * Returns: true if the output is valid, false if we've reached the end of the
> > > > > + * rectangle list. If the first call return false, means need full update.
> > > > > + */
> > > > > +bool
> > > > > +drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,
> > > > > + struct drm_rect *rect)
> > > > > +{
> > > > > + const struct drm_rect *curr_clip;
> > > > > +
> > > > > +next_clip:
> > > > > + if (iter->curr_clip >= iter->num_clips)
> > > > > + return false;
> > > > > +
> > > > > + curr_clip = &iter->clips[iter->curr_clip];
> > > > > + iter->curr_clip++;
> > > > > +
> > > > > + rect->x1 = curr_clip->x1;
> > > > > + rect->x2 = curr_clip->x2;
> > > > > + rect->y1 = curr_clip->y1;
> > > > > + rect->y2 = curr_clip->y2;
> > > > > +
> > > > > + /* Clip damage rect within fb limit */
> > > > > + if (!drm_rect_intersect(rect, &iter->fb_src))
> > > > > + goto next_clip;
> > > > > + else if (iter->type & DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_FB)
> > > > > + return true;
> > > > > +
> > > > > + /* Clip damage rect within plane limit */
> > > > > + if (!drm_rect_intersect(rect, &iter->plane_src))
> > > > > + goto next_clip;
> > > > > + else if (iter->type & DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_PLANE) {
> > > > > + drm_rect_translate(rect, iter->translate_plane_x,
> > > > > + iter->translate_plane_y);
> > > > > + return true;
> > > > > + }
> > > > > +
> > > > > + /* Clip damage rect within crtc limit */
> > > > > + if (!drm_rect_intersect(rect, &iter->crtc_src))
> > > > > + goto next_clip;
> > > > > +
> > > > > + drm_rect_translate(rect, iter->translate_crtc_x,
> > > > > + iter->translate_crtc_y);
> > > > > +
> > > > > + return true;
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
> > > > > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> > > > > index 26aaba5..ebd4b66 100644
> > > > > --- a/include/drm/drm_atomic_helper.h
> > > > > +++ b/include/drm/drm_atomic_helper.h
> > > > > @@ -36,6 +36,37 @@ struct drm_atomic_state;
> > > > > struct drm_private_obj;
> > > > > struct drm_private_state;
> > > > > +/**
> > > > > + * enum drm_atomic_helper_damage_clip_type - type of clips to iterator over
> > > > > + *
> > > > > + * While using drm_atomic_helper_damage_iter the type of clip coordinates caller
> > > > > + * is interested in.
> > > > > + */
> > > > > +enum drm_atomic_helper_damage_clip_type {
> > > > > + DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_FB = 0x0,
> > > > > + DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_PLANE = 0x1,
> > > > > + DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_CRTC = 0x2,
> > > > I'm confused with what exactly these different types of iterators are
> > > > supposed to achieve. TYPE_FB makes sense, that's what vmwgfx and other
> > > > virtual drivers need to figure out which parts of a buffer to upload to
> > > > the host.
> > > >
> > > > TYPE_PLANE I have no idea who needs that. I suggest we just drop it.
> > > >
> > > > TYPE_CRTC is what I'd want to use for manual upload hw, were instead of
> > > > compositing the entire screen we can limit the uploaded area to 1 or 2
> > > > rectangles (depending upon how the hw works). But those drivers want all
> > > > the crtc clip rects for _all_ the planes combined, not for each plane
> > > > individually.
> > > >
> > > > My suggestion is to drop TYPE_CRTC until someone needs it for a driver.
> > > > And most likely the only iterator we want for TYPE_CRTC is one that gives
> > > > you the overall damage area, including alpha/ctm/gamma/everything else,
> > > > coalesced into just 1 clip rect. So probably an entirely different
> > > > function.
> > > Actually for vmwgfx, the display part of the hardware comes in different
> > > versions and we'd typically expect both the FB coordinates and the CRTC
> > > coordinates. In the latest version the surface backing the screen needs to
> > > exactly fit the CRTC scanout area, and if the FB doesn't do that, we need to
> > > create a surface that does and copy in the kernel driver. After that, as a
> > > second step, we need to notify the display system of the damaged area in
> > > CRTC coordinates.
> > Hm, that's an unexpected use-case. I'm leaning towards just having a fb
> > coordination iterator and adjusting the static offset in vmwgfx code. Your
> > use-case is rather different from manual upload screens I think, so having
> > a CRTC damage iterator which doesn't actually do what most other drivers
> > want will be confusing.
> >
> > And since it's just a static drm_rect_translate for you it'll be only 1
> > additional line in vmwgfx. Imo that's worth the safer/cleaner semantics
> > for the core helpers.
>
> Sounds reasonable. We could have an fb coord iterator only, and do the
> translation to crtc coordinates outside. But I still think we should do the
> plane clipping in
> the iterator, if needed with a big WARN_ON for transformed planes.
Shouldn't need a WARN_ON for transformed planes, since we'd clip to the
src rect. So just agreeing on how we're going to round stuff (I'd vote for
including any partial pixels).
One trouble with this is that if the plane src shifts, we need to upload
any of the new pixels (since we might have skipped them previously). So
the iterator would need to add those on its own as a new dirty rect.
Besides those details I think clipping to the visible area makes sense.
But it's again not entirely trivial, hence selftests for these special
cases would be real good.
Thanks, Daniel
>
> /Thomas
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Thu, Apr 05, 2018 at 10:51:42AM +0200, Thomas Hellstrom wrote:
> On 04/05/2018 09:52 AM, Daniel Vetter wrote:
> >
> > TYPE_PLANE I have no idea who needs that. I suggest we just drop it.
>
> I'm assuming CRTC plane coordinates here. They are used for uploading
> contents of hardware planes. Like, in the simplest case, cursor images.
Hm, I guess I'd need to see how it's being used, since I'm neither
following the intent of the original code nor your explanation here I
think ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On 04/05/2018 03:47 PM, Daniel Vetter wrote:
> On Thu, Apr 05, 2018 at 01:35:25PM +0200, Thomas Hellstrom wrote:
>> On 04/05/2018 12:03 PM, Daniel Vetter wrote:
>>> On Thu, Apr 05, 2018 at 11:00:15AM +0200, Thomas Hellstrom wrote:
>>>> On 04/05/2018 09:35 AM, Daniel Vetter wrote:
>>>>> On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
>>>>>> From: Lukasz Spintzyk <[email protected]>
>>>>>>
>>>>>> Optional plane property to mark damaged regions on the plane in
>>>>>> framebuffer coordinates of the framebuffer attached to the plane.
>>>>>>
>>>>>> The layout of blob data is simply an array of drm_mode_rect with maximum
>>>>>> array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
>>>>>> coordinates, damage clips are not in 16.16 fixed point.
>>>>>>
>>>>>> Damage clips are a hint to kernel as which area of framebuffer has
>>>>>> changed since last page-flip. This should be helpful for some drivers
>>>>>> especially for virtual devices where each framebuffer change needs to
>>>>>> be transmitted over network, usb, etc.
>>>>>>
>>>>>> Driver which are interested in enabling DAMAGE_CLIPS property for a
>>>>>> plane should enable this property using drm_plane_enable_damage_clips.
>>>>>>
>>>>>> Signed-off-by: Lukasz Spintzyk <[email protected]>
>>>>>> Signed-off-by: Deepak Rawat <[email protected]>
>>>>> The property uapi section is missing, see:
>>>>>
>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-2Dcomposition-2Dproperties&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ&m=J31YNV1uz3IVRaorM-eo48msi9__sQurqRZrig2UE1s&s=vzsuquF1agbOw54HSC_18Kk2k9j83m8RusmDCtPAFWQ&e=
>>>>>
>>>>> Plane composition feels like the best place to put this. Please use that
>>>>> section to tie all the various bits together, including the helpers you're
>>>>> adding in the following patches for drivers to use.
>>>>>
>>>>> Bunch of nitpicks below, but overall I'm agreeing now with just going with
>>>>> fb coordinate damage rects.
>>>>>
>>>>> Like you say, the thing needed here now is userspace + driver actually
>>>>> implementing this. I'd also say the compat helper to map the legacy
>>>>> fb->dirty to this new atomic way of doing things should be included here
>>>>> (gives us a lot more testing for these new paths).
>>>>>
>>>>> Icing on the cake would be an igt to make sure kernel rejects invalid clip
>>>>> rects correctly.
>>>>>
>>>>>> ---
>>>>>> drivers/gpu/drm/drm_atomic.c | 42 +++++++++++++++++++++++++++++++++++++
>>>>>> drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
>>>>>> drivers/gpu/drm/drm_mode_config.c | 5 +++++
>>>>>> drivers/gpu/drm/drm_plane.c | 12 +++++++++++
>>>>>> include/drm/drm_mode_config.h | 15 +++++++++++++
>>>>>> include/drm/drm_plane.h | 16 ++++++++++++++
>>>>>> include/uapi/drm/drm_mode.h | 15 +++++++++++++
>>>>>> 7 files changed, 109 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>>>>> index 7d25c42..9226d24 100644
>>>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>>>> @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p,
>>>>>> }
>>>>>> /**
>>>>>> + * drm_atomic_set_damage_for_plane - sets the damage clips property to plane
>>>>>> + * @state: plane state
>>>>>> + * @blob: damage clips in framebuffer coordinates
>>>>>> + *
>>>>>> + * Returns:
>>>>>> + *
>>>>>> + * Zero on success, error code on failure.
>>>>>> + */
>>>>>> +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
>>>>>> + struct drm_property_blob *blob)
>>>>>> +{
>>>>>> + if (blob == state->damage_clips)
>>>>>> + return 0;
>>>>>> +
>>>>>> + drm_property_blob_put(state->damage_clips);
>>>>>> + state->damage_clips = NULL;
>>>>>> +
>>>>>> + if (blob) {
>>>>>> + uint32_t count = blob->length/sizeof(struct drm_rect);
>>>>>> +
>>>>>> + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> + state->damage_clips = drm_property_blob_get(blob);
>>>>>> + state->num_clips = count;
>>>>>> + } else {
>>>>>> + state->damage_clips = NULL;
>>>>>> + state->num_clips = 0;
>>>>>> + }
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> * drm_atomic_get_plane_state - get plane state
>>>>>> * @state: global atomic state object
>>>>>> * @plane: plane to get state object for
>>>>>> @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>>>>>> state->color_encoding = val;
>>>>>> } else if (property == plane->color_range_property) {
>>>>>> state->color_range = val;
>>>>>> + } else if (property == config->prop_damage_clips) {
>>>>>> + struct drm_property_blob *blob =
>>>>>> + drm_property_lookup_blob(dev, val);
>>>>>> + int ret = drm_atomic_set_damage_for_plane(state, blob);
>>>>> There's already a helper with size-checking built-in, see
>>>>> drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd
>>>>> just provide a little inline helper that does the
>>>>> blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).
>>>>>
>>>>>> + drm_property_blob_put(blob);
>>>>>> + return ret;
>>>>>> } else if (plane->funcs->atomic_set_property) {
>>>>>> return plane->funcs->atomic_set_property(plane, state,
>>>>>> property, val);
>>>>>> @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>>>>>> *val = state->color_encoding;
>>>>>> } else if (property == plane->color_range_property) {
>>>>>> *val = state->color_range;
>>>>>> + } else if (property == config->prop_damage_clips) {
>>>>>> + *val = (state->damage_clips) ? state->damage_clips->base.id : 0;
>>>>>> } else if (plane->funcs->atomic_get_property) {
>>>>>> return plane->funcs->atomic_get_property(plane, state, property, val);
>>>>>> } else {
>>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>>>> index c356545..55b44e3 100644
>>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>>>> @@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>>>>>> state->fence = NULL;
>>>>>> state->commit = NULL;
>>>>>> + state->damage_clips = NULL;
>>>>>> + state->num_clips = 0;
>>>>>> }
>>>>>> EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
>>>>>> @@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>>>>>> if (state->commit)
>>>>>> drm_crtc_commit_put(state->commit);
>>>>>> +
>>>>>> + drm_property_blob_put(state->damage_clips);
>>>>>> }
>>>>>> EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>>>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
>>>>>> index e5c6533..e93b127 100644
>>>>>> --- a/drivers/gpu/drm/drm_mode_config.c
>>>>>> +++ b/drivers/gpu/drm/drm_mode_config.c
>>>>>> @@ -293,6 +293,11 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>>>>>> return -ENOMEM;
>>>>>> dev->mode_config.prop_crtc_id = prop;
>>>>>> + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, "DAMAGE_CLIPS", 0);
>>>>> Bit a bikeshed, but since the coordinates are in fb pixels, not plane
>>>>> pixels, I'd call this "FB_DAMAGE_CLIPS".
>>>>>
>>>>>> + if (!prop)
>>>>>> + return -ENOMEM;
>>>>>> + dev->mode_config.prop_damage_clips = prop;
>>>>>> +
>>>>>> prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
>>>>>> "ACTIVE");
>>>>>> if (!prop)
>>>>>> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>>>>>> index 6d2a6e4..071221b 100644
>>>>>> --- a/drivers/gpu/drm/drm_plane.c
>>>>>> +++ b/drivers/gpu/drm/drm_plane.c
>>>>>> @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>>>>>> return ret;
>>>>>> }
>>>>>> +
>>>>>> +/**
>>>>>> + * drm_plane_enable_damage_clips - enable damage clips property
>>>>>> + * @plane: plane on which this property to enable.
>>>>>> + */
>>>>>> +void drm_plane_enable_damage_clips(struct drm_plane *plane)
>>>>>> +{
>>>>>> + struct drm_device *dev = plane->dev;
>>>>>> + struct drm_mode_config *config = &dev->mode_config;
>>>>>> +
>>>>>> + drm_object_attach_property(&plane->base, config->prop_damage_clips, 0);
>>>>>> +}
>>>>>> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
>>>>>> index 7569f22..d8767da 100644
>>>>>> --- a/include/drm/drm_mode_config.h
>>>>>> +++ b/include/drm/drm_mode_config.h
>>>>>> @@ -628,6 +628,21 @@ struct drm_mode_config {
>>>>>> */
>>>>>> struct drm_property *prop_crtc_id;
>>>>>> /**
>>>>>> + * @prop_damage_clips: Optional plane property to mark damaged regions
>>>>>> + * on the plane in framebuffer coordinates of the framebuffer attached
>>>>>> + * to the plane.
>>>>> Why should we make this optional? Looks like just another thing drivers
>>>>> might screw up, since we have multiple callbacks and things to set up for
>>>>> proper dirty tracking.
>>>>>
>>>>> One option I'm seeing is that if this is set, and it's an atomic driver,
>>>>> then we just directly call into the default atomic fb->dirty
>>>>> implementation. That way there's only 1 thing drivers need to do to set up
>>>>> dirty rect tracking, and they'll get all of it.
>>>>>
>>>>>> + *
>>>>>> + * The layout of blob data is simply an array of drm_mode_rect with
>>>>>> + * maximum array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike
>>>>>> + * plane src coordinates, damage clips are not in 16.16 fixed point.
>>>>> I honestly have no idea where this limit is from. Worth keeping? I can
>>>>> easily imagine that userspace could trip over this - it's fairly high, but
>>>>> not unlimited.
>>>>>
>>>>>> + *
>>>>>> + * Damage clips are a hint to kernel as which area of framebuffer has
>>>>>> + * changed since last page-flip. This should be helpful
>>>>>> + * for some drivers especially for virtual devices where each
>>>>>> + * framebuffer change needs to be transmitted over network, usb, etc.
>>>>> I'd also clarify that userspace still must render the entire screen, i.e.
>>>>> make it more clear that it's really just a hint and not mandatory to only
>>>>> scan out the damaged parts.
>>>>>
>>>>>> + */
>>>>>> + struct drm_property *prop_damage_clips;
>>>>>> + /**
>>>>>> * @prop_active: Default atomic CRTC property to control the active
>>>>>> * state, which is the simplified implementation for DPMS in atomic
>>>>>> * drivers.
>>>>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>>>>>> index f7bf4a4..9f24548 100644
>>>>>> --- a/include/drm/drm_plane.h
>>>>>> +++ b/include/drm/drm_plane.h
>>>>>> @@ -146,6 +146,21 @@ struct drm_plane_state {
>>>>>> */
>>>>>> struct drm_crtc_commit *commit;
>>>>>> + /*
>>>>>> + * @damage_clips
>>>>>> + *
>>>>>> + * blob property with damage as array of drm_rect in framebuffer
>>>>> &drm_rect gives you a nice hyperlink in the generated docs.
>>>>>
>>>>>> + * coodinates.
>>>>>> + */
>>>>>> + struct drm_property_blob *damage_clips;
>>>>>> +
>>>>>> + /*
>>>>>> + * @num_clips
>>>>>> + *
>>>>>> + * Number of drm_rect in @damage_clips.
>>>>>> + */
>>>>>> + uint32_t num_clips;
>>>>>> +
>>>>>> struct drm_atomic_state *state;
>>>>>> };
>>>>>> @@ -611,6 +626,7 @@ int drm_plane_init(struct drm_device *dev,
>>>>>> const uint32_t *formats, unsigned int format_count,
>>>>>> bool is_primary);
>>>>>> void drm_plane_cleanup(struct drm_plane *plane);
>>>>>> +void drm_plane_enable_damage_clips(struct drm_plane *plane);
>>>>>> /**
>>>>>> * drm_plane_index - find the index of a registered plane
>>>>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>>>>>> index 50bcf42..0ad0d5b 100644
>>>>>> --- a/include/uapi/drm/drm_mode.h
>>>>>> +++ b/include/uapi/drm/drm_mode.h
>>>>>> @@ -873,6 +873,21 @@ struct drm_mode_revoke_lease {
>>>>>> __u32 lessee_id;
>>>>>> };
>>>>>> +/**
>>>>>> + * struct drm_mode_rect - two dimensional rectangle drm_rect exported to
>>>>>> + * user-space.
>>>>>> + * @x1: horizontal starting coordinate (inclusive)
>>>>>> + * @y1: vertical starting coordinate (inclusive)
>>>>>> + * @x2: horizontal ending coordinate (exclusive)
>>>>>> + * @y2: vertical ending coordinate (exclusive)
>>>>>> + */
>>>>>> +struct drm_mode_rect {
>>>>>> + __s32 x1;
>>>>>> + __s32 y1;
>>>>>> + __s32 x2;
>>>>>> + __s32 y2;
>>>>> Why signed? Negative damage rects on an fb don't make sense to me. Also,
>>>>> please specify what this is exactly (to avoid confusion with the 16.16
>>>>> fixed point src rects), and maybe mention in the commit message why we're
>>>>> not using drm_clip_rect (going to proper uapi types and 32bit makes sense
>>>>> to me).
>>>> IMO, while we don't expect negative damage coordinates,
>>>> to avoid yet another drm uapi rect in the future when we actually need
>>>> negative numbers signed is a good choice...
>>> Makes sense. Another thing I realized: Since src rect are 16.16 fixed
>>> point, do we really need s32? drm_clip_rect is a bit meh, but it gives us
>>> s16 already. That would avoid having to sprinkle the code with tons of
>>> overflow checks for input validation.
>> IMHO, sooner or later we're going to run into the s16 limitation, and I
>> think we should start
>> making user-space APIs > 15 bit coordinate safe. I agree this could lead to
>> some validation
>> grief in the short run, but less to fix up later.
> If all the corner-case validation comes with selftests I'm happy to review
> them for completeness. I'm not good enough to figure out whether there's
> missing stuff by just looking at the validation code generally.
>
>> How difficult is it to make the kernel-internal 16.16 fixed point 48.16?
> Since we range-limit properties it should work out. But that means easy
> bitshifting will much more likely overflow, and I kinda don't want to
> audit all those places for something we don't yet need. Even 8k is still a
> bit off from the limit.
>
> Once we have hw that can scan out 32k buffers we'll probably have to fix
> up everything :-/
We have frequent requests for 4x4K functionality. In the 3D case we're
currently typically restricted by using a single GPU texture as a
framebuffer, but when we're 2D only, like mainly on ESX, there's no such
restriction. Our kernel driver copies out of the giant framebuffer if
needed, but 16 bit is probably soon starting to be a real restriction
(8Kx4) , as least as long as compositors insist on a single giant
framebuffer.
/Thomas
On Wed, Apr 04, 2018 at 04:49:07PM -0700, Deepak Rawat wrote:
> With damage property in drm_plane_state, this patch adds helper iterator
> to traverse the damage clips. Iterator will return the damage rectangles
> in framebuffer, plane or crtc coordinates as need by driver
> implementation.
>
> Signed-off-by: Deepak Rawat <[email protected]>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 122 ++++++++++++++++++++++++++++++++++++
> include/drm/drm_atomic_helper.h | 39 ++++++++++++
> 2 files changed, 161 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 55b44e3..355b514 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3865,3 +3865,125 @@ void __drm_atomic_helper_private_obj_duplicate_state(struct drm_private_obj *obj
> memcpy(state, obj->state, sizeof(*state));
> }
> EXPORT_SYMBOL(__drm_atomic_helper_private_obj_duplicate_state);
> +
> +/**
> + * drm_atomic_helper_damage_iter_init - initialize the damage iterator
> + * @iter: The iterator to initialize.
> + * @type: Coordinate type caller is interested in.
> + * @state: plane_state from which to iterate the damage clips.
> + * @hdisplay: Width of crtc on which plane is scanned out.
> + * @vdisplay: Height of crtc on which plane is scanned out.
> + *
> + * Initialize an iterator that is used to translate and clip a set of damage
> + * rectangles in framebuffer coordinates to plane and crtc coordinates. The type
> + * argument specify which type of coordinate to iterate in.
> + *
> + * Returns: 0 on success and negative error code on error. If an error code is
> + * returned then it means the plane state should not update.
> + */
> +int
> +drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
> + enum drm_atomic_helper_damage_clip_type type,
> + const struct drm_plane_state *state,
> + uint32_t hdisplay, uint32_t vdisplay)
> +{
> + if (!state || !state->crtc || !state->fb)
> + return -EINVAL;
> +
> + memset(iter, 0, sizeof(*iter));
> + iter->clips = (struct drm_rect *)state->damage_clips->data;
> + iter->num_clips = state->num_clips;
> + iter->type = type;
> +
> + /*
> + * Full update in case of scaling or rotation. In future support for
> + * scaling/rotating damage clips can be added
> + */
> + if (state->crtc_w != (state->src_w >> 16) ||
> + state->crtc_h != state->src_h >> 16 || state->rotation != 0) {
> + iter->curr_clip = iter->num_clips;
> + return 0;
> + }
> +
> + iter->fb_src.x1 = 0;
> + iter->fb_src.y1 = 0;
> + iter->fb_src.x2 = state->fb->width;
> + iter->fb_src.y2 = state->fb->height;
> +
> + iter->plane_src.x1 = state->src_x >> 16;
> + iter->plane_src.y1 = state->src_y >> 16;
> + iter->plane_src.x2 = iter->plane_src.x1 + (state->src_w >> 16);
> + iter->plane_src.y2 = iter->plane_src.y1 + (state->src_h >> 16);
> + iter->translate_plane_x = -iter->plane_src.x1;
> + iter->translate_plane_y = -iter->plane_src.y1;
> +
> + /* Clip plane src rect to fb dimensions */
> + drm_rect_intersect(&iter->plane_src, &iter->fb_src);
> +
> + iter->crtc_src.x1 = 0;
> + iter->crtc_src.y1 = 0;
> + iter->crtc_src.x2 = hdisplay;
> + iter->crtc_src.y2 = vdisplay;
> + iter->translate_crtc_x = -(iter->plane_src.x1 - state->crtc_x);
> + iter->translate_crtc_x = -(iter->plane_src.y1 - state->crtc_y);
^
I believe you mean translate_crtc_y here
> +
> + /* Clip crtc src rect to plane dimensions */
> + drm_rect_translate(&iter->crtc_src, -iter->translate_crtc_x,
> + -iter->translate_crtc_x);
Also here ^
>
> On Wed, Apr 04, 2018 at 04:49:05PM -0700, Deepak Rawat wrote:
> > Hi All,
> >
> > This is extension to Lukasz Spintzyk earlier draft of damage interface for
> drm.
> > Bascially a new plane property is added called "DAMAGE_CLIPS" which is
> simply
> > an array of drm_rect (exported to userspace as drm_mode_rect). The clips
> > represents damage in framebuffer coordinate of attached fb to the plane.
> >
> > Helper iterator is added to traverse the damage rectangles and get the
> damage
> > clips in framebuffer, plane or crtc coordinates as need by driver
> > implementation. Finally a helper to reset damage in case need full update is
> > required. Drivers interested in page-flip with damage should call this from
> > atomic_check hook.
> >
> > With the RFC for atomic implementation of dirtyfb ioctl I was thinking
> > should we need to consider dirty_fb flags, especially
> > DRM_MODE_FB_DIRTY_ANNOTATE_COPY to be passed with atomic
> > DAMAGE_CLIPS property blob? I didn't considered that untill now. If no
> driver
> > uses that in my opinion for simplicity this can be ignored?
>
> Last time I've checked no driver nor userspace really used this. I'd drop
> it for the new uapi like you've done.
Thanks Daniel for the review.
Agreed, will drop dirty_fb flags unless someone has any objection.
>
> > About overlaping of damage rectangles is also not finalized. This really
> > depends on driver specific implementation and can be left open-ended?
>
> Overlapping is userspace being stupid imo :-) I guess we should say that
> drivers should at least not fall over overlapping rects, and if they can't
> handle them, either coalesce into one big rect, or split them on their own
> somehow.
I think the best is to suggest user-space to not send overlapped rects. But
as someone earlier suggested it is hard or I should say unnecessary check
to enforce this in driver.
>
> > My knowledge is limited to vmwgfx so would like to hear about other
> driver use
> > cases and this can be modified in keeping other drivers need.
> >
> > Going forward driver implementation for vmwgfx and user-space
> implementation
> > of kmscube/weston will be next step to test the changes.
>
> I think an implementation for -modesetting and weston would be perfect.
> Kmscube has very littel value for damage tracking purposes (it's not a
> full compositor, just renders new frame each scene). And for kms I'm not a
> fan of using vendor-specific drivers to demonstrate new generic uapi - way
> too big chances you're making some vendor driver specific hardcoded
> assumption that doesn't hold anywhere else. Also makes it harder for
> others to test-implement your uapi in their drivers.
> -Daniel
Agreed implementation in modesetting and Weston and for kernel part as
I read in another mail dirty_fb implementation is also a good use-case.
>
> >
> > Thanks,
> > Deepak
> >
> > Deepak Rawat (2):
> > drm: Add helper iterator functions to iterate over plane damage.
> > drm: Add helper to validate damage during modeset_check
> >
> > Lukasz Spintzyk (1):
> > drm: Add DAMAGE_CLIPS property to plane
> >
> > drivers/gpu/drm/drm_atomic.c | 42 +++++++++
> > drivers/gpu/drm/drm_atomic_helper.c | 173
> ++++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/drm_mode_config.c | 5 ++
> > drivers/gpu/drm/drm_plane.c | 12 +++
> > include/drm/drm_atomic_helper.h | 41 +++++++++
> > include/drm/drm_mode_config.h | 15 ++++
> > include/drm/drm_plane.h | 16 ++++
> > include/uapi/drm/drm_mode.h | 15 ++++
> > 8 files changed, 319 insertions(+)
> >
> > --
> > 2.7.4
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__blog.ffwll.ch&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK
> 0762SxAf-
> cyZdStnd2NQpRu98lJP2iYGw&m=CrefGziKAEk50MpTaNv64iDljHY7GesUlFHZc
> hWpiqI&s=gFk0ko5cD_cnIyPuZOrqyAwLsmTt9PiSDbWdHoi-8cU&e=
>
> On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
> > From: Lukasz Spintzyk <[email protected]>
> >
> > Optional plane property to mark damaged regions on the plane in
> > framebuffer coordinates of the framebuffer attached to the plane.
> >
> > The layout of blob data is simply an array of drm_mode_rect with
> maximum
> > array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
> > coordinates, damage clips are not in 16.16 fixed point.
> >
> > Damage clips are a hint to kernel as which area of framebuffer has
> > changed since last page-flip. This should be helpful for some drivers
> > especially for virtual devices where each framebuffer change needs to
> > be transmitted over network, usb, etc.
> >
> > Driver which are interested in enabling DAMAGE_CLIPS property for a
> > plane should enable this property using drm_plane_enable_damage_clips.
> >
> > Signed-off-by: Lukasz Spintzyk <[email protected]>
> > Signed-off-by: Deepak Rawat <[email protected]>
>
> The property uapi section is missing, see:
>
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-
> 2Dcomposition-
> 2Dproperties&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762
> SxAf-
> cyZdStnd2NQpRu98lJP2iYGw&m=ELAUsNTjD0ICwUEDFjPW4jsmy2A5AkhS5Q
> z_3vlEC9Q&s=nH-HNXPczoJQMj1iwHiGfrhXz4-00b0M8-3kirjm-EY&e=
>
> Plane composition feels like the best place to put this. Please use that
> section to tie all the various bits together, including the helpers you're
> adding in the following patches for drivers to use.
>
> Bunch of nitpicks below, but overall I'm agreeing now with just going with
> fb coordinate damage rects.
>
> Like you say, the thing needed here now is userspace + driver actually
> implementing this. I'd also say the compat helper to map the legacy
> fb->dirty to this new atomic way of doing things should be included here
> (gives us a lot more testing for these new paths).
>
> Icing on the cake would be an igt to make sure kernel rejects invalid clip
> rects correctly.
>
> > ---
> > drivers/gpu/drm/drm_atomic.c | 42
> +++++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
> > drivers/gpu/drm/drm_mode_config.c | 5 +++++
> > drivers/gpu/drm/drm_plane.c | 12 +++++++++++
> > include/drm/drm_mode_config.h | 15 +++++++++++++
> > include/drm/drm_plane.h | 16 ++++++++++++++
> > include/uapi/drm/drm_mode.h | 15 +++++++++++++
> > 7 files changed, 109 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c
> b/drivers/gpu/drm/drm_atomic.c
> > index 7d25c42..9226d24 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct
> drm_printer *p,
> > }
> >
> > /**
> > + * drm_atomic_set_damage_for_plane - sets the damage clips property
> to plane
> > + * @state: plane state
> > + * @blob: damage clips in framebuffer coordinates
> > + *
> > + * Returns:
> > + *
> > + * Zero on success, error code on failure.
> > + */
> > +static int drm_atomic_set_damage_for_plane(struct drm_plane_state
> *state,
> > + struct drm_property_blob *blob)
> > +{
> > + if (blob == state->damage_clips)
> > + return 0;
> > +
> > + drm_property_blob_put(state->damage_clips);
> > + state->damage_clips = NULL;
> > +
> > + if (blob) {
> > + uint32_t count = blob->length/sizeof(struct drm_rect);
> > +
> > + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
> > + return -EINVAL;
> > +
> > + state->damage_clips = drm_property_blob_get(blob);
> > + state->num_clips = count;
> > + } else {
> > + state->damage_clips = NULL;
> > + state->num_clips = 0;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > * drm_atomic_get_plane_state - get plane state
> > * @state: global atomic state object
> > * @plane: plane to get state object for
> > @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct
> drm_plane *plane,
> > state->color_encoding = val;
> > } else if (property == plane->color_range_property) {
> > state->color_range = val;
> > + } else if (property == config->prop_damage_clips) {
> > + struct drm_property_blob *blob =
> > + drm_property_lookup_blob(dev, val);
> > + int ret = drm_atomic_set_damage_for_plane(state, blob);
>
> There's already a helper with size-checking built-in, see
> drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips
> I'd
> just provide a little inline helper that does the
> blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).
>
> > + drm_property_blob_put(blob);
> > + return ret;
> > } else if (plane->funcs->atomic_set_property) {
> > return plane->funcs->atomic_set_property(plane, state,
> > property, val);
> > @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane
> *plane,
> > *val = state->color_encoding;
> > } else if (property == plane->color_range_property) {
> > *val = state->color_range;
> > + } else if (property == config->prop_damage_clips) {
> > + *val = (state->damage_clips) ? state->damage_clips->base.id
> : 0;
> > } else if (plane->funcs->atomic_get_property) {
> > return plane->funcs->atomic_get_property(plane, state,
> property, val);
> > } else {
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> > index c356545..55b44e3 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -3506,6 +3506,8 @@ void
> __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
> >
> > state->fence = NULL;
> > state->commit = NULL;
> > + state->damage_clips = NULL;
> > + state->num_clips = 0;
> > }
> > EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
> >
> > @@ -3550,6 +3552,8 @@ void
> __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
> >
> > if (state->commit)
> > drm_crtc_commit_put(state->commit);
> > +
> > + drm_property_blob_put(state->damage_clips);
> > }
> > EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
> >
> > diff --git a/drivers/gpu/drm/drm_mode_config.c
> b/drivers/gpu/drm/drm_mode_config.c
> > index e5c6533..e93b127 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -293,6 +293,11 @@ static int
> drm_mode_create_standard_properties(struct drm_device *dev)
> > return -ENOMEM;
> > dev->mode_config.prop_crtc_id = prop;
> >
> > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> "DAMAGE_CLIPS", 0);
>
> Bit a bikeshed, but since the coordinates are in fb pixels, not plane
> pixels, I'd call this "FB_DAMAGE_CLIPS".
>
> > + if (!prop)
> > + return -ENOMEM;
> > + dev->mode_config.prop_damage_clips = prop;
> > +
> > prop = drm_property_create_bool(dev,
> DRM_MODE_PROP_ATOMIC,
> > "ACTIVE");
> > if (!prop)
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index 6d2a6e4..071221b 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct
> drm_device *dev,
> >
> > return ret;
> > }
> > +
> > +/**
> > + * drm_plane_enable_damage_clips - enable damage clips property
> > + * @plane: plane on which this property to enable.
> > + */
> > +void drm_plane_enable_damage_clips(struct drm_plane *plane)
> > +{
> > + struct drm_device *dev = plane->dev;
> > + struct drm_mode_config *config = &dev->mode_config;
> > +
> > + drm_object_attach_property(&plane->base, config-
> >prop_damage_clips, 0);
> > +}
> > diff --git a/include/drm/drm_mode_config.h
> b/include/drm/drm_mode_config.h
> > index 7569f22..d8767da 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -628,6 +628,21 @@ struct drm_mode_config {
> > */
> > struct drm_property *prop_crtc_id;
> > /**
> > + * @prop_damage_clips: Optional plane property to mark damaged
> regions
> > + * on the plane in framebuffer coordinates of the framebuffer
> attached
> > + * to the plane.
>
> Why should we make this optional? Looks like just another thing drivers
> might screw up, since we have multiple callbacks and things to set up for
> proper dirty tracking.
Thanks Daniel for the review.
I think not all compositor will be interested in sending damage, that was the
reason to make this optional. Also when damage is not set that means
user-space need full update just like eglSwapBuffersWithDamageKHR.
I will add better documentation.
>
> One option I'm seeing is that if this is set, and it's an atomic driver,
> then we just directly call into the default atomic fb->dirty
> implementation. That way there's only 1 thing drivers need to do to set up
> dirty rect tracking, and they'll get all of it.
>
> > + *
> > + * The layout of blob data is simply an array of drm_mode_rect with
> > + * maximum array size limited by
> DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike
> > + * plane src coordinates, damage clips are not in 16.16 fixed point.
>
> I honestly have no idea where this limit is from. Worth keeping? I can
> easily imagine that userspace could trip over this - it's fairly high, but
> not unlimited.
DRM_MODE_FB_DIRTY_MAX_CLIPS was used to just have an upper limit
and its already exposed to user-space. IMHO there has to be an upper limit
to avoid unnecessary overflow ?
>
> > + *
> > + * Damage clips are a hint to kernel as which area of framebuffer has
> > + * changed since last page-flip. This should be helpful
> > + * for some drivers especially for virtual devices where each
> > + * framebuffer change needs to be transmitted over network, usb,
> etc.
>
> I'd also clarify that userspace still must render the entire screen, i.e.
> make it more clear that it's really just a hint and not mandatory to only
> scan out the damaged parts.
Yes will work on better documentation.
>
> > + */
> > + struct drm_property *prop_damage_clips;
> > + /**
> > * @prop_active: Default atomic CRTC property to control the active
> > * state, which is the simplified implementation for DPMS in atomic
> > * drivers.
> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > index f7bf4a4..9f24548 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -146,6 +146,21 @@ struct drm_plane_state {
> > */
> > struct drm_crtc_commit *commit;
> >
> > + /*
> > + * @damage_clips
> > + *
> > + * blob property with damage as array of drm_rect in framebuffer
>
> &drm_rect gives you a nice hyperlink in the generated docs.
>
> > + * coodinates.
> > + */
> > + struct drm_property_blob *damage_clips;
> > +
> > + /*
> > + * @num_clips
> > + *
> > + * Number of drm_rect in @damage_clips.
> > + */
> > + uint32_t num_clips;
> > +
> > struct drm_atomic_state *state;
> > };
> >
> > @@ -611,6 +626,7 @@ int drm_plane_init(struct drm_device *dev,
> > const uint32_t *formats, unsigned int format_count,
> > bool is_primary);
> > void drm_plane_cleanup(struct drm_plane *plane);
> > +void drm_plane_enable_damage_clips(struct drm_plane *plane);
> >
> > /**
> > * drm_plane_index - find the index of a registered plane
> > diff --git a/include/uapi/drm/drm_mode.h
> b/include/uapi/drm/drm_mode.h
> > index 50bcf42..0ad0d5b 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -873,6 +873,21 @@ struct drm_mode_revoke_lease {
> > __u32 lessee_id;
> > };
> >
> > +/**
> > + * struct drm_mode_rect - two dimensional rectangle drm_rect exported
> to
> > + * user-space.
> > + * @x1: horizontal starting coordinate (inclusive)
> > + * @y1: vertical starting coordinate (inclusive)
> > + * @x2: horizontal ending coordinate (exclusive)
> > + * @y2: vertical ending coordinate (exclusive)
> > + */
> > +struct drm_mode_rect {
> > + __s32 x1;
> > + __s32 y1;
> > + __s32 x2;
> > + __s32 y2;
>
> Why signed? Negative damage rects on an fb don't make sense to me. Also,
> please specify what this is exactly (to avoid confusion with the 16.16
> fixed point src rects), and maybe mention in the commit message why we're
> not using drm_clip_rect (going to proper uapi types and 32bit makes sense
> to me).
>
> Cheers, Daniel
> > +};
> > +
> > #if defined(__cplusplus)
> > }
> > #endif
> > --
> > 2.7.4
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__blog.ffwll.ch&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK
> 0762SxAf-
> cyZdStnd2NQpRu98lJP2iYGw&m=ELAUsNTjD0ICwUEDFjPW4jsmy2A5AkhS5Q
> z_3vlEC9Q&s=kLx5XCTMRYRoecNhwwwN2XItT4Rt0ib12-UP5VB4XLI&e=
>
> On Wed, Apr 04, 2018 at 04:49:07PM -0700, Deepak Rawat wrote:
> > With damage property in drm_plane_state, this patch adds helper iterator
> > to traverse the damage clips. Iterator will return the damage rectangles
> > in framebuffer, plane or crtc coordinates as need by driver
> > implementation.
> >
> > Signed-off-by: Deepak Rawat <[email protected]>
>
> I'd really like selftest/unittests for this stuff. There's an awful lot of
> cornercases in this here (for any of the transformed iterators at least),
> and unit tests is the best way to make sure we handle them all correctly.
>
> Bonus points if you integrate the new selftests into igt so intel CI can
> run them, seel igt/tests/drm_mm.c for an example. drm_mm selftest is also
> the framework I'd copy for this stuff.
>
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 122
> ++++++++++++++++++++++++++++++++++++
> > include/drm/drm_atomic_helper.h | 39 ++++++++++++
> > 2 files changed, 161 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> > index 55b44e3..355b514 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -3865,3 +3865,125 @@ void
> __drm_atomic_helper_private_obj_duplicate_state(struct drm_private_obj
> *obj
> > memcpy(state, obj->state, sizeof(*state));
> > }
> > EXPORT_SYMBOL(__drm_atomic_helper_private_obj_duplicate_state);
> > +
> > +/**
> > + * drm_atomic_helper_damage_iter_init - initialize the damage iterator
> > + * @iter: The iterator to initialize.
> > + * @type: Coordinate type caller is interested in.
> > + * @state: plane_state from which to iterate the damage clips.
> > + * @hdisplay: Width of crtc on which plane is scanned out.
> > + * @vdisplay: Height of crtc on which plane is scanned out.
> > + *
> > + * Initialize an iterator that is used to translate and clip a set of damage
> > + * rectangles in framebuffer coordinates to plane and crtc coordinates.
> The type
> > + * argument specify which type of coordinate to iterate in.
> > + *
> > + * Returns: 0 on success and negative error code on error. If an error code
> is
> > + * returned then it means the plane state should not update.
> > + */
> > +int
> > +drm_atomic_helper_damage_iter_init(struct
> drm_atomic_helper_damage_iter *iter,
> > + enum
> drm_atomic_helper_damage_clip_type type,
> > + const struct drm_plane_state *state,
> > + uint32_t hdisplay, uint32_t vdisplay)
> > +{
> > + if (!state || !state->crtc || !state->fb)
> > + return -EINVAL;
> > +
> > + memset(iter, 0, sizeof(*iter));
> > + iter->clips = (struct drm_rect *)state->damage_clips->data;
> > + iter->num_clips = state->num_clips;
> > + iter->type = type;
> > +
> > + /*
> > + * Full update in case of scaling or rotation. In future support for
> > + * scaling/rotating damage clips can be added
> > + */
> > + if (state->crtc_w != (state->src_w >> 16) ||
> > + state->crtc_h != state->src_h >> 16 || state->rotation != 0) {
> > + iter->curr_clip = iter->num_clips;
> > + return 0;
>
> Given there's no user of this I have no idea how this manages to provoke a
> full clip rect. selftest code would be perfect for stuff like this.
>
> Also, I think we should provide a full clip for the case of num_clips ==
> 0, so that driver code can simply iterate over all clips and doesn't ever
> have to handle the "no clip rects provided" case as a special case itself.
The notion was if iterator does not provide any clip rect then driver need a
full update but yes I will work on providing a full clip here.
>
> > + }
> > +
> > + iter->fb_src.x1 = 0;
> > + iter->fb_src.y1 = 0;
> > + iter->fb_src.x2 = state->fb->width;
> > + iter->fb_src.y2 = state->fb->height;
> > +
> > + iter->plane_src.x1 = state->src_x >> 16;
> > + iter->plane_src.y1 = state->src_y >> 16;
> > + iter->plane_src.x2 = iter->plane_src.x1 + (state->src_w >> 16);
> > + iter->plane_src.y2 = iter->plane_src.y1 + (state->src_h >> 16);
> > + iter->translate_plane_x = -iter->plane_src.x1;
> > + iter->translate_plane_y = -iter->plane_src.y1;
> > +
> > + /* Clip plane src rect to fb dimensions */
> > + drm_rect_intersect(&iter->plane_src, &iter->fb_src);
>
> This smells like driver bug. Also, see Ville's recent efforts to improve
> the atomic plane clipping, I think drm_plane_state already has all the
> clip rects you want.
>
> > +
> > + iter->crtc_src.x1 = 0;
> > + iter->crtc_src.y1 = 0;
> > + iter->crtc_src.x2 = hdisplay;
> > + iter->crtc_src.y2 = vdisplay;
> > + iter->translate_crtc_x = -(iter->plane_src.x1 - state->crtc_x);
> > + iter->translate_crtc_x = -(iter->plane_src.y1 - state->crtc_y);
> > +
> > + /* Clip crtc src rect to plane dimensions */
> > + drm_rect_translate(&iter->crtc_src, -iter->translate_crtc_x,
> > + -iter->translate_crtc_x);
>
> We can also scale.
>
> > + drm_rect_intersect(&iter->crtc_src, &iter->plane_src);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init);
> > +
> > +/**
> > + * drm_atomic_helper_damage_iter_next - advance the damage iterator
> > + * @iter: The iterator to advance.
> > + * @rect: Return a rectangle in coordinate specified during iterator init.
> > + *
> > + * Returns: true if the output is valid, false if we've reached the end of
> the
> > + * rectangle list. If the first call return false, means need full update.
> > + */
> > +bool
> > +drm_atomic_helper_damage_iter_next(struct
> drm_atomic_helper_damage_iter *iter,
> > + struct drm_rect *rect)
> > +{
> > + const struct drm_rect *curr_clip;
> > +
> > +next_clip:
> > + if (iter->curr_clip >= iter->num_clips)
> > + return false;
> > +
> > + curr_clip = &iter->clips[iter->curr_clip];
> > + iter->curr_clip++;
> > +
> > + rect->x1 = curr_clip->x1;
> > + rect->x2 = curr_clip->x2;
> > + rect->y1 = curr_clip->y1;
> > + rect->y2 = curr_clip->y2;
> > +
> > + /* Clip damage rect within fb limit */
> > + if (!drm_rect_intersect(rect, &iter->fb_src))
> > + goto next_clip;
> > + else if (iter->type &
> DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_FB)
> > + return true;
> > +
> > + /* Clip damage rect within plane limit */
> > + if (!drm_rect_intersect(rect, &iter->plane_src))
> > + goto next_clip;
> > + else if (iter->type &
> DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_PLANE) {
> > + drm_rect_translate(rect, iter->translate_plane_x,
> > + iter->translate_plane_y);
> > + return true;
> > + }
> > +
> > + /* Clip damage rect within crtc limit */
> > + if (!drm_rect_intersect(rect, &iter->crtc_src))
> > + goto next_clip;
> > +
> > + drm_rect_translate(rect, iter->translate_crtc_x,
> > + iter->translate_crtc_y);
> > +
> > + return true;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
> > diff --git a/include/drm/drm_atomic_helper.h
> b/include/drm/drm_atomic_helper.h
> > index 26aaba5..ebd4b66 100644
> > --- a/include/drm/drm_atomic_helper.h
> > +++ b/include/drm/drm_atomic_helper.h
> > @@ -36,6 +36,37 @@ struct drm_atomic_state;
> > struct drm_private_obj;
> > struct drm_private_state;
> >
> > +/**
> > + * enum drm_atomic_helper_damage_clip_type - type of clips to iterator
> over
> > + *
> > + * While using drm_atomic_helper_damage_iter the type of clip
> coordinates caller
> > + * is interested in.
> > + */
> > +enum drm_atomic_helper_damage_clip_type {
> > + DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_FB = 0x0,
> > + DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_PLANE = 0x1,
> > + DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_CRTC = 0x2,
>
> I'm confused with what exactly these different types of iterators are
> supposed to achieve. TYPE_FB makes sense, that's what vmwgfx and other
> virtual drivers need to figure out which parts of a buffer to upload to
> the host.
>
> TYPE_PLANE I have no idea who needs that. I suggest we just drop it.
>
> TYPE_CRTC is what I'd want to use for manual upload hw, were instead of
> compositing the entire screen we can limit the uploaded area to 1 or 2
> rectangles (depending upon how the hw works). But those drivers want all
> the crtc clip rects for _all_ the planes combined, not for each plane
> individually.
>
> My suggestion is to drop TYPE_CRTC until someone needs it for a driver.
> And most likely the only iterator we want for TYPE_CRTC is one that gives
> you the overall damage area, including alpha/ctm/gamma/everything else,
> coalesced into just 1 clip rect. So probably an entirely different
> function.
>
> Summarizing all this, I'd simplify the iterator to:
>
> drm_atomic_helper_damage_iter_init(iter, plane_state);
>
> And leave the other 2 use-cases to the next patch series. For crtc damage
> we probably want:
>
> drm_atomic_helper_crtc_damage(drm_atomic_state, rect)
>
> Which internally loops over all planes and also takes all the other state
> changes into account. That way you also don't have to fix the scaling
> issue, since your current code only handles translation.
>
> Another bit: drm_atomic_helper.c is huge, I'd vote to put all this stuff
> into a new drm_damage_helper.[hc], including new section in drm-kms.rst
> and all that. Splitting up drm_atomic_helper.[hc] is somewhere on my todo
Agreed with the conclusion with inputs from other email.
> ...
>
> Cheers, Daniel
>
> > +};
> > +
> > +/**
> > + * struct drm_atomic_helper_damage_iter - damage clip iterator
> > + *
> > + * This iterator tracks state needed to walk the list of damage clips.
> > + */
> > +struct drm_atomic_helper_damage_iter {
> > + enum drm_atomic_helper_damage_clip_type type;
> > + const struct drm_rect *clips;
> > + uint32_t num_clips;
> > + uint32_t curr_clip;
> > + struct drm_rect fb_src;
> > + int translate_plane_x;
> > + int translate_plane_y;
> > + struct drm_rect plane_src;
> > + int translate_crtc_x;
> > + int translate_crtc_y;
> > + struct drm_rect crtc_src;
> > +};
> > +
> > int drm_atomic_helper_check_modeset(struct drm_device *dev,
> > struct drm_atomic_state *state);
> > int drm_atomic_helper_check_plane_state(struct drm_plane_state
> *plane_state,
> > @@ -185,6 +216,14 @@ int drm_atomic_helper_legacy_gamma_set(struct
> drm_crtc *crtc,
> > struct drm_modeset_acquire_ctx *ctx);
> > void __drm_atomic_helper_private_obj_duplicate_state(struct
> drm_private_obj *obj,
> > struct drm_private_state
> *state);
> > +int
> > +drm_atomic_helper_damage_iter_init(struct
> drm_atomic_helper_damage_iter *iter,
> > + enum
> drm_atomic_helper_damage_clip_type type,
> > + const struct drm_plane_state *state,
> > + uint32_t hdisplay, uint32_t vdisplay);
> > +bool
> > +drm_atomic_helper_damage_iter_next(struct
> drm_atomic_helper_damage_iter *iter,
> > + struct drm_rect *rect);
> >
> > /**
> > * drm_atomic_crtc_for_each_plane - iterate over planes currently
> attached to CRTC
> > --
> > 2.7.4
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__blog.ffwll.ch&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK
> 0762SxAf-
> cyZdStnd2NQpRu98lJP2iYGw&m=OWr46Afx4MYOgDehJbODL7IzBsDEeoGiJn
> okZtIh2Qc&s=BH7dN6UEpjEaMKYHooi2AKk-zLYHXl5F7YT7j55qWO8&e=
>
> On Wed, Apr 04, 2018 at 04:49:08PM -0700, Deepak Rawat wrote:
> > This patch adds a helper which should be called by driver which enable
> > damage (by calling drm_plane_enable_damage_clips) from atomic_check
> > hook. This helper for now set the damage to NULL for the planes on crtc
> > which need full modeset.
> >
> > The driver also need to check for other crtc properties which can
> > affect damage in atomic_check hook, like gamma, ctm, etc. Plane related
> > properties which affect damage can be handled in damage iterator.
> >
> > Signed-off-by: Deepak Rawat <[email protected]>
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 47
> +++++++++++++++++++++++++++++++++++++
> > include/drm/drm_atomic_helper.h | 2 ++
> > 2 files changed, 49 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> > index 355b514..23f44ab 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -3987,3 +3987,50 @@ drm_atomic_helper_damage_iter_next(struct
> drm_atomic_helper_damage_iter *iter,
> > return true;
> > }
> > EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
> > +
> > +/**
> > + * drm_atomic_helper_check_damage - validate state object for damage
> changes
> > + * @dev: DRM device
> > + * @state: the driver state object
> > + *
> > + * Check the state object if damage is required or not. In case damage is
> not
> > + * required e.g. need modeset, the damage blob is set to NULL.
>
> Why is that needed?
>
> I can imagine that drivers unconditionally upload everything for a
> modeset, and hence don't need special damage tracking. But for that it's
> imo better to have a clear_damage() helper.
Don't we need a generic helper which all drivers can use to see if anything
in drm_atomic_state will result in full update? My intention for calling that
function from atomic_check hook was because state access can
return -EDEADLK.
I think for now having drm_damage_helper_clear_damage helper and
calling it from atomic_check seems better option. In future once I have
clear idea, a generic function can be done.
>
> But even for modesets (e.g. resolution changes) I'd expect that virtual
> drivers don't want to upload unecessary amounts of data. Manual upload
> hw drivers probably need to upload everything, because the panel will have
> forgotten all the old data once power cycled.
AFAIK current vmwgfx will do full upload for resolution change.
>
> And if you think this is really the right thing, then we need to rename
> the function to tell what it does, e.g.
>
> drm_damage_helper_clear_damage_on_modeset()
>
> drm_damage_helper because I think we should stuff this all into
> drm_damage_helper.c, see previous patch.
>
> But I think a
>
> drm_damage_helper_clear_damage(crtc_state)
>
> which you can use in your crtc->atomic_check function like
>
> crtc_atomic_check(state)
> {
> if (drm_atomic_crtc_needs_modeset(state))
> drm_damage_helper_clear_damage(state);
> }
>
> is more flexible and useful for drivers. There might be other cases where
> clearing damage is the right thing to do. Also, there's the question of
> whether no damage clips == full damage or not, so maybe we should call
> this helper full_damage() instead.
In my opinion if via proper documentation it is conveyed that no damage
means full-update the clear_damage makes sense.
> -Daniel
>
> > + *
> > + * NOTE: This helper is not called by core. Those driver which enable
> damage
> > + * using drm_plane_enable_damage_clips should call this from their
> @atomic_check
> > + * hook.
> > + */
> > +int drm_atomic_helper_check_damage(struct drm_device *dev,
> > + struct drm_atomic_state *state)
> > +{
> > + struct drm_crtc *crtc;
> > + struct drm_plane *plane;
> > + struct drm_crtc_state *crtc_state;
> > + unsigned plane_mask;
> > + int i;
> > +
> > + for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > + if (!drm_atomic_crtc_needs_modeset(crtc_state))
> > + continue;
> > +
> > + plane_mask = crtc_state->plane_mask;
> > + plane_mask |= crtc->state->plane_mask;
> > +
> > + drm_for_each_plane_mask(plane, dev, plane_mask) {
> > + struct drm_plane_state *plane_state =
> > + drm_atomic_get_plane_state(state, plane);
> > +
> > + if (IS_ERR(plane_state))
> > + return PTR_ERR(plane_state);
> > +
> > + if (plane_state->damage_clips) {
> > + drm_property_blob_put(plane_state-
> >damage_clips);
> > + plane_state->damage_clips = NULL;
> > + plane_state->num_clips = 0;
> > + }
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_check_damage);
> > diff --git a/include/drm/drm_atomic_helper.h
> b/include/drm/drm_atomic_helper.h
> > index ebd4b66..b12335c 100644
> > --- a/include/drm/drm_atomic_helper.h
> > +++ b/include/drm/drm_atomic_helper.h
> > @@ -224,6 +224,8 @@ drm_atomic_helper_damage_iter_init(struct
> drm_atomic_helper_damage_iter *iter,
> > bool
> > drm_atomic_helper_damage_iter_next(struct
> drm_atomic_helper_damage_iter *iter,
> > struct drm_rect *rect);
> > +int drm_atomic_helper_check_damage(struct drm_device *dev,
> > + struct drm_atomic_state *state);
> >
> > /**
> > * drm_atomic_crtc_for_each_plane - iterate over planes currently
> attached to CRTC
> > --
> > 2.7.4
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__blog.ffwll.ch&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK
> 0762SxAf-
> cyZdStnd2NQpRu98lJP2iYGw&m=70sQYwsOsrWAPt1SdaK8gDC1Fr3KTUpJLw
> 008Coi8rY&s=wCKqHwASJhJBVWlirJDaofj0YDju_QHCPE4uZw8W3Mg&e=
> plane damage.
>
> On 04/05/2018 09:52 AM, Daniel Vetter wrote:
> >
> > TYPE_PLANE I have no idea who needs that. I suggest we just drop it.
>
> I'm assuming CRTC plane coordinates here. They are used for uploading
> contents of hardware planes. Like, in the simplest case, cursor images.
Yes they are CRTC plane coordinates, so is TYPE_PLANE naming confusing ?
And should be named to TYPE_CRTC_PLANE but then it is confusing with
TYPE_CRTC.
>
> /Thomas
On Thu, Apr 05, 2018 at 11:07:19PM +0000, Deepak Singh Rawat wrote:
> >
> > On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
> > > From: Lukasz Spintzyk <[email protected]>
> > >
> > > Optional plane property to mark damaged regions on the plane in
> > > framebuffer coordinates of the framebuffer attached to the plane.
> > >
> > > The layout of blob data is simply an array of drm_mode_rect with
> > maximum
> > > array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
> > > coordinates, damage clips are not in 16.16 fixed point.
> > >
> > > Damage clips are a hint to kernel as which area of framebuffer has
> > > changed since last page-flip. This should be helpful for some drivers
> > > especially for virtual devices where each framebuffer change needs to
> > > be transmitted over network, usb, etc.
> > >
> > > Driver which are interested in enabling DAMAGE_CLIPS property for a
> > > plane should enable this property using drm_plane_enable_damage_clips.
> > >
> > > Signed-off-by: Lukasz Spintzyk <[email protected]>
> > > Signed-off-by: Deepak Rawat <[email protected]>
> >
> > The property uapi section is missing, see:
> >
> > https://urldefense.proofpoint.com/v2/url?u=https-
> > 3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-
> > 2Dcomposition-
> > 2Dproperties&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762
> > SxAf-
> > cyZdStnd2NQpRu98lJP2iYGw&m=ELAUsNTjD0ICwUEDFjPW4jsmy2A5AkhS5Q
> > z_3vlEC9Q&s=nH-HNXPczoJQMj1iwHiGfrhXz4-00b0M8-3kirjm-EY&e=
> >
> > Plane composition feels like the best place to put this. Please use that
> > section to tie all the various bits together, including the helpers you're
> > adding in the following patches for drivers to use.
> >
> > Bunch of nitpicks below, but overall I'm agreeing now with just going with
> > fb coordinate damage rects.
> >
> > Like you say, the thing needed here now is userspace + driver actually
> > implementing this. I'd also say the compat helper to map the legacy
> > fb->dirty to this new atomic way of doing things should be included here
> > (gives us a lot more testing for these new paths).
> >
> > Icing on the cake would be an igt to make sure kernel rejects invalid clip
> > rects correctly.
> >
> > > ---
> > > drivers/gpu/drm/drm_atomic.c | 42
> > +++++++++++++++++++++++++++++++++++++
> > > drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
> > > drivers/gpu/drm/drm_mode_config.c | 5 +++++
> > > drivers/gpu/drm/drm_plane.c | 12 +++++++++++
> > > include/drm/drm_mode_config.h | 15 +++++++++++++
> > > include/drm/drm_plane.h | 16 ++++++++++++++
> > > include/uapi/drm/drm_mode.h | 15 +++++++++++++
> > > 7 files changed, 109 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic.c
> > b/drivers/gpu/drm/drm_atomic.c
> > > index 7d25c42..9226d24 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct
> > drm_printer *p,
> > > }
> > >
> > > /**
> > > + * drm_atomic_set_damage_for_plane - sets the damage clips property
> > to plane
> > > + * @state: plane state
> > > + * @blob: damage clips in framebuffer coordinates
> > > + *
> > > + * Returns:
> > > + *
> > > + * Zero on success, error code on failure.
> > > + */
> > > +static int drm_atomic_set_damage_for_plane(struct drm_plane_state
> > *state,
> > > + struct drm_property_blob *blob)
> > > +{
> > > + if (blob == state->damage_clips)
> > > + return 0;
> > > +
> > > + drm_property_blob_put(state->damage_clips);
> > > + state->damage_clips = NULL;
> > > +
> > > + if (blob) {
> > > + uint32_t count = blob->length/sizeof(struct drm_rect);
> > > +
> > > + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
> > > + return -EINVAL;
> > > +
> > > + state->damage_clips = drm_property_blob_get(blob);
> > > + state->num_clips = count;
> > > + } else {
> > > + state->damage_clips = NULL;
> > > + state->num_clips = 0;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > * drm_atomic_get_plane_state - get plane state
> > > * @state: global atomic state object
> > > * @plane: plane to get state object for
> > > @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct
> > drm_plane *plane,
> > > state->color_encoding = val;
> > > } else if (property == plane->color_range_property) {
> > > state->color_range = val;
> > > + } else if (property == config->prop_damage_clips) {
> > > + struct drm_property_blob *blob =
> > > + drm_property_lookup_blob(dev, val);
> > > + int ret = drm_atomic_set_damage_for_plane(state, blob);
> >
> > There's already a helper with size-checking built-in, see
> > drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips
> > I'd
> > just provide a little inline helper that does the
> > blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).
> >
> > > + drm_property_blob_put(blob);
> > > + return ret;
> > > } else if (plane->funcs->atomic_set_property) {
> > > return plane->funcs->atomic_set_property(plane, state,
> > > property, val);
> > > @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane
> > *plane,
> > > *val = state->color_encoding;
> > > } else if (property == plane->color_range_property) {
> > > *val = state->color_range;
> > > + } else if (property == config->prop_damage_clips) {
> > > + *val = (state->damage_clips) ? state->damage_clips->base.id
> > : 0;
> > > } else if (plane->funcs->atomic_get_property) {
> > > return plane->funcs->atomic_get_property(plane, state,
> > property, val);
> > > } else {
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > > index c356545..55b44e3 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -3506,6 +3506,8 @@ void
> > __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
> > >
> > > state->fence = NULL;
> > > state->commit = NULL;
> > > + state->damage_clips = NULL;
> > > + state->num_clips = 0;
> > > }
> > > EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
> > >
> > > @@ -3550,6 +3552,8 @@ void
> > __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
> > >
> > > if (state->commit)
> > > drm_crtc_commit_put(state->commit);
> > > +
> > > + drm_property_blob_put(state->damage_clips);
> > > }
> > > EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
> > >
> > > diff --git a/drivers/gpu/drm/drm_mode_config.c
> > b/drivers/gpu/drm/drm_mode_config.c
> > > index e5c6533..e93b127 100644
> > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > @@ -293,6 +293,11 @@ static int
> > drm_mode_create_standard_properties(struct drm_device *dev)
> > > return -ENOMEM;
> > > dev->mode_config.prop_crtc_id = prop;
> > >
> > > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> > "DAMAGE_CLIPS", 0);
> >
> > Bit a bikeshed, but since the coordinates are in fb pixels, not plane
> > pixels, I'd call this "FB_DAMAGE_CLIPS".
> >
> > > + if (!prop)
> > > + return -ENOMEM;
> > > + dev->mode_config.prop_damage_clips = prop;
> > > +
> > > prop = drm_property_create_bool(dev,
> > DRM_MODE_PROP_ATOMIC,
> > > "ACTIVE");
> > > if (!prop)
> > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > index 6d2a6e4..071221b 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct
> > drm_device *dev,
> > >
> > > return ret;
> > > }
> > > +
> > > +/**
> > > + * drm_plane_enable_damage_clips - enable damage clips property
> > > + * @plane: plane on which this property to enable.
> > > + */
> > > +void drm_plane_enable_damage_clips(struct drm_plane *plane)
> > > +{
> > > + struct drm_device *dev = plane->dev;
> > > + struct drm_mode_config *config = &dev->mode_config;
> > > +
> > > + drm_object_attach_property(&plane->base, config-
> > >prop_damage_clips, 0);
> > > +}
> > > diff --git a/include/drm/drm_mode_config.h
> > b/include/drm/drm_mode_config.h
> > > index 7569f22..d8767da 100644
> > > --- a/include/drm/drm_mode_config.h
> > > +++ b/include/drm/drm_mode_config.h
> > > @@ -628,6 +628,21 @@ struct drm_mode_config {
> > > */
> > > struct drm_property *prop_crtc_id;
> > > /**
> > > + * @prop_damage_clips: Optional plane property to mark damaged
> > regions
> > > + * on the plane in framebuffer coordinates of the framebuffer
> > attached
> > > + * to the plane.
> >
> > Why should we make this optional? Looks like just another thing drivers
> > might screw up, since we have multiple callbacks and things to set up for
> > proper dirty tracking.
>
> Thanks Daniel for the review.
>
> I think not all compositor will be interested in sending damage, that was the
> reason to make this optional. Also when damage is not set that means
> user-space need full update just like eglSwapBuffersWithDamageKHR.
>
> I will add better documentation.
I think if we also handle this case in the helper that'd be even better:
In the case of no damage, the helper/core code could automatically supply
a damage rect for the entire buffer. That way drivers don't have to handle
this case specially.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Thu, Apr 05, 2018 at 11:59:57PM +0000, Deepak Singh Rawat wrote:
> > plane damage.
> >
> > On 04/05/2018 09:52 AM, Daniel Vetter wrote:
> > >
> > > TYPE_PLANE I have no idea who needs that. I suggest we just drop it.
> >
> > I'm assuming CRTC plane coordinates here. They are used for uploading
> > contents of hardware planes. Like, in the simplest case, cursor images.
>
> Yes they are CRTC plane coordinates, so is TYPE_PLANE naming confusing ?
> And should be named to TYPE_CRTC_PLANE but then it is confusing with
> TYPE_CRTC.
Yeah, I think TYPE_PLANE is really confusing, and too much aimied at your
vmwgfx special case (where the virtual hw requires that this all lines up
properly). I think providing FB coordinates, and doing the vmwgfx-specific
remapping in vmwgfx code is better.
And someone else can then figure out how to handle CRTC overall damage for
physical devices. As mentioned by me (and Rob Clark too), most hw only
allows for 1 (or maybe 2) overall damage rects, so that helper would need
to combine all the damge rects into 1. Plus take stuff like
gamma/ctm/alpha into account too. Better we leave that to someone who
needs it.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Thu, Apr 05, 2018 at 11:55:29PM +0000, Deepak Singh Rawat wrote:
> >
> > On Wed, Apr 04, 2018 at 04:49:08PM -0700, Deepak Rawat wrote:
> > > This patch adds a helper which should be called by driver which enable
> > > damage (by calling drm_plane_enable_damage_clips) from atomic_check
> > > hook. This helper for now set the damage to NULL for the planes on crtc
> > > which need full modeset.
> > >
> > > The driver also need to check for other crtc properties which can
> > > affect damage in atomic_check hook, like gamma, ctm, etc. Plane related
> > > properties which affect damage can be handled in damage iterator.
> > >
> > > Signed-off-by: Deepak Rawat <[email protected]>
> > > ---
> > > drivers/gpu/drm/drm_atomic_helper.c | 47
> > +++++++++++++++++++++++++++++++++++++
> > > include/drm/drm_atomic_helper.h | 2 ++
> > > 2 files changed, 49 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 355b514..23f44ab 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -3987,3 +3987,50 @@ drm_atomic_helper_damage_iter_next(struct
> > drm_atomic_helper_damage_iter *iter,
> > > return true;
> > > }
> > > EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
> > > +
> > > +/**
> > > + * drm_atomic_helper_check_damage - validate state object for damage
> > changes
> > > + * @dev: DRM device
> > > + * @state: the driver state object
> > > + *
> > > + * Check the state object if damage is required or not. In case damage is
> > not
> > > + * required e.g. need modeset, the damage blob is set to NULL.
> >
> > Why is that needed?
> >
> > I can imagine that drivers unconditionally upload everything for a
> > modeset, and hence don't need special damage tracking. But for that it's
> > imo better to have a clear_damage() helper.
>
> Don't we need a generic helper which all drivers can use to see if anything
> in drm_atomic_state will result in full update? My intention for calling that
> function from atomic_check hook was because state access can
> return -EDEADLK.
>
> I think for now having drm_damage_helper_clear_damage helper and
> calling it from atomic_check seems better option. In future once I have
> clear idea, a generic function can be done.
Yeah, if some of the future helpers need to e.g. allocate memory, then we
need to do any necessary prep steps from ->atomic_check.
But this isn't just prep, it clears stuff, so giving it a name that
indicates better what it does seems like a good idea to me. Just make it
clear that this should be called from ->atomic_check callbacks.
> > But even for modesets (e.g. resolution changes) I'd expect that virtual
> > drivers don't want to upload unecessary amounts of data. Manual upload
> > hw drivers probably need to upload everything, because the panel will have
> > forgotten all the old data once power cycled.
>
> AFAIK current vmwgfx will do full upload for resolution change.
>
> >
> > And if you think this is really the right thing, then we need to rename
> > the function to tell what it does, e.g.
> >
> > drm_damage_helper_clear_damage_on_modeset()
> >
> > drm_damage_helper because I think we should stuff this all into
> > drm_damage_helper.c, see previous patch.
> >
> > But I think a
> >
> > drm_damage_helper_clear_damage(crtc_state)
> >
> > which you can use in your crtc->atomic_check function like
> >
> > crtc_atomic_check(state)
> > {
> > if (drm_atomic_crtc_needs_modeset(state))
> > drm_damage_helper_clear_damage(state);
> > }
> >
> > is more flexible and useful for drivers. There might be other cases where
> > clearing damage is the right thing to do. Also, there's the question of
> > whether no damage clips == full damage or not, so maybe we should call
> > this helper full_damage() instead.
>
> In my opinion if via proper documentation it is conveyed that no damage
> means full-update the clear_damage makes sense.
Documentation is the worst documentation. Function names, or just outright
implemented behaviour is much better (e.g. runtime checks). For full
damage if there's no clip rect I think the iterator should implement that
for us.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
> > > > +void drm_plane_enable_damage_clips(struct drm_plane *plane)
> > > > +{
> > > > + struct drm_device *dev = plane->dev;
> > > > + struct drm_mode_config *config = &dev->mode_config;
> > > > +
> > > > + drm_object_attach_property(&plane->base, config-
> > > >prop_damage_clips, 0);
> > > > +}
> > > > diff --git a/include/drm/drm_mode_config.h
> > > b/include/drm/drm_mode_config.h
> > > > index 7569f22..d8767da 100644
> > > > --- a/include/drm/drm_mode_config.h
> > > > +++ b/include/drm/drm_mode_config.h
> > > > @@ -628,6 +628,21 @@ struct drm_mode_config {
> > > > */
> > > > struct drm_property *prop_crtc_id;
> > > > /**
> > > > + * @prop_damage_clips: Optional plane property to mark damaged
> > > regions
> > > > + * on the plane in framebuffer coordinates of the framebuffer
> > > attached
> > > > + * to the plane.
> > >
> > > Why should we make this optional? Looks like just another thing drivers
> > > might screw up, since we have multiple callbacks and things to set up for
> > > proper dirty tracking.
> >
> > Thanks Daniel for the review.
> >
> > I think not all compositor will be interested in sending damage, that was the
> > reason to make this optional. Also when damage is not set that means
> > user-space need full update just like eglSwapBuffersWithDamageKHR.
> >
> > I will add better documentation.
>
> I think if we also handle this case in the helper that'd be even better:
> In the case of no damage, the helper/core code could automatically supply
> a damage rect for the entire buffer. That way drivers don't have to handle
> this case specially.
> -Daniel
Agreed.
> --
On 05/04/2018 01:49, Deepak Rawat wrote:
> From: Lukasz Spintzyk <[email protected]>
>
> Optional plane property to mark damaged regions on the plane in
> framebuffer coordinates of the framebuffer attached to the plane.
>
> The layout of blob data is simply an array of drm_mode_rect with maximum
> array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
> coordinates, damage clips are not in 16.16 fixed point.
>
> Damage clips are a hint to kernel as which area of framebuffer has
> changed since last page-flip. This should be helpful for some drivers
> especially for virtual devices where each framebuffer change needs to
> be transmitted over network, usb, etc.
>
> Driver which are interested in enabling DAMAGE_CLIPS property for a
> plane should enable this property using drm_plane_enable_damage_clips.
>
> Signed-off-by: Lukasz Spintzyk <[email protected]>
> Signed-off-by: Deepak Rawat <[email protected]>
> ---
> drivers/gpu/drm/drm_atomic.c | 42 +++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
> drivers/gpu/drm/drm_mode_config.c | 5 +++++
> drivers/gpu/drm/drm_plane.c | 12 +++++++++++
> include/drm/drm_mode_config.h | 15 +++++++++++++
> include/drm/drm_plane.h | 16 ++++++++++++++
> include/uapi/drm/drm_mode.h | 15 +++++++++++++
> 7 files changed, 109 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7d25c42..9226d24 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p,
> }
>
> /**
> + * drm_atomic_set_damage_for_plane - sets the damage clips property to plane
> + * @state: plane state
> + * @blob: damage clips in framebuffer coordinates
> + *
> + * Returns:
> + *
> + * Zero on success, error code on failure.
> + */
> +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
> + struct drm_property_blob *blob)
> +{
> + if (blob == state->damage_clips)
> + return 0;
> +
> + drm_property_blob_put(state->damage_clips);
> + state->damage_clips = NULL;
> +
> + if (blob) {
> + uint32_t count = blob->length/sizeof(struct drm_rect);
> +
> + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
> + return -EINVAL;
> +
> + state->damage_clips = drm_property_blob_get(blob);
> + state->num_clips = count;
> + } else {
> + state->damage_clips = NULL;
> + state->num_clips = 0;
> + }
> +
> + return 0;
> +}
> +
> +/**
> * drm_atomic_get_plane_state - get plane state
> * @state: global atomic state object
> * @plane: plane to get state object for
> @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
> state->color_encoding = val;
> } else if (property == plane->color_range_property) {
> state->color_range = val;
> + } else if (property == config->prop_damage_clips) {
> + struct drm_property_blob *blob =
> + drm_property_lookup_blob(dev, val);
> + int ret = drm_atomic_set_damage_for_plane(state, blob);
> + drm_property_blob_put(blob);
> + return ret;
> } else if (plane->funcs->atomic_set_property) {
> return plane->funcs->atomic_set_property(plane, state,
> property, val);
> @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> *val = state->color_encoding;
> } else if (property == plane->color_range_property) {
> *val = state->color_range;
> + } else if (property == config->prop_damage_clips) {
> + *val = (state->damage_clips) ? state->damage_clips->base.id : 0;
> } else if (plane->funcs->atomic_get_property) {
> return plane->funcs->atomic_get_property(plane, state, property, val);
> } else {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index c356545..55b44e3 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>
> state->fence = NULL;
> state->commit = NULL;
> + state->damage_clips = NULL;
> + state->num_clips = 0;
> }
> EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
>
> @@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>
> if (state->commit)
> drm_crtc_commit_put(state->commit);
> +
> + drm_property_blob_put(state->damage_clips);
> }
> EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index e5c6533..e93b127 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -293,6 +293,11 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
> return -ENOMEM;
> dev->mode_config.prop_crtc_id = prop;
>
> + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, "DAMAGE_CLIPS", 0);
> + if (!prop)
> + return -ENOMEM;
> + dev->mode_config.prop_damage_clips = prop;
> +
> prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
> "ACTIVE");
> if (!prop)
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 6d2a6e4..071221b 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>
> return ret;
> }
> +
> +/**
> + * drm_plane_enable_damage_clips - enable damage clips property
> + * @plane: plane on which this property to enable.
> + */
> +void drm_plane_enable_damage_clips(struct drm_plane *plane)
> +{
> + struct drm_device *dev = plane->dev;
> + struct drm_mode_config *config = &dev->mode_config;
> +
> + drm_object_attach_property(&plane->base, config->prop_damage_clips, 0);
> +}
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 7569f22..d8767da 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -628,6 +628,21 @@ struct drm_mode_config {
> */
> struct drm_property *prop_crtc_id;
> /**
> + * @prop_damage_clips: Optional plane property to mark damaged regions
> + * on the plane in framebuffer coordinates of the framebuffer attached
> + * to the plane.
> + *
> + * The layout of blob data is simply an array of drm_mode_rect with
> + * maximum array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike
> + * plane src coordinates, damage clips are not in 16.16 fixed point.
> + *
> + * Damage clips are a hint to kernel as which area of framebuffer has
> + * changed since last page-flip. This should be helpful
> + * for some drivers especially for virtual devices where each
> + * framebuffer change needs to be transmitted over network, usb, etc.
> + */
> + struct drm_property *prop_damage_clips;
> + /**
> * @prop_active: Default atomic CRTC property to control the active
> * state, which is the simplified implementation for DPMS in atomic
> * drivers.
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index f7bf4a4..9f24548 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -146,6 +146,21 @@ struct drm_plane_state {
> */
> struct drm_crtc_commit *commit;
>
> + /*
> + * @damage_clips
> + *
> + * blob property with damage as array of drm_rect in framebuffer
> + * coodinates.
> + */
> + struct drm_property_blob *damage_clips;
> +
> + /*
> + * @num_clips
> + *
> + * Number of drm_rect in @damage_clips.
> + */
> + uint32_t num_clips;
> +
> struct drm_atomic_state *state;
> };
>
> @@ -611,6 +626,7 @@ int drm_plane_init(struct drm_device *dev,
> const uint32_t *formats, unsigned int format_count,
> bool is_primary);
> void drm_plane_cleanup(struct drm_plane *plane);
> +void drm_plane_enable_damage_clips(struct drm_plane *plane);
>
> /**
> * drm_plane_index - find the index of a registered plane
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 50bcf42..0ad0d5b 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -873,6 +873,21 @@ struct drm_mode_revoke_lease {
> __u32 lessee_id;
> };
>
> +/**
> + * struct drm_mode_rect - two dimensional rectangle drm_rect exported to
> + * user-space.
> + * @x1: horizontal starting coordinate (inclusive)
> + * @y1: vertical starting coordinate (inclusive)
> + * @x2: horizontal ending coordinate (exclusive)
> + * @y2: vertical ending coordinate (exclusive)
> + */
> +struct drm_mode_rect {
> + __s32 x1;
> + __s32 y1;
> + __s32 x2;
> + __s32 y2;
> +};
I wonder why we can't use move 'struct drm_rect'? definition from
'include/drm/drm_rect.h'
and include 'uapi/drm/drm_mode.h' in private header
'include/drm/drm_rect.h'.
Is there any general rule that disallows it?
> +
> #if defined(__cplusplus)
> }
> #endif
>Hi,
>
>Many thanks that you have picked it up.
>Unfortunately I didn't had time to work on it for a while.
>
>I am ok with what you have done, ihmo the review is going in good direction.
>I will try not to miss your update of it.
>From my side I can say that damage rects with frame-buffer coordinates are perfectly fine for DisplayLink case.
>
>Btw I have noticed that there is also a patch from Rob Clark that is supplying helper for legacy dirtyfb callback.
>Maybe we should unify the naming of the rects. Here we have "damage_clips", Clark's patch has 'dirty_rects' notion.
>On other hand existing legacy dirtyfb callback in drm_framebuffer_funcs is also using 'clip_rects' :).
The reason to name it damage is inspired by eglSwapBuffersWithDamageKHR
which user-space will be calling before submitting a page-flip. So it makes naming
consistent and in sync with user-space.
>
>Thanks
>
>Łukasz Spintzyk
>
>On 05/04/2018 01:49, Deepak Rawat wrote:
>Hi All,
>
>This is extension to Lukasz Spintzyk earlier draft of damage interface for drm.
>Bascially a new plane property is added called "DAMAGE_CLIPS" which is simply
>an array of drm_rect (exported to userspace as drm_mode_rect). The clips
>represents damage in framebuffer coordinate of attached fb to the plane.
>
>Helper iterator is added to traverse the damage rectangles and get the damage
>clips in framebuffer, plane or crtc coordinates as need by driver
>implementation. Finally a helper to reset damage in case need full update is
>required. Drivers interested in page-flip with damage should call this from
>atomic_check hook.
>
>With the RFC for atomic implementation of dirtyfb ioctl I was thinking
>should we need to consider dirty_fb flags, especially
>DRM_MODE_FB_DIRTY_ANNOTATE_COPY to be passed with atomic
>DAMAGE_CLIPS property blob? I didn't considered that untill now. If no driver
>uses that in my opinion for simplicity this can be ignored?
>
>About overlaping of damage rectangles is also not finalized. This really
>depends on driver specific implementation and can be left open-ended?
>
>My knowledge is limited to vmwgfx so would like to hear about other driver use
>cases and this can be modified in keeping other drivers need.
>
>Going forward driver implementation for vmwgfx and user-space implementation
>of kmscube/weston will be next step to test the changes.
>
>Thanks,
>Deepak
>
>Deepak Rawat (2):
>drm: Add helper iterator functions to iterate over plane damage.
>drm: Add helper to validate damage during modeset_check
>
>Lukasz Spintzyk (1):
>drm: Add DAMAGE_CLIPS property to plane
>
>drivers/gpu/drm/drm_atomic.c | 42 +++++++++
>drivers/gpu/drm/drm_atomic_helper.c | 173 ++++++++++++++++++++++++++++++++++++
>drivers/gpu/drm/drm_mode_config.c | 5 ++
>drivers/gpu/drm/drm_plane.c | 12 +++
>include/drm/drm_atomic_helper.h | 41 +++++++++
>include/drm/drm_mode_config.h | 15 ++++
>include/drm/drm_plane.h | 16 ++++
>include/uapi/drm/drm_mode.h | 15 ++++
>8 files changed, 319 insertions(+)
>
>--
>2.7.4
>