Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933255AbcDYSfb (ORCPT ); Mon, 25 Apr 2016 14:35:31 -0400 Received: from smtp.domeneshop.no ([194.63.252.55]:42728 "EHLO smtp.domeneshop.no" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932528AbcDYSf1 (ORCPT ); Mon, 25 Apr 2016 14:35:27 -0400 Subject: Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions To: =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , linux-fbdev@vger.kernel.org, tomi.valkeinen@ti.com, laurent.pinchart@ideasonboard.com, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org References: <1461530942-22485-1-git-send-email-noralf@tronnes.org> <1461530942-22485-2-git-send-email-noralf@tronnes.org> <20160425123907.GY4329@intel.com> <571E13D8.4060100@tronnes.org> <20160425130229.GZ4329@intel.com> <571E23A1.2040100@tronnes.org> <20160425150944.GB4329@intel.com> <20160425160520.GV2510@phenom.ffwll.local> <20160425163816.GF4329@intel.com> From: =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= Message-ID: <571E6366.20504@tronnes.org> Date: Mon, 25 Apr 2016 20:35:18 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.7.1 MIME-Version: 1.0 In-Reply-To: <20160425163816.GF4329@intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 13313 Lines: 360 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 >>>>>>>> --- >>>>>>>> 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 >>>>>>>> + >>>>>>>> /** >>>>>>>> * 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