lkml.org 
[lkml]   [2016]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions
    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

    --
    Ville Syrjälä
    Intel OTC

    \
     
     \ /
      Last update: 2016-04-25 18:41    [W:6.154 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site