* 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).