| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1615589.QRyc9Clv2Y@avalon>
Date: Tue, 26 Apr 2016 00:13:49 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Noralf Trønnes <noralf@...nnes.org>
Cc: Ville Syrjälä
<ville.syrjala@...ux.intel.com>, linux-fbdev@...r.kernel.org,
tomi.valkeinen@...com, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/8] drm/rect: Add some drm_clip_rect utility functions
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);
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists