* Re: xf86-video-intel: 3 commits - src/intel_display.c src/intel_dri.c src/intel.h [not found] <20110324002457.C7976F80E7@kemper.freedesktop.org> @ 2011-03-24 1:02 ` Julien Cristau 2011-03-24 18:09 ` Keith Packard 0 siblings, 1 reply; 6+ messages in thread From: Julien Cristau @ 2011-03-24 1:02 UTC (permalink / raw) To: Keith Packard; +Cc: intel-gfx Hi Keith, a couple suggestions below from a quick look over these patches. On Wed, Mar 23, 2011 at 17:24:57 -0700, Keith Packard wrote: > commit e1ff5182304e00c0d392092069422cae7626cf8d > Author: Keith Packard <keithp@keithp.com> > Date: Wed Mar 9 17:00:41 2011 -0800 > > Handle drawable/client destruction in pending swaps/flips > > A pending swap or flip holds references to drawables and clients which > become invalid when destroyed. Add suitable resources to the database > to track those lifetimes and clean up the pending data structure then. > > Later, when the pending swap or flip occurs, handle a missing drawable > by just discarding the flip or swap. Handle a missing client by not > sending an event or reply. > > Signed-off-by: Keith Packard <keithp@keithp.com> > [...] > diff --git a/src/intel_display.c b/src/intel_display.c > index eb07cf5..4734844 100644 > --- a/src/intel_display.c > +++ b/src/intel_display.c > @@ -1512,7 +1512,7 @@ static const xf86CrtcConfigFuncsRec intel_xf86crtc_config_funcs = { > > static void > intel_vblank_handler(int fd, unsigned int frame, unsigned int tv_sec, > - unsigned int tv_usec, DRI2FrameEventPtr event) > + unsigned int tv_usec, void *event) This seems to just revert a change from the previous commit? > { > I830DRI2FrameEventHandler(frame, tv_sec, tv_usec, event); > } > diff --git a/src/intel_dri.c b/src/intel_dri.c > index 9e8c370..f039e9d 100644 > --- a/src/intel_dri.c > +++ b/src/intel_dri.c > @@ -577,6 +577,69 @@ I830DRI2DrawablePipe(DrawablePtr pDraw) [...] > +/* > + * Hook this frame event into the server resource > + * database so we can clean it up if the drawable or > + * client exits while the swap is pending > + */ > +static Bool > +i830_dri2_add_frame_event(DRI2FrameEventPtr frame_event) > +{ > + frame_event->client_id = FakeClientID(frame_event->client->index); > + > + if (!AddResource(frame_event->client_id, frame_event_client_type, frame_event)) > + return FALSE; > + > + if (!AddResource(frame_event->drawable_id, frame_event_drawable_type, frame_event)) > + return FALSE; > + > + return TRUE; > +} > + > +static void > +i830_dri2_del_frame_event(DRI2FrameEventPtr frame_event) > +{ > + if (frame_event->client_id) > + FreeResourceByType(frame_event->client_id, frame_event_client_type, FALSE); > + if (frame_event->drawable_id) > + FreeResourceByType(frame_event->drawable_id, frame_event_drawable_type, FALSE); > +} > + > static void > I830DRI2ExchangeBuffers(DrawablePtr draw, DRI2BufferPtr front, > DRI2BufferPtr back) > @@ -642,11 +705,18 @@ I830DRI2ScheduleFlip(struct intel_screen_private *intel, > flip_info->event_data = data; > flip_info->frame = target_msc; > > + i830_dri2_add_frame_event(flip_info); > + if (!i830_dri_add_frame_event(flip_info)) return FALSE; ? > /* Page flip the full screen buffer */ > back_priv = back->driverPrivate; > - return intel_do_pageflip(intel, > - intel_get_pixmap_bo(back_priv->pixmap), > - flip_info, ref_crtc_hw_id); > + if (intel_do_pageflip(intel, > + intel_get_pixmap_bo(back_priv->pixmap), > + flip_info, ref_crtc_hw_id)) > + return TRUE; > + > + i830_dri2_del_frame_event(flip_info); > + free(flip_info); > + return FALSE; > } > > static Bool [...] > @@ -876,6 +958,8 @@ I830DRI2ScheduleSwap(ClientPtr client, DrawablePtr draw, DRI2BufferPtr front, > I830DRI2ReferenceBuffer(front); > I830DRI2ReferenceBuffer(back); > > + i830_dri2_add_frame_event(swap_info); > + if (!i830_dri2_add_frame_event(swap_info) goto blit_fallback; ? > /* Get current count */ > vbl.request.type = DRM_VBLANK_RELATIVE; > if (pipe > 0) [...] Cheers, Julien ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: xf86-video-intel: 3 commits - src/intel_display.c src/intel_dri.c src/intel.h 2011-03-24 1:02 ` xf86-video-intel: 3 commits - src/intel_display.c src/intel_dri.c src/intel.h Julien Cristau @ 2011-03-24 18:09 ` Keith Packard 2011-03-24 18:31 ` Julien Cristau 0 siblings, 1 reply; 6+ messages in thread From: Keith Packard @ 2011-03-24 18:09 UTC (permalink / raw) To: Julien Cristau, Keith Packard; +Cc: intel-gfx [-- Attachment #1.1: Type: text/plain, Size: 2608 bytes --] On Thu, 24 Mar 2011 02:02:56 +0100, Julien Cristau <jcristau@debian.org> wrote: > Hi Keith, > > a couple suggestions below from a quick look over these patches. Thanks for your review! > > static void > > intel_vblank_handler(int fd, unsigned int frame, unsigned int tv_sec, > > - unsigned int tv_usec, DRI2FrameEventPtr event) > > + unsigned int tv_usec, void *event) > > This seems to just revert a change from the previous commit? Sadly, yes -- intel_vblank_handler gets stuffed into the event_context.vblank_handler callback slot which uses 'void *' in the function signature. As DRI2FrameEventPtr is a private type, we can't go fix the public drmEventContext type to match. A cleaner patch sequence would have removed the change from both patches. Oops! > > + i830_dri2_add_frame_event(flip_info); > > + > > if (!i830_dri_add_frame_event(flip_info)) > return FALSE; > ? ... > > + i830_dri2_add_frame_event(swap_info); > > + > > if (!i830_dri2_add_frame_event(swap_info) > goto blit_fallback; > ? Good catch. Not quite that easy to fix; the swap_info needs to be freed, and a partial add_frame_event must clean up after itself. diff --git a/src/intel_dri.c b/src/intel_dri.c index 3b80823..f7a4fc4 100644 --- a/src/intel_dri.c +++ b/src/intel_dri.c @@ -625,8 +625,10 @@ i830_dri2_add_frame_event(DRI2FrameEventPtr frame_event) if (!AddResource(frame_event->client_id, frame_event_client_type, frame_event)) return FALSE; - if (!AddResource(frame_event->drawable_id, frame_event_drawable_type, frame_event)) + if (!AddResource(frame_event->drawable_id, frame_event_drawable_type, frame_event)) { + FreeResourceByType(frame_event->client_id, frame_event_client_type, TRUE); return FALSE; + } return TRUE; } @@ -705,7 +707,10 @@ I830DRI2ScheduleFlip(struct intel_screen_private *intel, flip_info->event_data = data; flip_info->frame = target_msc; - i830_dri2_add_frame_event(flip_info); + if (!i830_dri2_add_frame_event(flip_info)) { + free(flip_info); + return FALSE; + } /* Page flip the full screen buffer */ back_priv = back->driverPrivate; @@ -958,7 +963,10 @@ I830DRI2ScheduleSwap(ClientPtr client, DrawablePtr draw, DRI2BufferPtr front, I830DRI2ReferenceBuffer(front); I830DRI2ReferenceBuffer(back); - i830_dri2_add_frame_event(swap_info); + if (!i830_dri2_add_frame_event(swap_info)) { + free(swap_info); + goto blit_fallback; + } /* Get current count */ vbl.request.type = DRM_VBLANK_RELATIVE; -- keith.packard@intel.com [-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: xf86-video-intel: 3 commits - src/intel_display.c src/intel_dri.c src/intel.h 2011-03-24 18:09 ` Keith Packard @ 2011-03-24 18:31 ` Julien Cristau 2011-03-24 23:52 ` Keith Packard 0 siblings, 1 reply; 6+ messages in thread From: Julien Cristau @ 2011-03-24 18:31 UTC (permalink / raw) To: Keith Packard; +Cc: Keith Packard, intel-gfx On Thu, Mar 24, 2011 at 11:09:36 -0700, Keith Packard wrote: > diff --git a/src/intel_dri.c b/src/intel_dri.c > index 3b80823..f7a4fc4 100644 > --- a/src/intel_dri.c > +++ b/src/intel_dri.c > @@ -625,8 +625,10 @@ i830_dri2_add_frame_event(DRI2FrameEventPtr frame_event) > if (!AddResource(frame_event->client_id, frame_event_client_type, frame_event)) > return FALSE; > > - if (!AddResource(frame_event->drawable_id, frame_event_drawable_type, frame_event)) > + if (!AddResource(frame_event->drawable_id, frame_event_drawable_type, frame_event)) { > + FreeResourceByType(frame_event->client_id, frame_event_client_type, TRUE); > return FALSE; > + } > > return TRUE; > } > @@ -705,7 +707,10 @@ I830DRI2ScheduleFlip(struct intel_screen_private *intel, > flip_info->event_data = data; > flip_info->frame = target_msc; > > - i830_dri2_add_frame_event(flip_info); > + if (!i830_dri2_add_frame_event(flip_info)) { > + free(flip_info); > + return FALSE; > + } > > /* Page flip the full screen buffer */ > back_priv = back->driverPrivate; > @@ -958,7 +963,10 @@ I830DRI2ScheduleSwap(ClientPtr client, DrawablePtr draw, DRI2BufferPtr front, > I830DRI2ReferenceBuffer(front); > I830DRI2ReferenceBuffer(back); > > - i830_dri2_add_frame_event(swap_info); > + if (!i830_dri2_add_frame_event(swap_info)) { > + free(swap_info); This will cause a double free as the blit_fallback path does it too. > + goto blit_fallback; > + } > > /* Get current count */ > vbl.request.type = DRM_VBLANK_RELATIVE; > Cheers, Julien ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: xf86-video-intel: 3 commits - src/intel_display.c src/intel_dri.c src/intel.h 2011-03-24 18:31 ` Julien Cristau @ 2011-03-24 23:52 ` Keith Packard 2011-03-24 23:57 ` Julien Cristau 0 siblings, 1 reply; 6+ messages in thread From: Keith Packard @ 2011-03-24 23:52 UTC (permalink / raw) To: Julien Cristau; +Cc: Keith Packard, intel-gfx [-- Attachment #1.1: Type: text/plain, Size: 917 bytes --] On Thu, 24 Mar 2011 19:31:03 +0100, Julien Cristau <jcristau@debian.org> wrote: > This will cause a double free as the blit_fallback path does it too. Argh! So we need to check before we reference the buffers and set swap_info to NULL. This code is too twisty... @@ -955,11 +960,16 @@ I830DRI2ScheduleSwap(ClientPtr client, DrawablePtr draw, DRI2BufferPtr front, swap_info->event_data = data; swap_info->front = front; swap_info->back = back; + + if (!i830_dri2_add_frame_event(swap_info)) { + free(swap_info); + swap_info = NULL; + goto blit_fallback; + } + I830DRI2ReferenceBuffer(front); I830DRI2ReferenceBuffer(back); - i830_dri2_add_frame_event(swap_info); - /* Get current count */ vbl.request.type = DRM_VBLANK_RELATIVE; if (pipe > 0) -- keith.packard@intel.com [-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: xf86-video-intel: 3 commits - src/intel_display.c src/intel_dri.c src/intel.h 2011-03-24 23:52 ` Keith Packard @ 2011-03-24 23:57 ` Julien Cristau 2011-03-25 0:14 ` Keith Packard 0 siblings, 1 reply; 6+ messages in thread From: Julien Cristau @ 2011-03-24 23:57 UTC (permalink / raw) To: Keith Packard; +Cc: Keith Packard, intel-gfx On Thu, Mar 24, 2011 at 16:52:25 -0700, Keith Packard wrote: > On Thu, 24 Mar 2011 19:31:03 +0100, Julien Cristau <jcristau@debian.org> wrote: > > > This will cause a double free as the blit_fallback path does it too. > > Argh! So we need to check before we reference the buffers and set > swap_info to NULL. This code is too twisty... > > @@ -955,11 +960,16 @@ I830DRI2ScheduleSwap(ClientPtr client, DrawablePtr draw, DRI2BufferPtr front, > swap_info->event_data = data; > swap_info->front = front; > swap_info->back = back; > + > + if (!i830_dri2_add_frame_event(swap_info)) { > + free(swap_info); > + swap_info = NULL; > + goto blit_fallback; > + } > + > I830DRI2ReferenceBuffer(front); > I830DRI2ReferenceBuffer(back); > > - i830_dri2_add_frame_event(swap_info); > - > /* Get current count */ > vbl.request.type = DRM_VBLANK_RELATIVE; > if (pipe > 0) > With that change, Reviewed-by: Julien Cristau <jcristau@debian.org> Thanks! Julien ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: xf86-video-intel: 3 commits - src/intel_display.c src/intel_dri.c src/intel.h 2011-03-24 23:57 ` Julien Cristau @ 2011-03-25 0:14 ` Keith Packard 0 siblings, 0 replies; 6+ messages in thread From: Keith Packard @ 2011-03-25 0:14 UTC (permalink / raw) To: Julien Cristau; +Cc: Keith Packard, intel-gfx [-- Attachment #1.1: Type: text/plain, Size: 303 bytes --] On Fri, 25 Mar 2011 00:57:10 +0100, Julien Cristau <jcristau@debian.org> wrote: > With that change, > Reviewed-by: Julien Cristau <jcristau@debian.org> Thanks for catching my bugs before they caused any damage... Pushed. ec133ab..7ccbec8 master -> master -- keith.packard@intel.com [-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-03-25 0:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20110324002457.C7976F80E7@kemper.freedesktop.org>
2011-03-24 1:02 ` xf86-video-intel: 3 commits - src/intel_display.c src/intel_dri.c src/intel.h Julien Cristau
2011-03-24 18:09 ` Keith Packard
2011-03-24 18:31 ` Julien Cristau
2011-03-24 23:52 ` Keith Packard
2011-03-24 23:57 ` Julien Cristau
2011-03-25 0:14 ` Keith Packard
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.