Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933149AbcDYTDX (ORCPT ); Mon, 25 Apr 2016 15:03:23 -0400 Received: from mail-wm0-f45.google.com ([74.125.82.45]:38250 "EHLO mail-wm0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754962AbcDYTDU (ORCPT ); Mon, 25 Apr 2016 15:03:20 -0400 Date: Mon, 25 Apr 2016 21:03:15 +0200 From: Daniel Vetter To: Noralf =?iso-8859-1?Q?Tr=F8nnes?= Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= , linux-fbdev@vger.kernel.org, tomi.valkeinen@ti.com, laurent.pinchart@ideasonboard.com, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions Message-ID: <20160425190315.GX2510@phenom.ffwll.local> Mail-Followup-To: Noralf =?iso-8859-1?Q?Tr=F8nnes?= , Ville =?iso-8859-1?Q?Syrj=E4l=E4?= , 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> <571E6366.20504@tronnes.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <571E6366.20504@tronnes.org> X-Operating-System: Linux phenom 4.4.0-1-amd64 User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5972 Lines: 150 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch