| Message ID | 1461530942-22485-2-git-send-email-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote: > Add some utility functions for struct drm_clip_rect. > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > --- > drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++ > include/drm/drm_rect.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 136 insertions(+) > > diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c > index a8e2c86..a9fb1a8 100644 > --- a/drivers/gpu/drm/drm_rect.c > +++ b/drivers/gpu/drm/drm_rect.c > @@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r, > } > } > EXPORT_SYMBOL(drm_rect_rotate_inv); > + > +/** > + * drm_clip_rect_intersect - intersect two clip rectangles > + * @r1: first clip rectangle > + * @r2: second clip rectangle > + * > + * Calculate the intersection of clip rectangles @r1 and @r2. > + * @r1 will be overwritten with the intersection. > + * > + * RETURNS: > + * %true if rectangle @r1 is still visible after the operation, > + * %false otherwise. > + */ > +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, > + const struct drm_clip_rect *r2) > +{ > + r1->x1 = max(r1->x1, r2->x1); > + r1->y1 = max(r1->y1, r2->y1); > + r1->x2 = min(r1->x2, r2->x2); > + r1->y2 = min(r1->y2, r2->y2); > + > + return drm_clip_rect_visible(r1); > +} > +EXPORT_SYMBOL(drm_clip_rect_intersect); > + > +/** > + * drm_clip_rect_merge - Merge clip rectangles > + * @dst: destination clip rectangle > + * @src: source clip rectangle(s), can be NULL > + * @num_clips: number of source clip rectangles > + * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY) > + * @width: width of clip rectangle if @src is NULL > + * @height: height of clip rectangle if @src is NULL > + * > + * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in, > + * so if @src is NULL, width and height is used to set a full clip rectangle. > + * @dst takes part in the merge unless it is empty {0,0,0,0}. > + */ > +void drm_clip_rect_merge(struct drm_clip_rect *dst, > + struct drm_clip_rect *src, unsigned num_clips, > + unsigned flags, u32 width, u32 height) > +{ > + int i; > + > + if (!src || !num_clips) { > + dst->x1 = 0; > + dst->x2 = width; > + dst->y1 = 0; > + dst->y2 = height; > + return; > + } > + > + if (drm_clip_rect_is_empty(dst)) { > + dst->x1 = ~0; > + dst->y1 = ~0; > + } > + > + for (i = 0; i < num_clips; i++) { > + if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) > + i++; > + dst->x1 = min(dst->x1, src[i].x1); > + dst->x2 = max(dst->x2, src[i].x2); > + dst->y1 = min(dst->y1, src[i].y1); > + dst->y2 = max(dst->y2, src[i].y2); > + } > +} > +EXPORT_SYMBOL(drm_clip_rect_merge); > diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h > index 83bb156..936ad8d 100644 > --- a/include/drm/drm_rect.h > +++ b/include/drm/drm_rect.h > @@ -24,6 +24,8 @@ > #ifndef DRM_RECT_H > #define DRM_RECT_H > > +#include <uapi/drm/drm.h> > + > /** > * DOC: rect utils > * > @@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r, > int width, int height, > unsigned int rotation); > > +/** > + * drm_clip_rect_width - determine the clip rectangle width > + * @r: clip rectangle whose width is returned > + * > + * RETURNS: > + * The width of the clip rectangle. > + */ > +static inline int drm_clip_rect_width(const struct drm_clip_rect *r) > +{ > + return r->x2 - r->x1; > +} > + > +/** > + * drm_clip_rect_height - determine the clip rectangle height > + * @r: clip rectangle whose height is returned > + * > + * RETURNS: > + * The height of the clip rectangle. > + */ > +static inline int drm_clip_rect_height(const struct drm_clip_rect *r) > +{ > + return r->y2 - r->y1; > +} > + > +/** > + * drm_clip_rect_visible - determine if the the clip rectangle is visible > + * @r: clip rectangle whose visibility is returned > + * > + * RETURNS: > + * %true if the clip rectangle is visible, %false otherwise. > + */ > +static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r) > +{ > + return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0; > +} > + > +/** > + * drm_clip_rect_reset - Reset clip rectangle > + * @clip: clip rectangle > + * > + * Sets clip rectangle to {0,0,0,0}. > + */ > +static inline void drm_clip_rect_reset(struct drm_clip_rect *clip) > +{ > + clip->x1 = 0; > + clip->x2 = 0; > + clip->y1 = 0; > + clip->y2 = 0; > +} > + > +/** > + * drm_clip_rect_is_empty - Is clip rectangle empty? > + * @clip: clip rectangle > + * > + * Returns true if clip rectangle is {0,0,0,0}. > + */ > +static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip) Not sure this is a great name. At least for me empty means x1 == x2 && y1 == y2, which would be more generic than your test here. So either switch the testcase (imo preferred), or rename it to something like _is_zero? I checked the math, and besides this naming bikeshed it all looks good. So with that addressed Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > +{ > + return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2); > +} > + > +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, > + const struct drm_clip_rect *r2); > +void drm_clip_rect_merge(struct drm_clip_rect *dst, > + struct drm_clip_rect *src, unsigned num_clips, > + unsigned flags, u32 width, u32 height); > + > #endif > -- > 2.2.2 >
On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote: > Add some utility functions for struct drm_clip_rect. Looks like mostly you're just duplicating the drm_rect stuff. Why can't you use what's there already? > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > --- > drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++ > include/drm/drm_rect.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 136 insertions(+) > > diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c > index a8e2c86..a9fb1a8 100644 > --- a/drivers/gpu/drm/drm_rect.c > +++ b/drivers/gpu/drm/drm_rect.c > @@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r, > } > } > EXPORT_SYMBOL(drm_rect_rotate_inv); > + > +/** > + * drm_clip_rect_intersect - intersect two clip rectangles > + * @r1: first clip rectangle > + * @r2: second clip rectangle > + * > + * Calculate the intersection of clip rectangles @r1 and @r2. > + * @r1 will be overwritten with the intersection. > + * > + * RETURNS: > + * %true if rectangle @r1 is still visible after the operation, > + * %false otherwise. > + */ > +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, > + const struct drm_clip_rect *r2) > +{ > + r1->x1 = max(r1->x1, r2->x1); > + r1->y1 = max(r1->y1, r2->y1); > + r1->x2 = min(r1->x2, r2->x2); > + r1->y2 = min(r1->y2, r2->y2); > + > + return drm_clip_rect_visible(r1); > +} > +EXPORT_SYMBOL(drm_clip_rect_intersect); > + > +/** > + * drm_clip_rect_merge - Merge clip rectangles > + * @dst: destination clip rectangle > + * @src: source clip rectangle(s), can be NULL > + * @num_clips: number of source clip rectangles > + * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY) > + * @width: width of clip rectangle if @src is NULL > + * @height: height of clip rectangle if @src is NULL > + * > + * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in, > + * so if @src is NULL, width and height is used to set a full clip rectangle. > + * @dst takes part in the merge unless it is empty {0,0,0,0}. > + */ > +void drm_clip_rect_merge(struct drm_clip_rect *dst, > + struct drm_clip_rect *src, unsigned num_clips, > + unsigned flags, u32 width, u32 height) > +{ > + int i; > + > + if (!src || !num_clips) { > + dst->x1 = 0; > + dst->x2 = width; > + dst->y1 = 0; > + dst->y2 = height; > + return; > + } > + > + if (drm_clip_rect_is_empty(dst)) { > + dst->x1 = ~0; > + dst->y1 = ~0; > + } > + > + for (i = 0; i < num_clips; i++) { > + if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) > + i++; > + dst->x1 = min(dst->x1, src[i].x1); > + dst->x2 = max(dst->x2, src[i].x2); > + dst->y1 = min(dst->y1, src[i].y1); > + dst->y2 = max(dst->y2, src[i].y2); > + } > +} > +EXPORT_SYMBOL(drm_clip_rect_merge); > diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h > index 83bb156..936ad8d 100644 > --- a/include/drm/drm_rect.h > +++ b/include/drm/drm_rect.h > @@ -24,6 +24,8 @@ > #ifndef DRM_RECT_H > #define DRM_RECT_H > > +#include <uapi/drm/drm.h> > + > /** > * DOC: rect utils > * > @@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r, > int width, int height, > unsigned int rotation); > > +/** > + * drm_clip_rect_width - determine the clip rectangle width > + * @r: clip rectangle whose width is returned > + * > + * RETURNS: > + * The width of the clip rectangle. > + */ > +static inline int drm_clip_rect_width(const struct drm_clip_rect *r) > +{ > + return r->x2 - r->x1; > +} > + > +/** > + * drm_clip_rect_height - determine the clip rectangle height > + * @r: clip rectangle whose height is returned > + * > + * RETURNS: > + * The height of the clip rectangle. > + */ > +static inline int drm_clip_rect_height(const struct drm_clip_rect *r) > +{ > + return r->y2 - r->y1; > +} > + > +/** > + * drm_clip_rect_visible - determine if the the clip rectangle is visible > + * @r: clip rectangle whose visibility is returned > + * > + * RETURNS: > + * %true if the clip rectangle is visible, %false otherwise. > + */ > +static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r) > +{ > + return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0; > +} > + > +/** > + * drm_clip_rect_reset - Reset clip rectangle > + * @clip: clip rectangle > + * > + * Sets clip rectangle to {0,0,0,0}. > + */ > +static inline void drm_clip_rect_reset(struct drm_clip_rect *clip) > +{ > + clip->x1 = 0; > + clip->x2 = 0; > + clip->y1 = 0; > + clip->y2 = 0; > +} > + > +/** > + * drm_clip_rect_is_empty - Is clip rectangle empty? > + * @clip: clip rectangle > + * > + * Returns true if clip rectangle is {0,0,0,0}. > + */ > +static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip) > +{ > + return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2); > +} > + > +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, > + const struct drm_clip_rect *r2); > +void drm_clip_rect_merge(struct drm_clip_rect *dst, > + struct drm_clip_rect *src, unsigned num_clips, > + unsigned flags, u32 width, u32 height); > + > #endif > -- > 2.2.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Den 25.04.2016 14:39, skrev Ville Syrjälä: > On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote: >> Add some utility functions for struct drm_clip_rect. > Looks like mostly you're just duplicating the drm_rect stuff. Why can't > you use what's there already? That's because the framebuffer flushing uses drm_clip_rect and not drm_rect: struct drm_framebuffer_funcs { [...] int (*dirty)(struct drm_framebuffer *framebuffer, struct drm_file *file_priv, unsigned flags, unsigned color, struct drm_clip_rect *clips, unsigned num_clips); }; >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >> --- >> drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++ >> include/drm/drm_rect.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 136 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c >> index a8e2c86..a9fb1a8 100644 >> --- a/drivers/gpu/drm/drm_rect.c >> +++ b/drivers/gpu/drm/drm_rect.c >> @@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r, >> } >> } >> EXPORT_SYMBOL(drm_rect_rotate_inv); >> + >> +/** >> + * drm_clip_rect_intersect - intersect two clip rectangles >> + * @r1: first clip rectangle >> + * @r2: second clip rectangle >> + * >> + * Calculate the intersection of clip rectangles @r1 and @r2. >> + * @r1 will be overwritten with the intersection. >> + * >> + * RETURNS: >> + * %true if rectangle @r1 is still visible after the operation, >> + * %false otherwise. >> + */ >> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, >> + const struct drm_clip_rect *r2) >> +{ >> + r1->x1 = max(r1->x1, r2->x1); >> + r1->y1 = max(r1->y1, r2->y1); >> + r1->x2 = min(r1->x2, r2->x2); >> + r1->y2 = min(r1->y2, r2->y2); >> + >> + return drm_clip_rect_visible(r1); >> +} >> +EXPORT_SYMBOL(drm_clip_rect_intersect); >> + >> +/** >> + * drm_clip_rect_merge - Merge clip rectangles >> + * @dst: destination clip rectangle >> + * @src: source clip rectangle(s), can be NULL >> + * @num_clips: number of source clip rectangles >> + * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY) >> + * @width: width of clip rectangle if @src is NULL >> + * @height: height of clip rectangle if @src is NULL >> + * >> + * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in, >> + * so if @src is NULL, width and height is used to set a full clip rectangle. >> + * @dst takes part in the merge unless it is empty {0,0,0,0}. >> + */ >> +void drm_clip_rect_merge(struct drm_clip_rect *dst, >> + struct drm_clip_rect *src, unsigned num_clips, >> + unsigned flags, u32 width, u32 height) >> +{ >> + int i; >> + >> + if (!src || !num_clips) { >> + dst->x1 = 0; >> + dst->x2 = width; >> + dst->y1 = 0; >> + dst->y2 = height; >> + return; >> + } >> + >> + if (drm_clip_rect_is_empty(dst)) { >> + dst->x1 = ~0; >> + dst->y1 = ~0; >> + } >> + >> + for (i = 0; i < num_clips; i++) { >> + if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) >> + i++; >> + dst->x1 = min(dst->x1, src[i].x1); >> + dst->x2 = max(dst->x2, src[i].x2); >> + dst->y1 = min(dst->y1, src[i].y1); >> + dst->y2 = max(dst->y2, src[i].y2); >> + } >> +} >> +EXPORT_SYMBOL(drm_clip_rect_merge); >> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h >> index 83bb156..936ad8d 100644 >> --- a/include/drm/drm_rect.h >> +++ b/include/drm/drm_rect.h >> @@ -24,6 +24,8 @@ >> #ifndef DRM_RECT_H >> #define DRM_RECT_H >> >> +#include <uapi/drm/drm.h> >> + >> /** >> * DOC: rect utils >> * >> @@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r, >> int width, int height, >> unsigned int rotation); >> >> +/** >> + * drm_clip_rect_width - determine the clip rectangle width >> + * @r: clip rectangle whose width is returned >> + * >> + * RETURNS: >> + * The width of the clip rectangle. >> + */ >> +static inline int drm_clip_rect_width(const struct drm_clip_rect *r) >> +{ >> + return r->x2 - r->x1; >> +} >> + >> +/** >> + * drm_clip_rect_height - determine the clip rectangle height >> + * @r: clip rectangle whose height is returned >> + * >> + * RETURNS: >> + * The height of the clip rectangle. >> + */ >> +static inline int drm_clip_rect_height(const struct drm_clip_rect *r) >> +{ >> + return r->y2 - r->y1; >> +} >> + >> +/** >> + * drm_clip_rect_visible - determine if the the clip rectangle is visible >> + * @r: clip rectangle whose visibility is returned >> + * >> + * RETURNS: >> + * %true if the clip rectangle is visible, %false otherwise. >> + */ >> +static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r) >> +{ >> + return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0; >> +} >> + >> +/** >> + * drm_clip_rect_reset - Reset clip rectangle >> + * @clip: clip rectangle >> + * >> + * Sets clip rectangle to {0,0,0,0}. >> + */ >> +static inline void drm_clip_rect_reset(struct drm_clip_rect *clip) >> +{ >> + clip->x1 = 0; >> + clip->x2 = 0; >> + clip->y1 = 0; >> + clip->y2 = 0; >> +} >> + >> +/** >> + * drm_clip_rect_is_empty - Is clip rectangle empty? >> + * @clip: clip rectangle >> + * >> + * Returns true if clip rectangle is {0,0,0,0}. >> + */ >> +static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip) >> +{ >> + return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2); >> +} >> + >> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, >> + const struct drm_clip_rect *r2); >> +void drm_clip_rect_merge(struct drm_clip_rect *dst, >> + struct drm_clip_rect *src, unsigned num_clips, >> + unsigned flags, u32 width, u32 height); >> + >> #endif >> -- >> 2.2.2 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Apr 25, 2016 at 02:55:52PM +0200, Noralf Trønnes wrote: > > Den 25.04.2016 14:39, skrev Ville Syrjälä: > > On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote: > >> Add some utility functions for struct drm_clip_rect. > > Looks like mostly you're just duplicating the drm_rect stuff. Why can't > > you use what's there already? > > That's because the framebuffer flushing uses drm_clip_rect and not drm_rect: Converting to drm_rect is not an option? > > struct drm_framebuffer_funcs { > [...] > int (*dirty)(struct drm_framebuffer *framebuffer, > struct drm_file *file_priv, unsigned flags, > unsigned color, struct drm_clip_rect *clips, > unsigned num_clips); > }; > > >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > >> --- > >> drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++ > >> include/drm/drm_rect.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++ > >> 2 files changed, 136 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c > >> index a8e2c86..a9fb1a8 100644 > >> --- a/drivers/gpu/drm/drm_rect.c > >> +++ b/drivers/gpu/drm/drm_rect.c > >> @@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r, > >> } > >> } > >> EXPORT_SYMBOL(drm_rect_rotate_inv); > >> + > >> +/** > >> + * drm_clip_rect_intersect - intersect two clip rectangles > >> + * @r1: first clip rectangle > >> + * @r2: second clip rectangle > >> + * > >> + * Calculate the intersection of clip rectangles @r1 and @r2. > >> + * @r1 will be overwritten with the intersection. > >> + * > >> + * RETURNS: > >> + * %true if rectangle @r1 is still visible after the operation, > >> + * %false otherwise. > >> + */ > >> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, > >> + const struct drm_clip_rect *r2) > >> +{ > >> + r1->x1 = max(r1->x1, r2->x1); > >> + r1->y1 = max(r1->y1, r2->y1); > >> + r1->x2 = min(r1->x2, r2->x2); > >> + r1->y2 = min(r1->y2, r2->y2); > >> + > >> + return drm_clip_rect_visible(r1); > >> +} > >> +EXPORT_SYMBOL(drm_clip_rect_intersect); > >> + > >> +/** > >> + * drm_clip_rect_merge - Merge clip rectangles > >> + * @dst: destination clip rectangle > >> + * @src: source clip rectangle(s), can be NULL > >> + * @num_clips: number of source clip rectangles > >> + * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY) > >> + * @width: width of clip rectangle if @src is NULL > >> + * @height: height of clip rectangle if @src is NULL > >> + * > >> + * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in, > >> + * so if @src is NULL, width and height is used to set a full clip rectangle. > >> + * @dst takes part in the merge unless it is empty {0,0,0,0}. > >> + */ > >> +void drm_clip_rect_merge(struct drm_clip_rect *dst, > >> + struct drm_clip_rect *src, unsigned num_clips, > >> + unsigned flags, u32 width, u32 height) > >> +{ > >> + int i; > >> + > >> + if (!src || !num_clips) { > >> + dst->x1 = 0; > >> + dst->x2 = width; > >> + dst->y1 = 0; > >> + dst->y2 = height; > >> + return; > >> + } > >> + > >> + if (drm_clip_rect_is_empty(dst)) { > >> + dst->x1 = ~0; > >> + dst->y1 = ~0; > >> + } > >> + > >> + for (i = 0; i < num_clips; i++) { > >> + if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) > >> + i++; > >> + dst->x1 = min(dst->x1, src[i].x1); > >> + dst->x2 = max(dst->x2, src[i].x2); > >> + dst->y1 = min(dst->y1, src[i].y1); > >> + dst->y2 = max(dst->y2, src[i].y2); > >> + } > >> +} > >> +EXPORT_SYMBOL(drm_clip_rect_merge); > >> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h > >> index 83bb156..936ad8d 100644 > >> --- a/include/drm/drm_rect.h > >> +++ b/include/drm/drm_rect.h > >> @@ -24,6 +24,8 @@ > >> #ifndef DRM_RECT_H > >> #define DRM_RECT_H > >> > >> +#include <uapi/drm/drm.h> > >> + > >> /** > >> * DOC: rect utils > >> * > >> @@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r, > >> int width, int height, > >> unsigned int rotation); > >> > >> +/** > >> + * drm_clip_rect_width - determine the clip rectangle width > >> + * @r: clip rectangle whose width is returned > >> + * > >> + * RETURNS: > >> + * The width of the clip rectangle. > >> + */ > >> +static inline int drm_clip_rect_width(const struct drm_clip_rect *r) > >> +{ > >> + return r->x2 - r->x1; > >> +} > >> + > >> +/** > >> + * drm_clip_rect_height - determine the clip rectangle height > >> + * @r: clip rectangle whose height is returned > >> + * > >> + * RETURNS: > >> + * The height of the clip rectangle. > >> + */ > >> +static inline int drm_clip_rect_height(const struct drm_clip_rect *r) > >> +{ > >> + return r->y2 - r->y1; > >> +} > >> + > >> +/** > >> + * drm_clip_rect_visible - determine if the the clip rectangle is visible > >> + * @r: clip rectangle whose visibility is returned > >> + * > >> + * RETURNS: > >> + * %true if the clip rectangle is visible, %false otherwise. > >> + */ > >> +static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r) > >> +{ > >> + return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0; > >> +} > >> + > >> +/** > >> + * drm_clip_rect_reset - Reset clip rectangle > >> + * @clip: clip rectangle > >> + * > >> + * Sets clip rectangle to {0,0,0,0}. > >> + */ > >> +static inline void drm_clip_rect_reset(struct drm_clip_rect *clip) > >> +{ > >> + clip->x1 = 0; > >> + clip->x2 = 0; > >> + clip->y1 = 0; > >> + clip->y2 = 0; > >> +} > >> + > >> +/** > >> + * drm_clip_rect_is_empty - Is clip rectangle empty? > >> + * @clip: clip rectangle > >> + * > >> + * Returns true if clip rectangle is {0,0,0,0}. > >> + */ > >> +static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip) > >> +{ > >> + return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2); > >> +} > >> + > >> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, > >> + const struct drm_clip_rect *r2); > >> +void drm_clip_rect_merge(struct drm_clip_rect *dst, > >> + struct drm_clip_rect *src, unsigned num_clips, > >> + unsigned flags, u32 width, u32 height); > >> + > >> #endif > >> -- > >> 2.2.2 > >> > >> _______________________________________________ > >> dri-devel mailing list > >> dri-devel@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Den 25.04.2016 15:02, skrev Ville Syrjälä: > On Mon, Apr 25, 2016 at 02:55:52PM +0200, Noralf Trønnes wrote: >> Den 25.04.2016 14:39, skrev Ville Syrjälä: >>> On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote: >>>> Add some utility functions for struct drm_clip_rect. >>> Looks like mostly you're just duplicating the drm_rect stuff. Why can't >>> you use what's there already? >> That's because the framebuffer flushing uses drm_clip_rect and not drm_rect: > Converting to drm_rect is not an option? That's difficult or at least verbose to do because clips is an array. I could use drm_rect on the calling side (fbdev) since it's only one clip which the changes are merged into, and then convert it when I call dirty(). But the driver can get zero or more clips from the dirty ioctl so I don't see a clean way to convert this array to drm_rect without more code than this proposal has. Here's the driver side: static int mipi_dbi_dirtyfb(struct drm_framebuffer *fb, void *vmem, unsigned flags, unsigned color, struct drm_clip_rect *clips, unsigned num_clips) { struct tinydrm_device *tdev = fb->dev->dev_private; struct lcdreg *reg = tdev->lcdreg; struct drm_clip_rect full_clip = { .x1 = 0, .x2 = fb->width, .y1 = 0, .y2 = fb->height, }; struct drm_clip_rect clip; int ret; drm_clip_rect_reset(&clip); drm_clip_rect_merge(&clip, clips, num_clips, flags, fb->width, fb->height); if (!drm_clip_rect_intersect(&clip, &full_clip)) { DRM_DEBUG_KMS("Empty clip\n"); return -EINVAL; } [...] >> struct drm_framebuffer_funcs { >> [...] >> int (*dirty)(struct drm_framebuffer *framebuffer, >> struct drm_file *file_priv, unsigned flags, >> unsigned color, struct drm_clip_rect *clips, >> unsigned num_clips); >> }; >> >>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >>>> --- >>>> drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++ >>>> include/drm/drm_rect.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 136 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c >>>> index a8e2c86..a9fb1a8 100644 >>>> --- a/drivers/gpu/drm/drm_rect.c >>>> +++ b/drivers/gpu/drm/drm_rect.c >>>> @@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r, >>>> } >>>> } >>>> EXPORT_SYMBOL(drm_rect_rotate_inv); >>>> + >>>> +/** >>>> + * drm_clip_rect_intersect - intersect two clip rectangles >>>> + * @r1: first clip rectangle >>>> + * @r2: second clip rectangle >>>> + * >>>> + * Calculate the intersection of clip rectangles @r1 and @r2. >>>> + * @r1 will be overwritten with the intersection. >>>> + * >>>> + * RETURNS: >>>> + * %true if rectangle @r1 is still visible after the operation, >>>> + * %false otherwise. >>>> + */ >>>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, >>>> + const struct drm_clip_rect *r2) >>>> +{ >>>> + r1->x1 = max(r1->x1, r2->x1); >>>> + r1->y1 = max(r1->y1, r2->y1); >>>> + r1->x2 = min(r1->x2, r2->x2); >>>> + r1->y2 = min(r1->y2, r2->y2); >>>> + >>>> + return drm_clip_rect_visible(r1); >>>> +} >>>> +EXPORT_SYMBOL(drm_clip_rect_intersect); >>>> + >>>> +/** >>>> + * drm_clip_rect_merge - Merge clip rectangles >>>> + * @dst: destination clip rectangle >>>> + * @src: source clip rectangle(s), can be NULL >>>> + * @num_clips: number of source clip rectangles >>>> + * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY) >>>> + * @width: width of clip rectangle if @src is NULL >>>> + * @height: height of clip rectangle if @src is NULL >>>> + * >>>> + * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in, >>>> + * so if @src is NULL, width and height is used to set a full clip rectangle. >>>> + * @dst takes part in the merge unless it is empty {0,0,0,0}. >>>> + */ >>>> +void drm_clip_rect_merge(struct drm_clip_rect *dst, >>>> + struct drm_clip_rect *src, unsigned num_clips, >>>> + unsigned flags, u32 width, u32 height) >>>> +{ >>>> + int i; >>>> + >>>> + if (!src || !num_clips) { >>>> + dst->x1 = 0; >>>> + dst->x2 = width; >>>> + dst->y1 = 0; >>>> + dst->y2 = height; >>>> + return; >>>> + } >>>> + >>>> + if (drm_clip_rect_is_empty(dst)) { >>>> + dst->x1 = ~0; >>>> + dst->y1 = ~0; >>>> + } >>>> + >>>> + for (i = 0; i < num_clips; i++) { >>>> + if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) >>>> + i++; >>>> + dst->x1 = min(dst->x1, src[i].x1); >>>> + dst->x2 = max(dst->x2, src[i].x2); >>>> + dst->y1 = min(dst->y1, src[i].y1); >>>> + dst->y2 = max(dst->y2, src[i].y2); >>>> + } >>>> +} >>>> +EXPORT_SYMBOL(drm_clip_rect_merge); >>>> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h >>>> index 83bb156..936ad8d 100644 >>>> --- a/include/drm/drm_rect.h >>>> +++ b/include/drm/drm_rect.h >>>> @@ -24,6 +24,8 @@ >>>> #ifndef DRM_RECT_H >>>> #define DRM_RECT_H >>>> >>>> +#include <uapi/drm/drm.h> >>>> + >>>> /** >>>> * DOC: rect utils >>>> * >>>> @@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r, >>>> int width, int height, >>>> unsigned int rotation); >>>> >>>> +/** >>>> + * drm_clip_rect_width - determine the clip rectangle width >>>> + * @r: clip rectangle whose width is returned >>>> + * >>>> + * RETURNS: >>>> + * The width of the clip rectangle. >>>> + */ >>>> +static inline int drm_clip_rect_width(const struct drm_clip_rect *r) >>>> +{ >>>> + return r->x2 - r->x1; >>>> +} >>>> + >>>> +/** >>>> + * drm_clip_rect_height - determine the clip rectangle height >>>> + * @r: clip rectangle whose height is returned >>>> + * >>>> + * RETURNS: >>>> + * The height of the clip rectangle. >>>> + */ >>>> +static inline int drm_clip_rect_height(const struct drm_clip_rect *r) >>>> +{ >>>> + return r->y2 - r->y1; >>>> +} >>>> + >>>> +/** >>>> + * drm_clip_rect_visible - determine if the the clip rectangle is visible >>>> + * @r: clip rectangle whose visibility is returned >>>> + * >>>> + * RETURNS: >>>> + * %true if the clip rectangle is visible, %false otherwise. >>>> + */ >>>> +static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r) >>>> +{ >>>> + return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0; >>>> +} >>>> + >>>> +/** >>>> + * drm_clip_rect_reset - Reset clip rectangle >>>> + * @clip: clip rectangle >>>> + * >>>> + * Sets clip rectangle to {0,0,0,0}. >>>> + */ >>>> +static inline void drm_clip_rect_reset(struct drm_clip_rect *clip) >>>> +{ >>>> + clip->x1 = 0; >>>> + clip->x2 = 0; >>>> + clip->y1 = 0; >>>> + clip->y2 = 0; >>>> +} >>>> + >>>> +/** >>>> + * drm_clip_rect_is_empty - Is clip rectangle empty? >>>> + * @clip: clip rectangle >>>> + * >>>> + * Returns true if clip rectangle is {0,0,0,0}. >>>> + */ >>>> +static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip) >>>> +{ >>>> + return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2); >>>> +} >>>> + >>>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, >>>> + const struct drm_clip_rect *r2); >>>> +void drm_clip_rect_merge(struct drm_clip_rect *dst, >>>> + struct drm_clip_rect *src, unsigned num_clips, >>>> + unsigned flags, u32 width, u32 height); >>>> + >>>> #endif >>>> -- >>>> 2.2.2 >>>> >>>> _______________________________________________ >>>> dri-devel mailing list >>>> dri-devel@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Apr 25, 2016 at 04:03:13PM +0200, Noralf Trønnes wrote: > > Den 25.04.2016 15:02, skrev Ville Syrjälä: > > On Mon, Apr 25, 2016 at 02:55:52PM +0200, Noralf Trønnes wrote: > >> Den 25.04.2016 14:39, skrev Ville Syrjälä: > >>> On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote: > >>>> Add some utility functions for struct drm_clip_rect. > >>> Looks like mostly you're just duplicating the drm_rect stuff. Why can't > >>> you use what's there already? > >> That's because the framebuffer flushing uses drm_clip_rect and not drm_rect: > > Converting to drm_rect is not an option? > > That's difficult or at least verbose to do because clips is an array. > I could use drm_rect on the calling side (fbdev) since it's only one clip > which the changes are merged into, and then convert it when I call dirty(). > But the driver can get zero or more clips from the dirty ioctl so I don't > see a clean way to convert this array to drm_rect without more code than > this proposal has. Just some kind of simple drm_clip_rect_to_rect() thing should be enough AFAICS. > > Here's the driver side: > > static int mipi_dbi_dirtyfb(struct drm_framebuffer *fb, void *vmem, > unsigned flags, unsigned color, > struct drm_clip_rect *clips, unsigned num_clips) > { > struct tinydrm_device *tdev = fb->dev->dev_private; > struct lcdreg *reg = tdev->lcdreg; > struct drm_clip_rect full_clip = { > .x1 = 0, > .x2 = fb->width, > .y1 = 0, > .y2 = fb->height, > }; > struct drm_clip_rect clip; > int ret; > > drm_clip_rect_reset(&clip); > drm_clip_rect_merge(&clip, clips, num_clips, flags, > fb->width, fb->height); > if (!drm_clip_rect_intersect(&clip, &full_clip)) { > DRM_DEBUG_KMS("Empty clip\n"); > return -EINVAL; > } > [...] > > >> struct drm_framebuffer_funcs { > >> [...] > >> int (*dirty)(struct drm_framebuffer *framebuffer, > >> struct drm_file *file_priv, unsigned flags, > >> unsigned color, struct drm_clip_rect *clips, > >> unsigned num_clips); > >> }; > >> > >>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > >>>> --- > >>>> drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++ > >>>> include/drm/drm_rect.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++ > >>>> 2 files changed, 136 insertions(+) > >>>> > >>>> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c > >>>> index a8e2c86..a9fb1a8 100644 > >>>> --- a/drivers/gpu/drm/drm_rect.c > >>>> +++ b/drivers/gpu/drm/drm_rect.c > >>>> @@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r, > >>>> } > >>>> } > >>>> EXPORT_SYMBOL(drm_rect_rotate_inv); > >>>> + > >>>> +/** > >>>> + * drm_clip_rect_intersect - intersect two clip rectangles > >>>> + * @r1: first clip rectangle > >>>> + * @r2: second clip rectangle > >>>> + * > >>>> + * Calculate the intersection of clip rectangles @r1 and @r2. > >>>> + * @r1 will be overwritten with the intersection. > >>>> + * > >>>> + * RETURNS: > >>>> + * %true if rectangle @r1 is still visible after the operation, > >>>> + * %false otherwise. > >>>> + */ > >>>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, > >>>> + const struct drm_clip_rect *r2) > >>>> +{ > >>>> + r1->x1 = max(r1->x1, r2->x1); > >>>> + r1->y1 = max(r1->y1, r2->y1); > >>>> + r1->x2 = min(r1->x2, r2->x2); > >>>> + r1->y2 = min(r1->y2, r2->y2); > >>>> + > >>>> + return drm_clip_rect_visible(r1); > >>>> +} > >>>> +EXPORT_SYMBOL(drm_clip_rect_intersect); > >>>> + > >>>> +/** > >>>> + * drm_clip_rect_merge - Merge clip rectangles > >>>> + * @dst: destination clip rectangle > >>>> + * @src: source clip rectangle(s), can be NULL > >>>> + * @num_clips: number of source clip rectangles > >>>> + * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY) > >>>> + * @width: width of clip rectangle if @src is NULL > >>>> + * @height: height of clip rectangle if @src is NULL > >>>> + * > >>>> + * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in, > >>>> + * so if @src is NULL, width and height is used to set a full clip rectangle. > >>>> + * @dst takes part in the merge unless it is empty {0,0,0,0}. > >>>> + */ > >>>> +void drm_clip_rect_merge(struct drm_clip_rect *dst, > >>>> + struct drm_clip_rect *src, unsigned num_clips, > >>>> + unsigned flags, u32 width, u32 height) > >>>> +{ > >>>> + int i; > >>>> + > >>>> + if (!src || !num_clips) { > >>>> + dst->x1 = 0; > >>>> + dst->x2 = width; > >>>> + dst->y1 = 0; > >>>> + dst->y2 = height; > >>>> + return; > >>>> + } > >>>> + > >>>> + if (drm_clip_rect_is_empty(dst)) { > >>>> + dst->x1 = ~0; > >>>> + dst->y1 = ~0; > >>>> + } > >>>> + > >>>> + for (i = 0; i < num_clips; i++) { > >>>> + if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) > >>>> + i++; > >>>> + dst->x1 = min(dst->x1, src[i].x1); > >>>> + dst->x2 = max(dst->x2, src[i].x2); > >>>> + dst->y1 = min(dst->y1, src[i].y1); > >>>> + dst->y2 = max(dst->y2, src[i].y2); > >>>> + } > >>>> +} > >>>> +EXPORT_SYMBOL(drm_clip_rect_merge); > >>>> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h > >>>> index 83bb156..936ad8d 100644 > >>>> --- a/include/drm/drm_rect.h > >>>> +++ b/include/drm/drm_rect.h > >>>> @@ -24,6 +24,8 @@ > >>>> #ifndef DRM_RECT_H > >>>> #define DRM_RECT_H > >>>> > >>>> +#include <uapi/drm/drm.h> > >>>> + > >>>> /** > >>>> * DOC: rect utils > >>>> * > >>>> @@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r, > >>>> int width, int height, > >>>> unsigned int rotation); > >>>> > >>>> +/** > >>>> + * drm_clip_rect_width - determine the clip rectangle width > >>>> + * @r: clip rectangle whose width is returned > >>>> + * > >>>> + * RETURNS: > >>>> + * The width of the clip rectangle. > >>>> + */ > >>>> +static inline int drm_clip_rect_width(const struct drm_clip_rect *r) > >>>> +{ > >>>> + return r->x2 - r->x1; > >>>> +} > >>>> + > >>>> +/** > >>>> + * drm_clip_rect_height - determine the clip rectangle height > >>>> + * @r: clip rectangle whose height is returned > >>>> + * > >>>> + * RETURNS: > >>>> + * The height of the clip rectangle. > >>>> + */ > >>>> +static inline int drm_clip_rect_height(const struct drm_clip_rect *r) > >>>> +{ > >>>> + return r->y2 - r->y1; > >>>> +} > >>>> + > >>>> +/** > >>>> + * drm_clip_rect_visible - determine if the the clip rectangle is visible > >>>> + * @r: clip rectangle whose visibility is returned > >>>> + * > >>>> + * RETURNS: > >>>> + * %true if the clip rectangle is visible, %false otherwise. > >>>> + */ > >>>> +static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r) > >>>> +{ > >>>> + return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0; > >>>> +} > >>>> + > >>>> +/** > >>>> + * drm_clip_rect_reset - Reset clip rectangle > >>>> + * @clip: clip rectangle > >>>> + * > >>>> + * Sets clip rectangle to {0,0,0,0}. > >>>> + */ > >>>> +static inline void drm_clip_rect_reset(struct drm_clip_rect *clip) > >>>> +{ > >>>> + clip->x1 = 0; > >>>> + clip->x2 = 0; > >>>> + clip->y1 = 0; > >>>> + clip->y2 = 0; > >>>> +} > >>>> + > >>>> +/** > >>>> + * drm_clip_rect_is_empty - Is clip rectangle empty? > >>>> + * @clip: clip rectangle > >>>> + * > >>>> + * Returns true if clip rectangle is {0,0,0,0}. > >>>> + */ > >>>> +static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip) > >>>> +{ > >>>> + return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2); > >>>> +} > >>>> + > >>>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, > >>>> + const struct drm_clip_rect *r2); > >>>> +void drm_clip_rect_merge(struct drm_clip_rect *dst, > >>>> + struct drm_clip_rect *src, unsigned num_clips, > >>>> + unsigned flags, u32 width, u32 height); > >>>> + > >>>> #endif > >>>> -- > >>>> 2.2.2 > >>>> > >>>> _______________________________________________ > >>>> dri-devel mailing list > >>>> dri-devel@lists.freedesktop.org > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Apr 25, 2016 at 06:09:44PM +0300, Ville Syrjälä wrote: > On Mon, Apr 25, 2016 at 04:03:13PM +0200, Noralf Trønnes wrote: > > > > Den 25.04.2016 15:02, skrev Ville Syrjälä: > > > On Mon, Apr 25, 2016 at 02:55:52PM +0200, Noralf Trønnes wrote: > > >> Den 25.04.2016 14:39, skrev Ville Syrjälä: > > >>> On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote: > > >>>> Add some utility functions for struct drm_clip_rect. > > >>> Looks like mostly you're just duplicating the drm_rect stuff. Why can't > > >>> you use what's there already? > > >> That's because the framebuffer flushing uses drm_clip_rect and not drm_rect: > > > Converting to drm_rect is not an option? > > > > That's difficult or at least verbose to do because clips is an array. > > I could use drm_rect on the calling side (fbdev) since it's only one clip > > which the changes are merged into, and then convert it when I call dirty(). > > But the driver can get zero or more clips from the dirty ioctl so I don't > > see a clean way to convert this array to drm_rect without more code than > > this proposal has. > > Just some kind of simple drm_clip_rect_to_rect() thing should be enough AFAICS. Yeah, drm_clip_rect is the uapi struct, drm_rect is the internal one. Similar case is drm_display_mode vs. drm_mode_modeinfo. We have umode_to_mode and mode_to_umode helpers to deal with that. I do agree that it would make sense to switch the internal ->dirty callback over to the internal drm_struct. Would need a kmalloc+copy in the dirtyfb ioctl, but since the structs actually match in their member names (just not the size/signedness, sigh) there shouldn't be any need for driver changes. So fairly simple patch. Ofc you need to compile-test all the drivers (at least those using ->dirty hook) to make sure gcc is still happy with all the signed vs. unsigned stuff. Maybe that turns up something, but hopefully not. Sorry for that late request, but I really didn't realize what's going on here :( -Daniel > > > > > Here's the driver side: > > > > static int mipi_dbi_dirtyfb(struct drm_framebuffer *fb, void *vmem, > > unsigned flags, unsigned color, > > struct drm_clip_rect *clips, unsigned num_clips) > > { > > struct tinydrm_device *tdev = fb->dev->dev_private; > > struct lcdreg *reg = tdev->lcdreg; > > struct drm_clip_rect full_clip = { > > .x1 = 0, > > .x2 = fb->width, > > .y1 = 0, > > .y2 = fb->height, > > }; > > struct drm_clip_rect clip; > > int ret; > > > > drm_clip_rect_reset(&clip); > > drm_clip_rect_merge(&clip, clips, num_clips, flags, > > fb->width, fb->height); > > if (!drm_clip_rect_intersect(&clip, &full_clip)) { > > DRM_DEBUG_KMS("Empty clip\n"); > > return -EINVAL; > > } > > [...] > > > > > >> struct drm_framebuffer_funcs { > > >> [...] > > >> int (*dirty)(struct drm_framebuffer *framebuffer, > > >> struct drm_file *file_priv, unsigned flags, > > >> unsigned color, struct drm_clip_rect *clips, > > >> unsigned num_clips); > > >> }; > > >> > > >>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > > >>>> --- > > >>>> drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++ > > >>>> include/drm/drm_rect.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++ > > >>>> 2 files changed, 136 insertions(+) > > >>>> > > >>>> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c > > >>>> index a8e2c86..a9fb1a8 100644 > > >>>> --- a/drivers/gpu/drm/drm_rect.c > > >>>> +++ b/drivers/gpu/drm/drm_rect.c > > >>>> @@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r, > > >>>> } > > >>>> } > > >>>> EXPORT_SYMBOL(drm_rect_rotate_inv); > > >>>> + > > >>>> +/** > > >>>> + * drm_clip_rect_intersect - intersect two clip rectangles > > >>>> + * @r1: first clip rectangle > > >>>> + * @r2: second clip rectangle > > >>>> + * > > >>>> + * Calculate the intersection of clip rectangles @r1 and @r2. > > >>>> + * @r1 will be overwritten with the intersection. > > >>>> + * > > >>>> + * RETURNS: > > >>>> + * %true if rectangle @r1 is still visible after the operation, > > >>>> + * %false otherwise. > > >>>> + */ > > >>>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, > > >>>> + const struct drm_clip_rect *r2) > > >>>> +{ > > >>>> + r1->x1 = max(r1->x1, r2->x1); > > >>>> + r1->y1 = max(r1->y1, r2->y1); > > >>>> + r1->x2 = min(r1->x2, r2->x2); > > >>>> + r1->y2 = min(r1->y2, r2->y2); > > >>>> + > > >>>> + return drm_clip_rect_visible(r1); > > >>>> +} > > >>>> +EXPORT_SYMBOL(drm_clip_rect_intersect); > > >>>> + > > >>>> +/** > > >>>> + * drm_clip_rect_merge - Merge clip rectangles > > >>>> + * @dst: destination clip rectangle > > >>>> + * @src: source clip rectangle(s), can be NULL > > >>>> + * @num_clips: number of source clip rectangles > > >>>> + * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY) > > >>>> + * @width: width of clip rectangle if @src is NULL > > >>>> + * @height: height of clip rectangle if @src is NULL > > >>>> + * > > >>>> + * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in, > > >>>> + * so if @src is NULL, width and height is used to set a full clip rectangle. > > >>>> + * @dst takes part in the merge unless it is empty {0,0,0,0}. > > >>>> + */ > > >>>> +void drm_clip_rect_merge(struct drm_clip_rect *dst, > > >>>> + struct drm_clip_rect *src, unsigned num_clips, > > >>>> + unsigned flags, u32 width, u32 height) > > >>>> +{ > > >>>> + int i; > > >>>> + > > >>>> + if (!src || !num_clips) { > > >>>> + dst->x1 = 0; > > >>>> + dst->x2 = width; > > >>>> + dst->y1 = 0; > > >>>> + dst->y2 = height; > > >>>> + return; > > >>>> + } > > >>>> + > > >>>> + if (drm_clip_rect_is_empty(dst)) { > > >>>> + dst->x1 = ~0; > > >>>> + dst->y1 = ~0; > > >>>> + } > > >>>> + > > >>>> + for (i = 0; i < num_clips; i++) { > > >>>> + if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) > > >>>> + i++; > > >>>> + dst->x1 = min(dst->x1, src[i].x1); > > >>>> + dst->x2 = max(dst->x2, src[i].x2); > > >>>> + dst->y1 = min(dst->y1, src[i].y1); > > >>>> + dst->y2 = max(dst->y2, src[i].y2); > > >>>> + } > > >>>> +} > > >>>> +EXPORT_SYMBOL(drm_clip_rect_merge); > > >>>> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h > > >>>> index 83bb156..936ad8d 100644 > > >>>> --- a/include/drm/drm_rect.h > > >>>> +++ b/include/drm/drm_rect.h > > >>>> @@ -24,6 +24,8 @@ > > >>>> #ifndef DRM_RECT_H > > >>>> #define DRM_RECT_H > > >>>> > > >>>> +#include <uapi/drm/drm.h> > > >>>> + > > >>>> /** > > >>>> * DOC: rect utils > > >>>> * > > >>>> @@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r, > > >>>> int width, int height, > > >>>> unsigned int rotation); > > >>>> > > >>>> +/** > > >>>> + * drm_clip_rect_width - determine the clip rectangle width > > >>>> + * @r: clip rectangle whose width is returned > > >>>> + * > > >>>> + * RETURNS: > > >>>> + * The width of the clip rectangle. > > >>>> + */ > > >>>> +static inline int drm_clip_rect_width(const struct drm_clip_rect *r) > > >>>> +{ > > >>>> + return r->x2 - r->x1; > > >>>> +} > > >>>> + > > >>>> +/** > > >>>> + * drm_clip_rect_height - determine the clip rectangle height > > >>>> + * @r: clip rectangle whose height is returned > > >>>> + * > > >>>> + * RETURNS: > > >>>> + * The height of the clip rectangle. > > >>>> + */ > > >>>> +static inline int drm_clip_rect_height(const struct drm_clip_rect *r) > > >>>> +{ > > >>>> + return r->y2 - r->y1; > > >>>> +} > > >>>> + > > >>>> +/** > > >>>> + * drm_clip_rect_visible - determine if the the clip rectangle is visible > > >>>> + * @r: clip rectangle whose visibility is returned > > >>>> + * > > >>>> + * RETURNS: > > >>>> + * %true if the clip rectangle is visible, %false otherwise. > > >>>> + */ > > >>>> +static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r) > > >>>> +{ > > >>>> + return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0; > > >>>> +} > > >>>> + > > >>>> +/** > > >>>> + * drm_clip_rect_reset - Reset clip rectangle > > >>>> + * @clip: clip rectangle > > >>>> + * > > >>>> + * Sets clip rectangle to {0,0,0,0}. > > >>>> + */ > > >>>> +static inline void drm_clip_rect_reset(struct drm_clip_rect *clip) > > >>>> +{ > > >>>> + clip->x1 = 0; > > >>>> + clip->x2 = 0; > > >>>> + clip->y1 = 0; > > >>>> + clip->y2 = 0; > > >>>> +} > > >>>> + > > >>>> +/** > > >>>> + * drm_clip_rect_is_empty - Is clip rectangle empty? > > >>>> + * @clip: clip rectangle > > >>>> + * > > >>>> + * Returns true if clip rectangle is {0,0,0,0}. > > >>>> + */ > > >>>> +static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip) > > >>>> +{ > > >>>> + return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2); > > >>>> +} > > >>>> + > > >>>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, > > >>>> + const struct drm_clip_rect *r2); > > >>>> +void drm_clip_rect_merge(struct drm_clip_rect *dst, > > >>>> + struct drm_clip_rect *src, unsigned num_clips, > > >>>> + unsigned flags, u32 width, u32 height); > > >>>> + > > >>>> #endif > > >>>> -- > > >>>> 2.2.2 > > >>>> > > >>>> _______________________________________________ > > >>>> dri-devel mailing list > > >>>> dri-devel@lists.freedesktop.org > > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Apr 25, 2016 at 06:05:20PM +0200, Daniel Vetter wrote: > On Mon, Apr 25, 2016 at 06:09:44PM +0300, Ville Syrjälä wrote: > > On Mon, Apr 25, 2016 at 04:03:13PM +0200, Noralf Trønnes wrote: > > > > > > Den 25.04.2016 15:02, skrev Ville Syrjälä: > > > > On Mon, Apr 25, 2016 at 02:55:52PM +0200, Noralf Trønnes wrote: > > > >> Den 25.04.2016 14:39, skrev Ville Syrjälä: > > > >>> On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote: > > > >>>> Add some utility functions for struct drm_clip_rect. > > > >>> Looks like mostly you're just duplicating the drm_rect stuff. Why can't > > > >>> you use what's there already? > > > >> That's because the framebuffer flushing uses drm_clip_rect and not drm_rect: > > > > Converting to drm_rect is not an option? > > > > > > That's difficult or at least verbose to do because clips is an array. > > > I could use drm_rect on the calling side (fbdev) since it's only one clip > > > which the changes are merged into, and then convert it when I call dirty(). > > > But the driver can get zero or more clips from the dirty ioctl so I don't > > > see a clean way to convert this array to drm_rect without more code than > > > this proposal has. > > > > Just some kind of simple drm_clip_rect_to_rect() thing should be enough AFAICS. > > Yeah, drm_clip_rect is the uapi struct, drm_rect is the internal one. > Similar case is drm_display_mode vs. drm_mode_modeinfo. We have > umode_to_mode and mode_to_umode helpers to deal with that. I do agree that > it would make sense to switch the internal ->dirty callback over to the > internal drm_struct. Would need a kmalloc+copy in the dirtyfb ioctl, but > since the structs actually match in their member names (just not the > size/signedness, sigh) there shouldn't be any need for driver changes. So > fairly simple patch. Or if we want to avoid the malloc, then the merge() thing could just internally convert one at a time on stack when going through them. Though then someone might want to do a merge() with internal drm_rects, and we'd be right where we started. But I'm not sure that will happen, so perhaps it's just too much future proofing. > > Ofc you need to compile-test all the drivers (at least those using ->dirty > hook) to make sure gcc is still happy with all the signed vs. unsigned > stuff. Maybe that turns up something, but hopefully not. > > Sorry for that late request, but I really didn't realize what's going on > here :( > -Daniel > > > > > > > > > Here's the driver side: > > > > > > static int mipi_dbi_dirtyfb(struct drm_framebuffer *fb, void *vmem, > > > unsigned flags, unsigned color, > > > struct drm_clip_rect *clips, unsigned num_clips) > > > { > > > struct tinydrm_device *tdev = fb->dev->dev_private; > > > struct lcdreg *reg = tdev->lcdreg; > > > struct drm_clip_rect full_clip = { > > > .x1 = 0, > > > .x2 = fb->width, > > > .y1 = 0, > > > .y2 = fb->height, > > > }; > > > struct drm_clip_rect clip; > > > int ret; > > > > > > drm_clip_rect_reset(&clip); > > > drm_clip_rect_merge(&clip, clips, num_clips, flags, > > > fb->width, fb->height); > > > if (!drm_clip_rect_intersect(&clip, &full_clip)) { > > > DRM_DEBUG_KMS("Empty clip\n"); > > > return -EINVAL; > > > } > > > [...] > > > > > > > > >> struct drm_framebuffer_funcs { > > > >> [...] > > > >> int (*dirty)(struct drm_framebuffer *framebuffer, > > > >> struct drm_file *file_priv, unsigned flags, > > > >> unsigned color, struct drm_clip_rect *clips, > > > >> unsigned num_clips); > > > >> }; > > > >> > > > >>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > > > >>>> --- > > > >>>> drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++ > > > >>>> include/drm/drm_rect.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++ > > > >>>> 2 files changed, 136 insertions(+) > > > >>>> > > > >>>> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c > > > >>>> index a8e2c86..a9fb1a8 100644 > > > >>>> --- a/drivers/gpu/drm/drm_rect.c > > > >>>> +++ b/drivers/gpu/drm/drm_rect.c > > > >>>> @@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r, > > > >>>> } > > > >>>> } > > > >>>> EXPORT_SYMBOL(drm_rect_rotate_inv); > > > >>>> + > > > >>>> +/** > > > >>>> + * drm_clip_rect_intersect - intersect two clip rectangles > > > >>>> + * @r1: first clip rectangle > > > >>>> + * @r2: second clip rectangle > > > >>>> + * > > > >>>> + * Calculate the intersection of clip rectangles @r1 and @r2. > > > >>>> + * @r1 will be overwritten with the intersection. > > > >>>> + * > > > >>>> + * RETURNS: > > > >>>> + * %true if rectangle @r1 is still visible after the operation, > > > >>>> + * %false otherwise. > > > >>>> + */ > > > >>>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, > > > >>>> + const struct drm_clip_rect *r2) > > > >>>> +{ > > > >>>> + r1->x1 = max(r1->x1, r2->x1); > > > >>>> + r1->y1 = max(r1->y1, r2->y1); > > > >>>> + r1->x2 = min(r1->x2, r2->x2); > > > >>>> + r1->y2 = min(r1->y2, r2->y2); > > > >>>> + > > > >>>> + return drm_clip_rect_visible(r1); > > > >>>> +} > > > >>>> +EXPORT_SYMBOL(drm_clip_rect_intersect); > > > >>>> + > > > >>>> +/** > > > >>>> + * drm_clip_rect_merge - Merge clip rectangles > > > >>>> + * @dst: destination clip rectangle > > > >>>> + * @src: source clip rectangle(s), can be NULL > > > >>>> + * @num_clips: number of source clip rectangles > > > >>>> + * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY) > > > >>>> + * @width: width of clip rectangle if @src is NULL > > > >>>> + * @height: height of clip rectangle if @src is NULL > > > >>>> + * > > > >>>> + * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in, > > > >>>> + * so if @src is NULL, width and height is used to set a full clip rectangle. > > > >>>> + * @dst takes part in the merge unless it is empty {0,0,0,0}. > > > >>>> + */ > > > >>>> +void drm_clip_rect_merge(struct drm_clip_rect *dst, > > > >>>> + struct drm_clip_rect *src, unsigned num_clips, > > > >>>> + unsigned flags, u32 width, u32 height) > > > >>>> +{ > > > >>>> + int i; > > > >>>> + > > > >>>> + if (!src || !num_clips) { > > > >>>> + dst->x1 = 0; > > > >>>> + dst->x2 = width; > > > >>>> + dst->y1 = 0; > > > >>>> + dst->y2 = height; > > > >>>> + return; > > > >>>> + } > > > >>>> + > > > >>>> + if (drm_clip_rect_is_empty(dst)) { > > > >>>> + dst->x1 = ~0; > > > >>>> + dst->y1 = ~0; > > > >>>> + } > > > >>>> + > > > >>>> + for (i = 0; i < num_clips; i++) { > > > >>>> + if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) > > > >>>> + i++; > > > >>>> + dst->x1 = min(dst->x1, src[i].x1); > > > >>>> + dst->x2 = max(dst->x2, src[i].x2); > > > >>>> + dst->y1 = min(dst->y1, src[i].y1); > > > >>>> + dst->y2 = max(dst->y2, src[i].y2); > > > >>>> + } > > > >>>> +} > > > >>>> +EXPORT_SYMBOL(drm_clip_rect_merge); > > > >>>> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h > > > >>>> index 83bb156..936ad8d 100644 > > > >>>> --- a/include/drm/drm_rect.h > > > >>>> +++ b/include/drm/drm_rect.h > > > >>>> @@ -24,6 +24,8 @@ > > > >>>> #ifndef DRM_RECT_H > > > >>>> #define DRM_RECT_H > > > >>>> > > > >>>> +#include <uapi/drm/drm.h> > > > >>>> + > > > >>>> /** > > > >>>> * DOC: rect utils > > > >>>> * > > > >>>> @@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r, > > > >>>> int width, int height, > > > >>>> unsigned int rotation); > > > >>>> > > > >>>> +/** > > > >>>> + * drm_clip_rect_width - determine the clip rectangle width > > > >>>> + * @r: clip rectangle whose width is returned > > > >>>> + * > > > >>>> + * RETURNS: > > > >>>> + * The width of the clip rectangle. > > > >>>> + */ > > > >>>> +static inline int drm_clip_rect_width(const struct drm_clip_rect *r) > > > >>>> +{ > > > >>>> + return r->x2 - r->x1; > > > >>>> +} > > > >>>> + > > > >>>> +/** > > > >>>> + * drm_clip_rect_height - determine the clip rectangle height > > > >>>> + * @r: clip rectangle whose height is returned > > > >>>> + * > > > >>>> + * RETURNS: > > > >>>> + * The height of the clip rectangle. > > > >>>> + */ > > > >>>> +static inline int drm_clip_rect_height(const struct drm_clip_rect *r) > > > >>>> +{ > > > >>>> + return r->y2 - r->y1; > > > >>>> +} > > > >>>> + > > > >>>> +/** > > > >>>> + * drm_clip_rect_visible - determine if the the clip rectangle is visible > > > >>>> + * @r: clip rectangle whose visibility is returned > > > >>>> + * > > > >>>> + * RETURNS: > > > >>>> + * %true if the clip rectangle is visible, %false otherwise. > > > >>>> + */ > > > >>>> +static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r) > > > >>>> +{ > > > >>>> + return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0; > > > >>>> +} > > > >>>> + > > > >>>> +/** > > > >>>> + * drm_clip_rect_reset - Reset clip rectangle > > > >>>> + * @clip: clip rectangle > > > >>>> + * > > > >>>> + * Sets clip rectangle to {0,0,0,0}. > > > >>>> + */ > > > >>>> +static inline void drm_clip_rect_reset(struct drm_clip_rect *clip) > > > >>>> +{ > > > >>>> + clip->x1 = 0; > > > >>>> + clip->x2 = 0; > > > >>>> + clip->y1 = 0; > > > >>>> + clip->y2 = 0; > > > >>>> +} > > > >>>> + > > > >>>> +/** > > > >>>> + * drm_clip_rect_is_empty - Is clip rectangle empty? > > > >>>> + * @clip: clip rectangle > > > >>>> + * > > > >>>> + * Returns true if clip rectangle is {0,0,0,0}. > > > >>>> + */ > > > >>>> +static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip) > > > >>>> +{ > > > >>>> + return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2); > > > >>>> +} > > > >>>> + > > > >>>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, > > > >>>> + const struct drm_clip_rect *r2); > > > >>>> +void drm_clip_rect_merge(struct drm_clip_rect *dst, > > > >>>> + struct drm_clip_rect *src, unsigned num_clips, > > > >>>> + unsigned flags, u32 width, u32 height); > > > >>>> + > > > >>>> #endif > > > >>>> -- > > > >>>> 2.2.2 > > > >>>> > > > >>>> _______________________________________________ > > > >>>> dri-devel mailing list > > > >>>> dri-devel@lists.freedesktop.org > > > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > > Ville Syrjälä > > Intel OTC > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
Den 25.04.2016 18:38, skrev Ville Syrjälä: > On Mon, Apr 25, 2016 at 06:05:20PM +0200, Daniel Vetter wrote: >> On Mon, Apr 25, 2016 at 06:09:44PM +0300, Ville Syrjälä wrote: >>> On Mon, Apr 25, 2016 at 04:03:13PM +0200, Noralf Trønnes wrote: >>>> Den 25.04.2016 15:02, skrev Ville Syrjälä: >>>>> On Mon, Apr 25, 2016 at 02:55:52PM +0200, Noralf Trønnes wrote: >>>>>> Den 25.04.2016 14:39, skrev Ville Syrjälä: >>>>>>> On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote: >>>>>>>> Add some utility functions for struct drm_clip_rect. >>>>>>> Looks like mostly you're just duplicating the drm_rect stuff. Why can't >>>>>>> you use what's there already? >>>>>> That's because the framebuffer flushing uses drm_clip_rect and not drm_rect: >>>>> Converting to drm_rect is not an option? >>>> That's difficult or at least verbose to do because clips is an array. >>>> I could use drm_rect on the calling side (fbdev) since it's only one clip >>>> which the changes are merged into, and then convert it when I call dirty(). >>>> But the driver can get zero or more clips from the dirty ioctl so I don't >>>> see a clean way to convert this array to drm_rect without more code than >>>> this proposal has. >>> Just some kind of simple drm_clip_rect_to_rect() thing should be enough AFAICS. >> Yeah, drm_clip_rect is the uapi struct, drm_rect is the internal one. >> Similar case is drm_display_mode vs. drm_mode_modeinfo. We have >> umode_to_mode and mode_to_umode helpers to deal with that. I do agree that >> it would make sense to switch the internal ->dirty callback over to the >> internal drm_struct. Would need a kmalloc+copy in the dirtyfb ioctl, but >> since the structs actually match in their member names (just not the >> size/signedness, sigh) there shouldn't be any need for driver changes. So >> fairly simple patch. > Or if we want to avoid the malloc, then the merge() thing could just > internally convert one at a time on stack when going through them. > Though then someone might want to do a merge() with internal drm_rects, > and we'd be right where we started. But I'm not sure that will happen, > so perhaps it's just too much future proofing. > >> Ofc you need to compile-test all the drivers (at least those using ->dirty >> hook) to make sure gcc is still happy with all the signed vs. unsigned >> stuff. Maybe that turns up something, but hopefully not. >> >> Sorry for that late request, but I really didn't realize what's going on >> here :( >> -Daniel How about we just drop this patch? I couldn't find anyone else that merge these clips, they just loop and handle them individually. The relevant part in drm_fb_helper would become: static void drm_fb_helper_dirty_work(struct work_struct *work) { struct drm_fb_helper *helper = container_of(work, struct drm_fb_helper, dirty_work); struct drm_clip_rect *clip = &helper->dirty_clip; struct drm_clip_rect clip_copy; unsigned long flags; spin_lock_irqsave(&helper->dirty_lock, flags); clip_copy = *clip; clip->x1 = clip->y1 = ~0; clip->x2 = clip->y2 = 0; spin_unlock_irqrestore(&helper->dirty_lock, flags); helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1); } static void drm_fb_helper_dirty_init(struct drm_fb_helper *helper) { spin_lock_init(&helper->dirty_lock); INIT_WORK(&helper->dirty_work, drm_fb_helper_dirty_work); helper->dirty_clip.x1 = helper->dirty_clip.y1 = ~0; } static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y, u32 width, u32 height) { struct drm_fb_helper *helper = info->par; struct drm_clip_rect *clip = &helper->dirty_clip; unsigned long flags; if (!helper->fb->funcs->dirty) return; spin_lock_irqsave(&helper->dirty_lock, flags); clip->x1 = min(clip->x1, x); clip->y1 = min(clip->y1, y); clip->x2 = max(clip->x2, x + width); clip->y2 = max(clip->y2, y + height); spin_unlock_irqrestore(&helper->dirty_lock, flags); schedule_work(&helper->dirty_work); } And the driver would use this tinydrm function: void tinydrm_merge_clips(struct drm_clip_rect *dst, struct drm_clip_rect *src, unsigned num_clips, unsigned flags, u32 width, u32 height) { int i; if (!src || !num_clips) { dst->x1 = 0; dst->x2 = width; dst->y1 = 0; dst->y2 = height; return; } dst->x1 = dst->y1 = ~0; dst->x2 = dst->y2 = 0; for (i = 0; i < num_clips; i++) { if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) i++; dst->x1 = min(dst->x1, src[i].x1); dst->x2 = max(dst->x2, src[i].x2); dst->y1 = min(dst->y1, src[i].y1); dst->y2 = max(dst->y2, src[i].y2); } if (dst->x2 > width || dst->y2 > height || dst->x1 >= dst->x2 || dst->y1 >= dst->y2) { DRM_DEBUG_KMS("Illegal clip: x1=%u, x2=%u, y1=%u, y2=%u\n", dst->x1, dst->x2, dst->y1, dst->y2); dst->x1 = dst->y1 = 0; dst->x2 = width; dst->y2 = height; } } static int mipi_dbi_dirtyfb(struct drm_framebuffer *fb, void *vmem, unsigned flags, unsigned color, struct drm_clip_rect *clips, unsigned num_clips) { struct drm_clip_rect clip; tinydrm_merge_clips(&clip, clips, num_clips, flags, fb->width, fb->height); >>>> Here's the driver side: >>>> >>>> static int mipi_dbi_dirtyfb(struct drm_framebuffer *fb, void *vmem, >>>> unsigned flags, unsigned color, >>>> struct drm_clip_rect *clips, unsigned num_clips) >>>> { >>>> struct tinydrm_device *tdev = fb->dev->dev_private; >>>> struct lcdreg *reg = tdev->lcdreg; >>>> struct drm_clip_rect full_clip = { >>>> .x1 = 0, >>>> .x2 = fb->width, >>>> .y1 = 0, >>>> .y2 = fb->height, >>>> }; >>>> struct drm_clip_rect clip; >>>> int ret; >>>> >>>> drm_clip_rect_reset(&clip); >>>> drm_clip_rect_merge(&clip, clips, num_clips, flags, >>>> fb->width, fb->height); >>>> if (!drm_clip_rect_intersect(&clip, &full_clip)) { >>>> DRM_DEBUG_KMS("Empty clip\n"); >>>> return -EINVAL; >>>> } >>>> [...] >>>>>> struct drm_framebuffer_funcs { >>>>>> [...] >>>>>> int (*dirty)(struct drm_framebuffer *framebuffer, >>>>>> struct drm_file *file_priv, unsigned flags, >>>>>> unsigned color, struct drm_clip_rect *clips, >>>>>> unsigned num_clips); >>>>>> }; >>>>>> >>>>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++ >>>>>>>> include/drm/drm_rect.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>>> 2 files changed, 136 insertions(+) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c >>>>>>>> index a8e2c86..a9fb1a8 100644 >>>>>>>> --- a/drivers/gpu/drm/drm_rect.c >>>>>>>> +++ b/drivers/gpu/drm/drm_rect.c >>>>>>>> @@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r, >>>>>>>> } >>>>>>>> } >>>>>>>> EXPORT_SYMBOL(drm_rect_rotate_inv); >>>>>>>> + >>>>>>>> +/** >>>>>>>> + * drm_clip_rect_intersect - intersect two clip rectangles >>>>>>>> + * @r1: first clip rectangle >>>>>>>> + * @r2: second clip rectangle >>>>>>>> + * >>>>>>>> + * Calculate the intersection of clip rectangles @r1 and @r2. >>>>>>>> + * @r1 will be overwritten with the intersection. >>>>>>>> + * >>>>>>>> + * RETURNS: >>>>>>>> + * %true if rectangle @r1 is still visible after the operation, >>>>>>>> + * %false otherwise. >>>>>>>> + */ >>>>>>>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, >>>>>>>> + const struct drm_clip_rect *r2) >>>>>>>> +{ >>>>>>>> + r1->x1 = max(r1->x1, r2->x1); >>>>>>>> + r1->y1 = max(r1->y1, r2->y1); >>>>>>>> + r1->x2 = min(r1->x2, r2->x2); >>>>>>>> + r1->y2 = min(r1->y2, r2->y2); >>>>>>>> + >>>>>>>> + return drm_clip_rect_visible(r1); >>>>>>>> +} >>>>>>>> +EXPORT_SYMBOL(drm_clip_rect_intersect); >>>>>>>> + >>>>>>>> +/** >>>>>>>> + * drm_clip_rect_merge - Merge clip rectangles >>>>>>>> + * @dst: destination clip rectangle >>>>>>>> + * @src: source clip rectangle(s), can be NULL >>>>>>>> + * @num_clips: number of source clip rectangles >>>>>>>> + * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY) >>>>>>>> + * @width: width of clip rectangle if @src is NULL >>>>>>>> + * @height: height of clip rectangle if @src is NULL >>>>>>>> + * >>>>>>>> + * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in, >>>>>>>> + * so if @src is NULL, width and height is used to set a full clip rectangle. >>>>>>>> + * @dst takes part in the merge unless it is empty {0,0,0,0}. >>>>>>>> + */ >>>>>>>> +void drm_clip_rect_merge(struct drm_clip_rect *dst, >>>>>>>> + struct drm_clip_rect *src, unsigned num_clips, >>>>>>>> + unsigned flags, u32 width, u32 height) >>>>>>>> +{ >>>>>>>> + int i; >>>>>>>> + >>>>>>>> + if (!src || !num_clips) { >>>>>>>> + dst->x1 = 0; >>>>>>>> + dst->x2 = width; >>>>>>>> + dst->y1 = 0; >>>>>>>> + dst->y2 = height; >>>>>>>> + return; >>>>>>>> + } >>>>>>>> + >>>>>>>> + if (drm_clip_rect_is_empty(dst)) { >>>>>>>> + dst->x1 = ~0; >>>>>>>> + dst->y1 = ~0; >>>>>>>> + } >>>>>>>> + >>>>>>>> + for (i = 0; i < num_clips; i++) { >>>>>>>> + if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) >>>>>>>> + i++; >>>>>>>> + dst->x1 = min(dst->x1, src[i].x1); >>>>>>>> + dst->x2 = max(dst->x2, src[i].x2); >>>>>>>> + dst->y1 = min(dst->y1, src[i].y1); >>>>>>>> + dst->y2 = max(dst->y2, src[i].y2); >>>>>>>> + } >>>>>>>> +} >>>>>>>> +EXPORT_SYMBOL(drm_clip_rect_merge); >>>>>>>> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h >>>>>>>> index 83bb156..936ad8d 100644 >>>>>>>> --- a/include/drm/drm_rect.h >>>>>>>> +++ b/include/drm/drm_rect.h >>>>>>>> @@ -24,6 +24,8 @@ >>>>>>>> #ifndef DRM_RECT_H >>>>>>>> #define DRM_RECT_H >>>>>>>> >>>>>>>> +#include <uapi/drm/drm.h> >>>>>>>> + >>>>>>>> /** >>>>>>>> * DOC: rect utils >>>>>>>> * >>>>>>>> @@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r, >>>>>>>> int width, int height, >>>>>>>> unsigned int rotation); >>>>>>>> >>>>>>>> +/** >>>>>>>> + * drm_clip_rect_width - determine the clip rectangle width >>>>>>>> + * @r: clip rectangle whose width is returned >>>>>>>> + * >>>>>>>> + * RETURNS: >>>>>>>> + * The width of the clip rectangle. >>>>>>>> + */ >>>>>>>> +static inline int drm_clip_rect_width(const struct drm_clip_rect *r) >>>>>>>> +{ >>>>>>>> + return r->x2 - r->x1; >>>>>>>> +} >>>>>>>> + >>>>>>>> +/** >>>>>>>> + * drm_clip_rect_height - determine the clip rectangle height >>>>>>>> + * @r: clip rectangle whose height is returned >>>>>>>> + * >>>>>>>> + * RETURNS: >>>>>>>> + * The height of the clip rectangle. >>>>>>>> + */ >>>>>>>> +static inline int drm_clip_rect_height(const struct drm_clip_rect *r) >>>>>>>> +{ >>>>>>>> + return r->y2 - r->y1; >>>>>>>> +} >>>>>>>> + >>>>>>>> +/** >>>>>>>> + * drm_clip_rect_visible - determine if the the clip rectangle is visible >>>>>>>> + * @r: clip rectangle whose visibility is returned >>>>>>>> + * >>>>>>>> + * RETURNS: >>>>>>>> + * %true if the clip rectangle is visible, %false otherwise. >>>>>>>> + */ >>>>>>>> +static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r) >>>>>>>> +{ >>>>>>>> + return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0; >>>>>>>> +} >>>>>>>> + >>>>>>>> +/** >>>>>>>> + * drm_clip_rect_reset - Reset clip rectangle >>>>>>>> + * @clip: clip rectangle >>>>>>>> + * >>>>>>>> + * Sets clip rectangle to {0,0,0,0}. >>>>>>>> + */ >>>>>>>> +static inline void drm_clip_rect_reset(struct drm_clip_rect *clip) >>>>>>>> +{ >>>>>>>> + clip->x1 = 0; >>>>>>>> + clip->x2 = 0; >>>>>>>> + clip->y1 = 0; >>>>>>>> + clip->y2 = 0; >>>>>>>> +} >>>>>>>> + >>>>>>>> +/** >>>>>>>> + * drm_clip_rect_is_empty - Is clip rectangle empty? >>>>>>>> + * @clip: clip rectangle >>>>>>>> + * >>>>>>>> + * Returns true if clip rectangle is {0,0,0,0}. >>>>>>>> + */ >>>>>>>> +static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip) >>>>>>>> +{ >>>>>>>> + return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2); >>>>>>>> +} >>>>>>>> + >>>>>>>> +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, >>>>>>>> + const struct drm_clip_rect *r2); >>>>>>>> +void drm_clip_rect_merge(struct drm_clip_rect *dst, >>>>>>>> + struct drm_clip_rect *src, unsigned num_clips, >>>>>>>> + unsigned flags, u32 width, u32 height); >>>>>>>> + >>>>>>>> #endif >>>>>>>> -- >>>>>>>> 2.2.2 >>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> dri-devel mailing list >>>>>>>> dri-devel@lists.freedesktop.org >>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>> -- >>> Ville Syrjälä >>> Intel OTC >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> http://blog.ffwll.ch
On Mon, Apr 25, 2016 at 08:35:18PM +0200, Noralf Trønnes wrote: > > Den 25.04.2016 18:38, skrev Ville Syrjälä: > >On Mon, Apr 25, 2016 at 06:05:20PM +0200, Daniel Vetter wrote: > >>On Mon, Apr 25, 2016 at 06:09:44PM +0300, Ville Syrjälä wrote: > >>>On Mon, Apr 25, 2016 at 04:03:13PM +0200, Noralf Trønnes wrote: > >>>>Den 25.04.2016 15:02, skrev Ville Syrjälä: > >>>>>On Mon, Apr 25, 2016 at 02:55:52PM +0200, Noralf Trønnes wrote: > >>>>>>Den 25.04.2016 14:39, skrev Ville Syrjälä: > >>>>>>>On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote: > >>>>>>>>Add some utility functions for struct drm_clip_rect. > >>>>>>>Looks like mostly you're just duplicating the drm_rect stuff. Why can't > >>>>>>>you use what's there already? > >>>>>>That's because the framebuffer flushing uses drm_clip_rect and not drm_rect: > >>>>>Converting to drm_rect is not an option? > >>>>That's difficult or at least verbose to do because clips is an array. > >>>>I could use drm_rect on the calling side (fbdev) since it's only one clip > >>>>which the changes are merged into, and then convert it when I call dirty(). > >>>>But the driver can get zero or more clips from the dirty ioctl so I don't > >>>>see a clean way to convert this array to drm_rect without more code than > >>>>this proposal has. > >>>Just some kind of simple drm_clip_rect_to_rect() thing should be enough AFAICS. > >>Yeah, drm_clip_rect is the uapi struct, drm_rect is the internal one. > >>Similar case is drm_display_mode vs. drm_mode_modeinfo. We have > >>umode_to_mode and mode_to_umode helpers to deal with that. I do agree that > >>it would make sense to switch the internal ->dirty callback over to the > >>internal drm_struct. Would need a kmalloc+copy in the dirtyfb ioctl, but > >>since the structs actually match in their member names (just not the > >>size/signedness, sigh) there shouldn't be any need for driver changes. So > >>fairly simple patch. > >Or if we want to avoid the malloc, then the merge() thing could just > >internally convert one at a time on stack when going through them. > >Though then someone might want to do a merge() with internal drm_rects, > >and we'd be right where we started. But I'm not sure that will happen, > >so perhaps it's just too much future proofing. > > > >>Ofc you need to compile-test all the drivers (at least those using ->dirty > >>hook) to make sure gcc is still happy with all the signed vs. unsigned > >>stuff. Maybe that turns up something, but hopefully not. > >> > >>Sorry for that late request, but I really didn't realize what's going on > >>here :( > >>-Daniel > > How about we just drop this patch? > I couldn't find anyone else that merge these clips, they just loop and > handle them individually. > > The relevant part in drm_fb_helper would become: > > static void drm_fb_helper_dirty_work(struct work_struct *work) > { > struct drm_fb_helper *helper = container_of(work, struct drm_fb_helper, > dirty_work); > struct drm_clip_rect *clip = &helper->dirty_clip; > struct drm_clip_rect clip_copy; > unsigned long flags; > > spin_lock_irqsave(&helper->dirty_lock, flags); > clip_copy = *clip; > clip->x1 = clip->y1 = ~0; > clip->x2 = clip->y2 = 0; > spin_unlock_irqrestore(&helper->dirty_lock, flags); > > helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1); > } > > static void drm_fb_helper_dirty_init(struct drm_fb_helper *helper) > { > spin_lock_init(&helper->dirty_lock); > INIT_WORK(&helper->dirty_work, drm_fb_helper_dirty_work); > helper->dirty_clip.x1 = helper->dirty_clip.y1 = ~0; > } > > static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y, > u32 width, u32 height) > { > struct drm_fb_helper *helper = info->par; > struct drm_clip_rect *clip = &helper->dirty_clip; > unsigned long flags; > > if (!helper->fb->funcs->dirty) > return; > > spin_lock_irqsave(&helper->dirty_lock, flags); > clip->x1 = min(clip->x1, x); > clip->y1 = min(clip->y1, y); > clip->x2 = max(clip->x2, x + width); > clip->y2 = max(clip->y2, y + height); > spin_unlock_irqrestore(&helper->dirty_lock, flags); > > schedule_work(&helper->dirty_work); > } > > > And the driver would use this tinydrm function: > > void tinydrm_merge_clips(struct drm_clip_rect *dst, > struct drm_clip_rect *src, unsigned num_clips, > unsigned flags, u32 width, u32 height) > { > int i; > > if (!src || !num_clips) { > dst->x1 = 0; > dst->x2 = width; > dst->y1 = 0; > dst->y2 = height; > return; > } > > dst->x1 = dst->y1 = ~0; > dst->x2 = dst->y2 = 0; > > for (i = 0; i < num_clips; i++) { > if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) > i++; > dst->x1 = min(dst->x1, src[i].x1); > dst->x2 = max(dst->x2, src[i].x2); > dst->y1 = min(dst->y1, src[i].y1); > dst->y2 = max(dst->y2, src[i].y2); > } > > if (dst->x2 > width || dst->y2 > height || > dst->x1 >= dst->x2 || dst->y1 >= dst->y2) { > DRM_DEBUG_KMS("Illegal clip: x1=%u, x2=%u, y1=%u, y2=%u\n", > dst->x1, dst->x2, dst->y1, dst->y2); > dst->x1 = dst->y1 = 0; > dst->x2 = width; > dst->y2 = height; > } > } > > static int mipi_dbi_dirtyfb(struct drm_framebuffer *fb, void *vmem, > unsigned flags, unsigned color, > struct drm_clip_rect *clips, unsigned num_clips) > { > struct drm_clip_rect clip; > > tinydrm_merge_clips(&clip, clips, num_clips, flags, > fb->width, fb->height); Seems ok too, and I'm starting to get a guilty feeling with signing you up for everything. I guess nuking drm_clip_rect from internal kms apis is something for the next person to show up ;-) -Daniel
Hi Noralf, On Monday 25 Apr 2016 20:35:18 Noralf Trønnes wrote: > Den 25.04.2016 18:38, skrev Ville Syrjälä: > > On Mon, Apr 25, 2016 at 06:05:20PM +0200, Daniel Vetter wrote: > >> On Mon, Apr 25, 2016 at 06:09:44PM +0300, Ville Syrjälä wrote: > >>> On Mon, Apr 25, 2016 at 04:03:13PM +0200, Noralf Trønnes wrote: > >>>> Den 25.04.2016 15:02, skrev Ville Syrjälä: > >>>>> On Mon, Apr 25, 2016 at 02:55:52PM +0200, Noralf Trønnes wrote: > >>>>>> Den 25.04.2016 14:39, skrev Ville Syrjälä: > >>>>>>> On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote: > >>>>>>>> Add some utility functions for struct drm_clip_rect. > >>>>>>> > >>>>>>> Looks like mostly you're just duplicating the drm_rect stuff. Why > >>>>>>> can't you use what's there already? > >>>>>> > >>>>>> That's because the framebuffer flushing uses drm_clip_rect and not > >>>>>> drm_rect: > >>>>> > >>>>> Converting to drm_rect is not an option? > >>>> > >>>> That's difficult or at least verbose to do because clips is an array. > >>>> I could use drm_rect on the calling side (fbdev) since it's only one > >>>> clip which the changes are merged into, and then convert it when I call > >>>> dirty(). But the driver can get zero or more clips from the dirty ioctl > >>>> so I don't see a clean way to convert this array to drm_rect without > >>>> more code than this proposal has. > >>> > >>> Just some kind of simple drm_clip_rect_to_rect() thing should be enough > >>> AFAICS. > >> > >> Yeah, drm_clip_rect is the uapi struct, drm_rect is the internal one. > >> Similar case is drm_display_mode vs. drm_mode_modeinfo. We have > >> umode_to_mode and mode_to_umode helpers to deal with that. I do agree > >> that it would make sense to switch the internal ->dirty callback over to > >> the internal drm_struct. Would need a kmalloc+copy in the dirtyfb ioctl, > >> but since the structs actually match in their member names (just not the > >> size/signedness, sigh) there shouldn't be any need for driver changes. So > >> fairly simple patch. > > > > Or if we want to avoid the malloc, then the merge() thing could just > > internally convert one at a time on stack when going through them. > > Though then someone might want to do a merge() with internal drm_rects, > > and we'd be right where we started. But I'm not sure that will happen, > > so perhaps it's just too much future proofing. > > > >> Ofc you need to compile-test all the drivers (at least those using > >> ->dirty hook) to make sure gcc is still happy with all the signed vs. > >> unsigned stuff. Maybe that turns up something, but hopefully not. > >> > >> Sorry for that late request, but I really didn't realize what's going on > >> here :( > > How about we just drop this patch? > I couldn't find anyone else that merge these clips, they just loop and > handle them individually. > > The relevant part in drm_fb_helper would become: > > static void drm_fb_helper_dirty_work(struct work_struct *work) > { > struct drm_fb_helper *helper = container_of(work, struct drm_fb_helper, > dirty_work); > struct drm_clip_rect *clip = &helper->dirty_clip; > struct drm_clip_rect clip_copy; > unsigned long flags; > > spin_lock_irqsave(&helper->dirty_lock, flags); > clip_copy = *clip; > clip->x1 = clip->y1 = ~0; > clip->x2 = clip->y2 = 0; > spin_unlock_irqrestore(&helper->dirty_lock, flags); > > helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1); > } > > static void drm_fb_helper_dirty_init(struct drm_fb_helper *helper) > { > spin_lock_init(&helper->dirty_lock); > INIT_WORK(&helper->dirty_work, drm_fb_helper_dirty_work); > helper->dirty_clip.x1 = helper->dirty_clip.y1 = ~0; > } > > static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y, > u32 width, u32 height) > { > struct drm_fb_helper *helper = info->par; > struct drm_clip_rect *clip = &helper->dirty_clip; > unsigned long flags; > > if (!helper->fb->funcs->dirty) > return; > > spin_lock_irqsave(&helper->dirty_lock, flags); > clip->x1 = min(clip->x1, x); > clip->y1 = min(clip->y1, y); > clip->x2 = max(clip->x2, x + width); > clip->y2 = max(clip->y2, y + height); > spin_unlock_irqrestore(&helper->dirty_lock, flags); > > schedule_work(&helper->dirty_work); > } > > > And the driver would use this tinydrm function: > > void tinydrm_merge_clips(struct drm_clip_rect *dst, > struct drm_clip_rect *src, unsigned num_clips, > unsigned flags, u32 width, u32 height) > { > int i; Nitpicking here, as i never takes negative values, could you make it an unsigned int ? > > if (!src || !num_clips) { > dst->x1 = 0; > dst->x2 = width; > dst->y1 = 0; > dst->y2 = height; > return; > } > > dst->x1 = dst->y1 = ~0; > dst->x2 = dst->y2 = 0; > > for (i = 0; i < num_clips; i++) { > if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) > i++; > dst->x1 = min(dst->x1, src[i].x1); > dst->x2 = max(dst->x2, src[i].x2); > dst->y1 = min(dst->y1, src[i].y1); > dst->y2 = max(dst->y2, src[i].y2); > } > > if (dst->x2 > width || dst->y2 > height || > dst->x1 >= dst->x2 || dst->y1 >= dst->y2) { > DRM_DEBUG_KMS("Illegal clip: x1=%u, x2=%u, y1=%u, y2=%u\n", > dst->x1, dst->x2, dst->y1, dst->y2); > dst->x1 = dst->y1 = 0; > dst->x2 = width; > dst->y2 = height; > } > } > > static int mipi_dbi_dirtyfb(struct drm_framebuffer *fb, void *vmem, > unsigned flags, unsigned color, > struct drm_clip_rect *clips, unsigned num_clips) > { > struct drm_clip_rect clip; > > tinydrm_merge_clips(&clip, clips, num_clips, flags, > fb->width, fb->height);
Den 25.04.2016 23:13, skrev Laurent Pinchart: > Hi Noralf, > > On Monday 25 Apr 2016 20:35:18 Noralf Trønnes wrote: >> Den 25.04.2016 18:38, skrev Ville Syrjälä: >>> On Mon, Apr 25, 2016 at 06:05:20PM +0200, Daniel Vetter wrote: >>>> On Mon, Apr 25, 2016 at 06:09:44PM +0300, Ville Syrjälä wrote: >>>>> On Mon, Apr 25, 2016 at 04:03:13PM +0200, Noralf Trønnes wrote: >>>>>> Den 25.04.2016 15:02, skrev Ville Syrjälä: >>>>>>> On Mon, Apr 25, 2016 at 02:55:52PM +0200, Noralf Trønnes wrote: >>>>>>>> Den 25.04.2016 14:39, skrev Ville Syrjälä: >>>>>>>>> On Sun, Apr 24, 2016 at 10:48:55PM +0200, Noralf Trønnes wrote: >>>>>>>>>> Add some utility functions for struct drm_clip_rect. [...] >> How about we just drop this patch? >> I couldn't find anyone else that merge these clips, they just loop and >> handle them individually. >> >> The relevant part in drm_fb_helper would become: >> >> static void drm_fb_helper_dirty_work(struct work_struct *work) >> { >> struct drm_fb_helper *helper = container_of(work, struct drm_fb_helper, >> dirty_work); >> struct drm_clip_rect *clip = &helper->dirty_clip; >> struct drm_clip_rect clip_copy; >> unsigned long flags; >> >> spin_lock_irqsave(&helper->dirty_lock, flags); >> clip_copy = *clip; >> clip->x1 = clip->y1 = ~0; >> clip->x2 = clip->y2 = 0; >> spin_unlock_irqrestore(&helper->dirty_lock, flags); >> >> helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1); >> } >> >> static void drm_fb_helper_dirty_init(struct drm_fb_helper *helper) >> { >> spin_lock_init(&helper->dirty_lock); >> INIT_WORK(&helper->dirty_work, drm_fb_helper_dirty_work); >> helper->dirty_clip.x1 = helper->dirty_clip.y1 = ~0; >> } >> >> static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y, >> u32 width, u32 height) >> { >> struct drm_fb_helper *helper = info->par; >> struct drm_clip_rect *clip = &helper->dirty_clip; >> unsigned long flags; >> >> if (!helper->fb->funcs->dirty) >> return; >> >> spin_lock_irqsave(&helper->dirty_lock, flags); >> clip->x1 = min(clip->x1, x); >> clip->y1 = min(clip->y1, y); >> clip->x2 = max(clip->x2, x + width); >> clip->y2 = max(clip->y2, y + height); >> spin_unlock_irqrestore(&helper->dirty_lock, flags); >> >> schedule_work(&helper->dirty_work); >> } >> >> >> And the driver would use this tinydrm function: >> >> void tinydrm_merge_clips(struct drm_clip_rect *dst, >> struct drm_clip_rect *src, unsigned num_clips, >> unsigned flags, u32 width, u32 height) >> { >> int i; > Nitpicking here, as i never takes negative values, could you make it an > unsigned int ? Sure. >> if (!src || !num_clips) { >> dst->x1 = 0; >> dst->x2 = width; >> dst->y1 = 0; >> dst->y2 = height; >> return; >> } >> >> dst->x1 = dst->y1 = ~0; >> dst->x2 = dst->y2 = 0; >> >> for (i = 0; i < num_clips; i++) { >> if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) >> i++; >> dst->x1 = min(dst->x1, src[i].x1); >> dst->x2 = max(dst->x2, src[i].x2); >> dst->y1 = min(dst->y1, src[i].y1); >> dst->y2 = max(dst->y2, src[i].y2); >> } >> >> if (dst->x2 > width || dst->y2 > height || >> dst->x1 >= dst->x2 || dst->y1 >= dst->y2) { >> DRM_DEBUG_KMS("Illegal clip: x1=%u, x2=%u, y1=%u, y2=%u\n", >> dst->x1, dst->x2, dst->y1, dst->y2); >> dst->x1 = dst->y1 = 0; >> dst->x2 = width; >> dst->y2 = height; >> } >> } >> >> static int mipi_dbi_dirtyfb(struct drm_framebuffer *fb, void *vmem, >> unsigned flags, unsigned color, >> struct drm_clip_rect *clips, unsigned num_clips) >> { >> struct drm_clip_rect clip; >> >> tinydrm_merge_clips(&clip, clips, num_clips, flags, >> fb->width, fb->height);
diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c index a8e2c86..a9fb1a8 100644 --- a/drivers/gpu/drm/drm_rect.c +++ b/drivers/gpu/drm/drm_rect.c @@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r, } } EXPORT_SYMBOL(drm_rect_rotate_inv); + +/** + * drm_clip_rect_intersect - intersect two clip rectangles + * @r1: first clip rectangle + * @r2: second clip rectangle + * + * Calculate the intersection of clip rectangles @r1 and @r2. + * @r1 will be overwritten with the intersection. + * + * RETURNS: + * %true if rectangle @r1 is still visible after the operation, + * %false otherwise. + */ +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, + const struct drm_clip_rect *r2) +{ + r1->x1 = max(r1->x1, r2->x1); + r1->y1 = max(r1->y1, r2->y1); + r1->x2 = min(r1->x2, r2->x2); + r1->y2 = min(r1->y2, r2->y2); + + return drm_clip_rect_visible(r1); +} +EXPORT_SYMBOL(drm_clip_rect_intersect); + +/** + * drm_clip_rect_merge - Merge clip rectangles + * @dst: destination clip rectangle + * @src: source clip rectangle(s), can be NULL + * @num_clips: number of source clip rectangles + * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY) + * @width: width of clip rectangle if @src is NULL + * @height: height of clip rectangle if @src is NULL + * + * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in, + * so if @src is NULL, width and height is used to set a full clip rectangle. + * @dst takes part in the merge unless it is empty {0,0,0,0}. + */ +void drm_clip_rect_merge(struct drm_clip_rect *dst, + struct drm_clip_rect *src, unsigned num_clips, + unsigned flags, u32 width, u32 height) +{ + int i; + + if (!src || !num_clips) { + dst->x1 = 0; + dst->x2 = width; + dst->y1 = 0; + dst->y2 = height; + return; + } + + if (drm_clip_rect_is_empty(dst)) { + dst->x1 = ~0; + dst->y1 = ~0; + } + + for (i = 0; i < num_clips; i++) { + if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) + i++; + dst->x1 = min(dst->x1, src[i].x1); + dst->x2 = max(dst->x2, src[i].x2); + dst->y1 = min(dst->y1, src[i].y1); + dst->y2 = max(dst->y2, src[i].y2); + } +} +EXPORT_SYMBOL(drm_clip_rect_merge); diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h index 83bb156..936ad8d 100644 --- a/include/drm/drm_rect.h +++ b/include/drm/drm_rect.h @@ -24,6 +24,8 @@ #ifndef DRM_RECT_H #define DRM_RECT_H +#include <uapi/drm/drm.h> + /** * DOC: rect utils * @@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r, int width, int height, unsigned int rotation); +/** + * drm_clip_rect_width - determine the clip rectangle width + * @r: clip rectangle whose width is returned + * + * RETURNS: + * The width of the clip rectangle. + */ +static inline int drm_clip_rect_width(const struct drm_clip_rect *r) +{ + return r->x2 - r->x1; +} + +/** + * drm_clip_rect_height - determine the clip rectangle height + * @r: clip rectangle whose height is returned + * + * RETURNS: + * The height of the clip rectangle. + */ +static inline int drm_clip_rect_height(const struct drm_clip_rect *r) +{ + return r->y2 - r->y1; +} + +/** + * drm_clip_rect_visible - determine if the the clip rectangle is visible + * @r: clip rectangle whose visibility is returned + * + * RETURNS: + * %true if the clip rectangle is visible, %false otherwise. + */ +static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r) +{ + return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0; +} + +/** + * drm_clip_rect_reset - Reset clip rectangle + * @clip: clip rectangle + * + * Sets clip rectangle to {0,0,0,0}. + */ +static inline void drm_clip_rect_reset(struct drm_clip_rect *clip) +{ + clip->x1 = 0; + clip->x2 = 0; + clip->y1 = 0; + clip->y2 = 0; +} + +/** + * drm_clip_rect_is_empty - Is clip rectangle empty? + * @clip: clip rectangle + * + * Returns true if clip rectangle is {0,0,0,0}. + */ +static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip) +{ + return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2); +} + +bool drm_clip_rect_intersect(struct drm_clip_rect *r1, + const struct drm_clip_rect *r2); +void drm_clip_rect_merge(struct drm_clip_rect *dst, + struct drm_clip_rect *src, unsigned num_clips, + unsigned flags, u32 width, u32 height); + #endif
Add some utility functions for struct drm_clip_rect. Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_rect.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+)