Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933009AbcDYQF1 (ORCPT ); Mon, 25 Apr 2016 12:05:27 -0400 Received: from mail-wm0-f41.google.com ([74.125.82.41]:38430 "EHLO mail-wm0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932901AbcDYQF0 (ORCPT ); Mon, 25 Apr 2016 12:05:26 -0400 Date: Mon, 25 Apr 2016 18:05:20 +0200 From: Daniel Vetter To: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Cc: Noralf =?iso-8859-1?Q?Tr=F8nnes?= , 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: <20160425160520.GV2510@phenom.ffwll.local> Mail-Followup-To: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= , Noralf =?iso-8859-1?Q?Tr=F8nnes?= , 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> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20160425150944.GB4329@intel.com> 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: 9790 Lines: 262 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 > > >>>> --- > > >>>> 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