On Thu, Sep 06, 2018 at 09:44:25PM +0000, Deepak Singh Rawat wrote:
#include <drm/drm_damage_helper.h>
/** @@ -75,6 +76,11 @@
- While kernel does not error for overlapped damage clips, it is
discouraged.
*/
+static int convert_fixed_to_32(int fixed) +{
- return ((fixed >> 15) & 1) + (fixed >> 16);
+}
/**
- drm_plane_enable_fb_damage_clips - enables plane fb damage clips
property
- @plane: plane on which to enable damage clips property
@@ -90,3 +96,90 @@ void drm_plane_enable_fb_damage_clips(struct
drm_plane *plane)
0);} EXPORT_SYMBOL(drm_plane_enable_fb_damage_clips);
+/**
- drm_atomic_helper_damage_iter_init - initialize the damage iterator
- @iter: The iterator to initialize.
- @old_state: old plane state for validation.
- @new_state: plane state from which to iterate the damage clips.
- Initialize an iterator that clip framebuffer damage in plane
fb_damage_clips
- blob to plane src clip. The iterator returns full plane src in case needing
- full update e.g. during full modeset.
- With this helper iterator, drivers which enabled fb_damage_clips
property can
- iterate over the damage clips that falls inside plane src during plane
- update.
- Returns: 0 on success and negative error code on error. If an error code
is
- returned then it means the plane state shouldn't update with attached
fb.
- */
+int +drm_atomic_helper_damage_iter_init(struct
drm_atomic_helper_damage_iter *iter,
const struct drm_plane_state *old_state,const struct drm_plane_state *new_state)+{
- if (!new_state || !new_state->crtc || !new_state->fb)
return -EINVAL;- if (!new_state->visible)
return -EINVAL;Can't we handle this by iterating 0 damage rects instead? Would make the code a bit cleaner I think.
Agreed.
- memset(iter, 0, sizeof(*iter));
- iter->clips = drm_plane_get_damage_clips(new_state);
- iter->num_clips = drm_plane_get_damage_clips_count(new_state);
- if (!iter->clips)
iter->full_update = true;- if (!drm_rect_equals(&new_state->src, &old_state->src))
iter->full_update = true;- iter->plane_src.x1 = convert_fixed_to_32(new_state->src.x1);
- iter->plane_src.y1 = convert_fixed_to_32(new_state->src.y1);
- iter->plane_src.x2 = convert_fixed_to_32(new_state->src.x2);
- iter->plane_src.y2 = convert_fixed_to_32(new_state->src.y2);
I think you want to clip with the clipped rectangles here, not with the ones userspace provides.
new_state->src.x1 is the clipped one, clipped in the function drm_atomic_helper_check_plane_state. Or am I missing something ?
You're right, I misremembered. This looks correct. Would be good to explain this in the kerneldoc, and that you must call drm_atomic_helper_check_plane_state() first before calling this.
Cheers, Daniel
Also I think you're rounding is wrong here - I think you need to round down for x/y1, and round up for x/y2 to make sure you catch all the pixels?
Unit tests for this, in the form of a drm selftest (like we have for drm_mm.c already, but for kms helpers) would be perfect. Much easier to review a testcase than do bitmath in my head :-)
Sure I will work on a unit test case for this scenario and work on round up of plane_src.
- if (iter->full_update) {
iter->clips = 0;iter->curr_clip = 0;iter->num_clips = 0;- }
- 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 fb coordinate clipped to plane src.
- Returns: true if the output is valid, false if we've reached the end of
the
- rectangle list.
- */
+bool +drm_atomic_helper_damage_iter_next(struct
drm_atomic_helper_damage_iter *iter,
struct drm_rect *rect)+{
- bool ret = false;
- if (iter->full_update) {
*rect = iter->plane_src;iter->full_update = false;return true;- }
- while (iter->curr_clip < iter->num_clips) {
*rect = iter->clips[iter->curr_clip];iter->curr_clip++;if (drm_rect_intersect(rect, &iter->plane_src)) {ret = true;break;}- }
- return ret;
+} +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next); diff --git a/include/drm/drm_damage_helper.h
b/include/drm/drm_damage_helper.h
index 217694e9168c..f1282b459a4f 100644 --- a/include/drm/drm_damage_helper.h +++ b/include/drm/drm_damage_helper.h @@ -32,6 +32,19 @@ #ifndef DRM_DAMAGE_HELPER_H_ #define DRM_DAMAGE_HELPER_H_
+/**
- 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 {
- const struct drm_rect *clips;
- struct drm_rect plane_src;
- uint32_t num_clips;
- uint32_t curr_clip;
- bool full_update;
+};
/**
- drm_plane_get_damage_clips_count - returns damage clips count
- @state: Plane state
@@ -59,5 +72,12 @@ drm_plane_get_damage_clips(const struct
drm_plane_state *state)
}
void drm_plane_enable_fb_damage_clips(struct drm_plane *plane); +int +drm_atomic_helper_damage_iter_init(struct
drm_atomic_helper_damage_iter *iter,
const struct drm_plane_state *old_state,const struct drm_plane_state *new_state);I think a for_each_damage macro would be sweet on top of these here. But easy to add later on. -Daniel
+bool +drm_atomic_helper_damage_iter_next(struct
drm_atomic_helper_damage_iter *iter,
struct drm_rect *rect);#endif
2.17.1
-- Daniel Vetter Software Engineer, Intel Corporation https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ff wll.ch&data=02%7C01%7Cdrawat%40vmware.com%7C117739774a7341f d8c0208d613cd83d5%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C 636718170756943488&sdata=tNK2zTqJrq8U5U4W96wp49FZ8LAB9fl2WV UixbOSWsU%3D&reserved=0