Re: [PATCH] DRI2/GLX: fix swap event handling
On Don, 2011-04-28 at 13:27 -0700, Jesse Barnes wrote:
> @@ -1114,7 +1169,7 @@ DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info)
> ds->ScheduleSwap = info->ScheduleSwap;
> ds->ScheduleWaitMSC = info->ScheduleWaitMSC;
> ds->GetMSC = info->GetMSC;
> - cur_minor = 3;
> + cur_minor = 4;
> } else {
> cur_minor = 1;
> }
This bugfix should probably be separate.
--
Earthling Michel Dänzer |http://www.vmware.com
Libre software enthusiast | Debian, X and DRI developer
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: Factor-out drm_emit_vblank_event code. (v2)
On Fre, 2011-04-29 at 13:57 +1000, christopher.halse.rog...@canonical.com wrote: > From: Christopher James Halse Rogers > > v2: Also pull out the drm_vblank_put call. > Signed-off-by: Christopher James Halse Rogers > Reviewed-by: Michel Dänzer -- Earthling Michel Dänzer |http://www.vmware.com Libre software enthusiast | Debian, X and DRI developer ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
i915: irq nobody cared
Hi, while watching flash video in a browser I got: irq 42: nobody cared (try booting with the "irqpoll" option) Pid: 0, comm: swapper Tainted: GW 2.6.39-rc3-mm1_64+ #1423 Call Trace: [] __report_bad_irq+0x31/0xc0 [] note_interrupt+0x194/0x1d0 [] handle_irq_event_percpu+0xe8/0x160 [] handle_irq_event+0x35/0x60 [] handle_edge_irq+0x6f/0x110 [] handle_irq+0x1d/0x30 [] do_IRQ+0x58/0xe0 [] common_interrupt+0x13/0x13 [] ? __switch_to+0x260/0x2c0 [] ? tick_nohz_stop_sched_tick+0x28e/0x3d0 [] ? default_idle+0x24/0x40 [] cpu_idle+0x36/0xb0 [] rest_init+0x68/0x70 [] start_kernel+0x35c/0x367 [] x86_64_start_reservations+0x132/0x136 [] x86_64_start_kernel+0xf1/0xf8 handlers: [] (i915_driver_irq_handler+0x0/0x420) Disabling IRQ #42 Now the interrupt is polled, so that the graphics is obviously sluggish. I have also /sys/kernel/debug/dri contents if you want anything of that. Going to reboot now. # lspci -vvnnxxxs 00:02.0 00:02.0 VGA compatible controller [0300]: Intel Corporation 82G33/G31 Express Integrated Graphics Controller [8086:29c2] (rev 02) (prog-if 00 [VGA controller]) Subsystem: Intel Corporation 82G33/G31 Express Integrated Graphics Controller [8086:29c2] Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+ Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- SERR- [disabled] Capabilities: [90] MSI: Enable+ Count=1/1 Maskable- 64bit- Address: fee0300c Data: 4161 Capabilities: [d0] Power Management version 2 Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-) Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME- Kernel driver in use: i915 00: 86 80 c2 29 07 04 90 00 02 00 00 03 00 00 00 00 10: 00 00 b8 fe 01 ec 00 00 08 00 00 d0 00 00 a0 fe 20: 00 00 00 00 00 00 00 00 00 00 00 00 86 80 c2 29 30: 00 00 00 00 90 00 00 00 00 00 00 00 05 01 00 00 40: 09 00 0b 01 00 00 00 00 01 00 00 00 00 00 00 00 50: 00 00 30 02 c9 03 00 00 00 00 00 00 00 00 80 af 60: 00 00 02 02 00 00 00 00 00 00 00 00 00 00 00 00 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 90: 05 d0 01 00 0c 30 e0 fe 61 41 00 00 00 00 00 00 a0: 11 11 00 00 00 00 06 03 00 00 00 00 00 00 00 00 b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 d0: 01 00 22 00 00 00 00 00 00 00 00 00 00 01 02 00 e0: 00 00 00 00 00 00 00 00 00 80 00 00 00 00 00 00 f0: 10 00 00 00 00 00 00 00 90 0f 03 00 e4 e0 5b af regards, -- js
[PATCH] drm: Factor-out drm_emit_vblank_event code. (v2)
From: Christopher James Halse Rogers
v2: Also pull out the drm_vblank_put call.
Signed-off-by: Christopher James Halse Rogers
---
drivers/gpu/drm/drm_irq.c | 44 ++--
1 files changed, 18 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 982ca8c..da56685 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -931,6 +931,20 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
}
EXPORT_SYMBOL(drm_vblank_put);
+static void drm_emit_vblank_event (struct drm_device *dev,
+ struct drm_pending_vblank_event *e,
+ unsigned int seq, struct timeval *now)
+{
+ e->event.sequence = seq;
+ e->event.tv_sec = now->tv_sec;
+ e->event.tv_usec = now->tv_usec;
+ drm_vblank_put(dev, e->pipe);
+ list_move_tail(&e->base.link, &e->base.file_priv->event_list);
+ wake_up_interruptible(&e->base.file_priv->event_wait);
+ trace_drm_vblank_event_delivered(e->base.pid, e->pipe,
+e->event.sequence);
+}
+
void drm_vblank_off(struct drm_device *dev, int crtc)
{
struct drm_pending_vblank_event *e, *t;
@@ -951,14 +965,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
wanted %d, current %d\n",
e->event.sequence, seq);
- e->event.sequence = seq;
- e->event.tv_sec = now.tv_sec;
- e->event.tv_usec = now.tv_usec;
- drm_vblank_put(dev, e->pipe);
- list_move_tail(&e->base.link, &e->base.file_priv->event_list);
- wake_up_interruptible(&e->base.file_priv->event_wait);
- trace_drm_vblank_event_delivered(e->base.pid, e->pipe,
-e->event.sequence);
+ drm_emit_vblank_event(dev, e, seq, &now);
}
WARN_ON(atomic_read(&dev->vblank_refcount[crtc]) != 0);
@@ -1104,18 +,10 @@ static int drm_queue_vblank_event(struct drm_device
*dev, int pipe,
vblwait->request.sequence);
e->event.sequence = vblwait->request.sequence;
+ list_add_tail(&e->base.link, &dev->vblank_event_list);
if ((seq - vblwait->request.sequence) <= (1 << 23)) {
- e->event.sequence = seq;
- e->event.tv_sec = now.tv_sec;
- e->event.tv_usec = now.tv_usec;
- drm_vblank_put(dev, pipe);
- list_add_tail(&e->base.link, &e->base.file_priv->event_list);
- wake_up_interruptible(&e->base.file_priv->event_wait);
- vblwait->reply.sequence = seq;
- trace_drm_vblank_event_delivered(current->pid, pipe,
-vblwait->request.sequence);
+ drm_emit_vblank_event(dev, e, seq, &now);
} else {
- list_add_tail(&e->base.link, &dev->vblank_event_list);
vblwait->reply.sequence = vblwait->request.sequence;
}
@@ -1249,14 +1248,7 @@ void drm_handle_vblank_events(struct drm_device *dev,
int crtc)
DRM_DEBUG("vblank event on %d, current %d\n",
e->event.sequence, seq);
- e->event.sequence = seq;
- e->event.tv_sec = now.tv_sec;
- e->event.tv_usec = now.tv_usec;
- drm_vblank_put(dev, e->pipe);
- list_move_tail(&e->base.link, &e->base.file_priv->event_list);
- wake_up_interruptible(&e->base.file_priv->event_wait);
- trace_drm_vblank_event_delivered(e->base.pid, e->pipe,
-e->event.sequence);
+ drm_emit_vblank_event(dev, e, seq, &now);
}
spin_unlock_irqrestore(&dev->event_lock, flags);
--
1.7.4.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC] drm: add overlays as first class KMS objects
On Thu, Apr 28, 2011 at 12:03:32PM -0500, Rob Clark wrote: > On Mon, Apr 25, 2011 at 5:12 PM, Jesse Barnes > wrote: > > Looking for comments on this. ?Obviously if we're going to add a new type > > of KMS object, we'd better get the ioctl more or less right to begin with, > > which means having all the attributes we'd like to track, plus some > > padding, available from the outset. > > > > So I'd like comments on this; the whole approach may be broken for things > > like OMAP; if so I'd like to hear about it now. ?Overall, overlays are > > treated a little like CRTCs, but without associated modes our encoder > > trees hanging off of them. ?That is, they can be enabled with a specific > > fb attached at a specific location, but they don't have to worry about > > mode setting, per se (though they do need to have an associated CRTC to > > actually pump their pixels out, post-blend). > > > > Flipping could be done either with the existing ioctl by updating the fb > > base pointer, or through a new flip ioctl similar to what we have already, > > but taking an overlay object instead of a CRTC. > > One thing I am wondering about is how to synchronize overlay position > w/ flipping in the primary gfx layer? Assuming you actually are > flipping in primary layer you'd want a new set of overlay > position/size to take effect on the same vblank that the flip in the > gfx layer happens, because you are probably relying on some > transparent pixels (or colorkey, if anyone still uses that) to be > drawn in the UI layer. That's a good reason to aim for an OpenWF Display type of API where you can commit the whole device state atomically. -- Ville Syrj?l? syrjala at sci.fi http://www.sci.fi/~syrjala/
[RFC] drm: add overlays as first class KMS objects
On Wed, Apr 27, 2011 at 11:12 PM, Jesse Barnes
wrote:
> On Wed, 27 Apr 2011 14:19:05 +0200
> Daniel Vetter wrote:
>
>> Hi Jesse,
>>
>> I like it. It's a bit of a chicken-egg api design problem, but if a
>> proof-of-concept
>> implementation exists for an embedded chip plus something to check whether
>> it's good enough to implement Xv on ancient hw (intel overlay or for the qemu
>> kms driver, if that could do this), that should be good enough.
>>
>> A few comments on the ioctl interface.
>
> Thanks for checking it out.
>
>> > +/* Overlays blend with or override other bits on the CRTC */
>> > +struct drm_mode_set_overlay {
>> > + ? ? ? __u32 overlay_id;
>> > + ? ? ? __u32 crtc_id;
>> > + ? ? ? __u32 fb_id; /* contains surface format type */
>> > +
>> > + ? ? ? __u32 crtc_x, crtc_y;
>> > + ? ? ? __u32 x, y;
>> > +
>> > + ? ? ? /* FIXME: color key/mask, scaling, z-order, other? */
>> > +};
>>
>> I think scaling is required, you can't implement Xv without that. The
>> problem is some arbitraray
>> hw range restrictions, but returning -EINVAL if they're violated looks okay.
>> So
>>
>> float scale_x, scale_y;
>
> Ok, I'll collect more fields based on this thread and make this
> structure bigger. ?Still not sure how to handle arbitrary blend and
> z-order restrictions without passing a tree around though...
What we send over the ioctl interface (in vmwgfx) to control the
overlays is this:
/**
* struct drm_vmw_control_stream_arg
*
* @stream_id: Stearm to control
* @enabled: If false all following arguments are ignored.
* @handle: Handle to buffer for getting data from.
* @format: Format of the overlay as understood by the host.
* @width: Width of the overlay.
* @height: Height of the overlay.
* @size: Size of the overlay in bytes.
* @pitch: Array of pitches, the two last are only used for YUV12 formats.
* @offset: Offset from start of dma buffer to overlay.
* @src: Source rect, must be within the defined area above.
* @dst: Destination rect, x and y may be negative.
*
* Argument to the DRM_VMW_CONTROL_STREAM Ioctl.
*/
struct drm_vmw_control_stream_arg {
uint32_t stream_id;
uint32_t enabled;
uint32_t flags;
uint32_t color_key;
uint32_t handle;
uint32_t offset;
int32_t format;
uint32_t size;
uint32_t width;
uint32_t height;
uint32_t pitch[3];
uint32_t pad64;
struct drm_vmw_rect src;
struct drm_vmw_rect dst;
};
The command contains information describing the source "framebuffer"
as well as the data for controlling the overlay. The args are by a
amazing coincidence is almost 1:1 what we also send down to the host.
The biggest change here from what you proposed is that we send two
rects describing from where within the buffer we get the data and to
where it should go. My vote is to do that as well (makes my life
easier at least).
Tho I guess that color_key and flags could be made into a property.
Cheers Jakob.
>
>>
>> should be good enough. For the remaining things (color conversion,
>> blending, ...) I don't think
>> there's any generic way to map hw capabilities (without going insane).
>> I think a bunch of
>> properties (with standard stuff for gamma, color key, ...) would be
>> perfect for that. If we
>> set the defaults such that it Just Works for Xv (after setting the
>> color key, if present), that
>> would be great!
>
> Yeah, properties for those is probably the way to go.
>
>>
>> > +struct drm_mode_get_overlay {
>> > + ? ? ? __u64 format_type_ptr;
>> > + ? ? ? __u32 overlay_id;
>> > +
>> > + ? ? ? __u32 crtc_id;
>> > + ? ? ? __u32 fb_id;
>> > +
>> > + ? ? ? __u32 crtc_x, crtc_y;
>> > + ? ? ? __u32 x, y;
>> > +
>> > + ? ? ? __u32 possible_crtcs;
>> > + ? ? ? __u32 gamma_size;
>> > +
>> > + ? ? ? __u32 count_format_types;
>> > +};
>>
>> Imo the real problem with formats is stride restrictions and other hw
>> restrictions (tiling, ...).
>> ARM/v4l people seem to want that to be in the kernel so that they can
>> e.g. dma decoded
>> video streams directly to the gpu (also for other stuff). Perhaps we
>> want to extend the
>> create_dumb_gem_object ioctl for that use case, but I'm not too
>> convinced that this belongs
>> into the kernel.
>
> I think it's a bit like handling tiling formats; for the most part the
> kernel doesn't care because it's not reading or writing the data, but
> it does need to know the format when programming certain regs. ?So I
> don't think we can avoid having surface format information passed in
> when creating an fb for an overlay. ?And if we do that we may as well
> enumerate the types we support when overlay info is fetched to make
> portability for app code a little easier.
>
> Jesse
[PATCH 2/3] drm: Warn if vblank state has become inconsistent.
On Apr 28, 2011, at 9:47 AM, Christopher James Halse Rogers wrote: > On Wed, 2011-04-27 at 17:50 +0200, Mario Kleiner wrote: >> On Apr 27, 2011, at 10:58 AM, dri-devel-request at lists.freedesktop.org >> wrote: >>> Message: 5 >>> Date: Wed, 27 Apr 2011 10:38:14 +0200 >>> From: Michel D?nzer >>> Subject: Re: [PATCH 2/3] drm: Warn if vblank state has become >>> inconsistent. >>> To: christopher.halse.rogers at canonical.com >>> Cc: dri-devel at lists.freedesktop.org >>> Message-ID: <1303893494.5633.129.camel at thor.local> >>> Content-Type: text/plain; charset="UTF-8" >>> >>> On Mit, 2011-04-27 at 16:10 +1000, >>> christopher.halse.rogers at canonical.com wrote: From: Christopher James Halse Rogers After emitting all the waiting vblank events no-one should hold a vblank reference. Emit a warning if this is not the case. Signed-off-by: Christopher James Halse Rogers --- drivers/gpu/drm/drm_irq.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index a1f12cb..72407fa 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -960,6 +960,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc) e->event.sequence); } + WARN_ON(atomic_read(&dev->vblank_refcount[crtc]) != 0); spin_unlock_irqrestore(&dev->vbl_lock, irqflags); } EXPORT_SYMBOL(drm_vblank_off); >>> >>> Reviewed-by: Michel D?nzer >>> >> >> >> Any pending kms pageflip will also hold a reference on the vblank of >> a crtc, so having the refcount non-zero there is not really a sign of >> inconsistency, so i'm not sure if a warning is appropriate there. > > But at this point the vblank interrupts have been disabled, or at > least > the driver's disable function has been called. Will that not mean > that > any pending pageflips will wait indefinitely for a vblank interrupt > that's not going to come - ie: exactly the state we're warning > against? > Ok, if that is the state we're warning against, then the warning makes sense. But it would make more sense to somehow handle this more gracefully. The pageflip completion path does depend on vblank irq's to unpin buffers, release fences and notify user apps of swap completion. Hanging there due to disabled vblank irq sounds bad. Otoh the only caller of drm_vblank_off() i can find in 2.6.38 is the crtc disable code in intel_display.c and those routines protect against pending pageflips - they wait for all pageflips to finish before calling drm_vblank_off(). If this warning is just a precaution against possible future bugs then i happily rest my case. -mario
[PATCH 1/3] drm: Send pending vblank events before disabling vblank.
On Wed, 2011-04-27 at 11:08 +0200, Michel D?nzer wrote: > On Mit, 2011-04-27 at 18:58 +1000, Christopher James Halse Rogers > wrote: > > On Wed, 2011-04-27 at 10:32 +0200, Michel D?nzer wrote: > > > On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers at > > > canonical.com wrote: > > > > From: Christopher James Halse Rogers > > > canonical.com> > > > > > > > > This is the least-bad behaviour. It means that we signal the > > > > vblank event before it actually happens, but since we're disabling > > > > vblanks there's no guarantee that it will *ever* happen otherwise. > > > > > > This may indeed be the best we can do for events that are pending when > > > the CRTC is disabled[0], but I can't see anything that would prevent new > > > events from getting scheduled (or synchronous vblank waits from timing > > > out) while the CRTC is disabled? > > > > > > [0] Though it might unnecessarily send events prematurely when the CRTC > > > is just disabled temporarily, e.g. as part of a modeset. > > > > > > > > > Also, this patch won't seem to help at all for other drivers which don't > > > call drm_vblank_off() directly when disabling a CRTC. > > > > This is true. On the other hand, the other drivers don't wedge the > > vblank code into a state where vblanks cannot be re-enabled. So it's > > only a problem when disabling one of 2+ monitors on those drivers, > > And with DPMS? > > > whereas it's easily triggerable on single monitor systems on intel. > > Anyway, this patch is probably good at least as a preliminary fix for > the inconsistent vblank state with the intel driver. > > > > > Maybe it would be possible to move those calls to core code, and/or only > > > force sending out events when the CRTC isn't just getting disabled > > > temporarily. > > > > > > > As in: have the core modesetting code call drm_vblank_off before making > > the driver-specific calls when disabling a crtc? I'll look into it - > > that would appear to be a more general solution. > > Yeah, something like that, thanks. > Ok. This appears to be surprisingly difficult. Particularly, the vblank code indexes crtcs based on the driver-private numbering, and there doesn't appear to be a generic interface to grab this number; Intel uses the DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID ioctl, radeon uses something different. I'll see what I can come up with, but I don't think I'm sufficiently familiar with the kms code to quickly come up with a nice API. -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 490 bytes Desc: This is a digitally signed message part URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20110428/5e64d8fc/attachment.pgp>
[Bug 36596] Major 2D performance bottleneck (most noticeable with compiz)
https://bugs.freedesktop.org/show_bug.cgi?id=36596 --- Comment #5 from Jason Cassell 2011-04-28 16:38:02 PDT --- Created an attachment (id=46146) --> (https://bugs.freedesktop.org/attachment.cgi?id=46146) /proc/interrupts -- Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 36596] Major 2D performance bottleneck (most noticeable with compiz)
https://bugs.freedesktop.org/show_bug.cgi?id=36596 --- Comment #5 from Jason Cassell 2011-04-28 16:38:02 PDT --- Created an attachment (id=46146) --> (https://bugs.freedesktop.org/attachment.cgi?id=46146) /proc/interrupts -- Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
[Bug 36596] Major 2D performance bottleneck (most noticeable with compiz)
https://bugs.freedesktop.org/show_bug.cgi?id=36596 --- Comment #4 from Jason Cassell 2011-04-28 16:36:31 PDT --- (In reply to comment #3) > Does that involve wobbly windows or any other effects? It's the same problem with and without wobbly windows. One thing I noticed with wobbly windows is that sometimes (but not every time) when the framerate goes back to normal, they'll wobble very fast, like they're trying to "catch up". It looks like it's trying to render all the frames, but some of them just show up late, like something got delayed in a buffer somewhere. I only notice that with wobbly windows, but that might just be because the wobbling makes the phenomenon easier to see. > Did you double-check that enabling LLVM actually worked? The configure script output says it's using llvm, and "llvm" appears a lot in the build log. I don't know if there's anything else to check. > If that's not the problem, and if the slowness coincides with high CPU usage, > a > profile from sysprof or oprofile might give some ideas. The slowness coincides with very low CPU usage. According to htop, when it's working fine, compiz uses from 5% to 16% CPU to move windows around, depending on the window and whether they're wobbly. When it's slow, compiz's CPU usage is 0.0%, and the CPU usage of everything else combined is less than 2%. This is on a dual core 2 GHz CPU, probably throttled back to 800 Mhz, but I don't know for sure because I wasn't running conky. htop refreshed every 2 seconds. Sometimes a refresh would make compiz freeze for a fraction of a second, but most of them didn't. Would it be useful to profile something anyway? BTW, I should have mentioned that I have to boot with the "noirqdebug" kernel parameter, otherwise the kernel will sometimes disable the Radeon's IRQ saying "irq 19: nobody cared" (the exact message is no longer in the logs). Once the IRQ was disabled, everything still worked, but more slowly. I also have scrolling performance problems in compiz, but compiz is infamous for causing things like that, so I'm guessing it's a different bug, and I'll worry about it later. -- Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 36596] Major 2D performance bottleneck (most noticeable with compiz)
https://bugs.freedesktop.org/show_bug.cgi?id=36596 --- Comment #4 from Jason Cassell 2011-04-28 16:36:31 PDT --- (In reply to comment #3) > Does that involve wobbly windows or any other effects? It's the same problem with and without wobbly windows. One thing I noticed with wobbly windows is that sometimes (but not every time) when the framerate goes back to normal, they'll wobble very fast, like they're trying to "catch up". It looks like it's trying to render all the frames, but some of them just show up late, like something got delayed in a buffer somewhere. I only notice that with wobbly windows, but that might just be because the wobbling makes the phenomenon easier to see. > Did you double-check that enabling LLVM actually worked? The configure script output says it's using llvm, and "llvm" appears a lot in the build log. I don't know if there's anything else to check. > If that's not the problem, and if the slowness coincides with high CPU usage, > a > profile from sysprof or oprofile might give some ideas. The slowness coincides with very low CPU usage. According to htop, when it's working fine, compiz uses from 5% to 16% CPU to move windows around, depending on the window and whether they're wobbly. When it's slow, compiz's CPU usage is 0.0%, and the CPU usage of everything else combined is less than 2%. This is on a dual core 2 GHz CPU, probably throttled back to 800 Mhz, but I don't know for sure because I wasn't running conky. htop refreshed every 2 seconds. Sometimes a refresh would make compiz freeze for a fraction of a second, but most of them didn't. Would it be useful to profile something anyway? BTW, I should have mentioned that I have to boot with the "noirqdebug" kernel parameter, otherwise the kernel will sometimes disable the Radeon's IRQ saying "irq 19: nobody cared" (the exact message is no longer in the logs). Once the IRQ was disabled, everything still worked, but more slowly. I also have scrolling performance problems in compiz, but compiz is infamous for causing things like that, so I'm guessing it's a different bug, and I'll worry about it later. -- Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
[PATCH] drm/nouveau: Remove interrupt handler around suspend/resume
On Wed, 2011-04-27 at 23:20 -0600, Alex Williamson wrote: > We're often using a shared interrupt line for nouveau, so we have > to be prepared that it could be called at any point in time. If > we've suspended the device via vga switcheroo and get a stray > interrupt on the line from another device, we'll read back -1 from > the device and head down all sorts of strange paths, most of which > eventually lock the system. > > On my system (Asus UL30VT) the interrupt line is shared with USB. > Attempting to disable the USB bluetooth device seems to trigger > a stray interrupt that ends up in nv04_fifo_isr() where we > eventually hit the "PFIFO still angry after 100 spins, halt", > which kills the system. > > Using free_irq/request_irq around the suspend seems to be a > reliable fix. Attempting to flag the device state in > nouvea_irq_handler(), similar to the intel_lid_notify() fix > is too racy since we can power off the device as an interrupt > is being processed. The actual solution is to check if we read back all Fs and return from the irq handler. Robust irq handlers are generally considered a good idea esp around race conditions at suspend/resume time. Dave.
Re: [PATCH] Pack swap complete bits into an XEvent
On Thu, 28 Apr 2011 13:27:19 -0700, Jesse Barnes wrote: > The defintion of the swap complete event was wrong; XEvents are only 32 > bytes long, and with padding the swap event was longer. So use some > creative packing to get all the bits we want transmitted. Requires a > proto version bump. If you've got a proto version on both sides, you should just stick the low 32 bits in place of the event type and pad fields, rather than abusing. Yes, this requires two different structures in the X server, but you need to do that anyways -- the event type has to shrink from 16 bits to 8 bits, moving it at the same time is not a problem. Besides, this will make the xcb case really ugly as xcb doesn't hide the protocol event structures from the application. -- keith.pack...@intel.com pgp0JA2d5da1B.pgp Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] Pack swap complete bits into an XEvent
On Thu, 28 Apr 2011 13:27:19 -0700, Jesse Barnes wrote: > The defintion of the swap complete event was wrong; XEvents are only 32 > bytes long, and with padding the swap event was longer. So use some > creative packing to get all the bits we want transmitted. Requires a > proto version bump. If you've got a proto version on both sides, you should just stick the low 32 bits in place of the event type and pad fields, rather than abusing. Yes, this requires two different structures in the X server, but you need to do that anyways -- the event type has to shrink from 16 bits to 8 bits, moving it at the same time is not a problem. Besides, this will make the xcb case really ugly as xcb doesn't hide the protocol event structures from the application. -- keith.packard at intel.com -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 189 bytes Desc: not available URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20110428/1bacbf2d/attachment.pgp>
Re: [PATCH] DRI2/GLX: fix swap event handling
On Thu, 28 Apr 2011 13:27:22 -0700, Jesse Barnes wrote: > Use the new swap event structure packing and send it to the client if > possible. This means tracking client version information when clients > connect. If they don't support the new packing, they'll get the old > bits and fill junk into their sbc values when they receive the event. > If they do support the new packing, send off the right data. > --- a/glx/glxdri2.c > +++ b/glx/glxdri2.c > @@ -192,8 +192,17 @@ __glXdriSwapEvent(ClientPtr client, void *data, int > type, CARD64 ust, > wire.ust_lo = ust & 0x; > wire.msc_hi = msc >> 32; > wire.msc_lo = msc & 0x; > -wire.sbc_hi = sbc >> 32; > -wire.sbc_lo = sbc & 0x; > +wire.sbc_hi = 0; was that supposed to be wire.sbc_lo and not whacking wire.sbc_hi? At first I was confused by this whole thing -- why not rearrange the structure a bit if we're messing things up? Then I realized that this let the server emit the mostly-the-same event structure and only steal the other half of event_type for clients that understand the new 8-bit event_type protocol. Seems like a reasonable approach. pgp81Hy5vnS1L.pgp Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] DRI2/GLX: fix swap event handling
On Thu, 28 Apr 2011 13:27:22 -0700, Jesse Barnes wrote: > Use the new swap event structure packing and send it to the client if > possible. This means tracking client version information when clients > connect. If they don't support the new packing, they'll get the old > bits and fill junk into their sbc values when they receive the event. > If they do support the new packing, send off the right data. > --- a/glx/glxdri2.c > +++ b/glx/glxdri2.c > @@ -192,8 +192,17 @@ __glXdriSwapEvent(ClientPtr client, void *data, int > type, CARD64 ust, > wire.ust_lo = ust & 0x; > wire.msc_hi = msc >> 32; > wire.msc_lo = msc & 0x; > -wire.sbc_hi = sbc >> 32; > -wire.sbc_lo = sbc & 0x; > +wire.sbc_hi = 0; was that supposed to be wire.sbc_lo and not whacking wire.sbc_hi? At first I was confused by this whole thing -- why not rearrange the structure a bit if we're messing things up? Then I realized that this let the server emit the mostly-the-same event structure and only steal the other half of event_type for clients that understand the new 8-bit event_type protocol. Seems like a reasonable approach. -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 197 bytes Desc: not available URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20110428/a42fc0d8/attachment-0001.pgp>
Re: [PATCH] Pack swap complete bits into an XEvent
On Thu, 28 Apr 2011 14:33:58 -0700
Eric Anholt wrote:
> On Thu, 28 Apr 2011 13:27:19 -0700, Jesse Barnes
> wrote:
> > The defintion of the swap complete event was wrong; XEvents are only 32
> > bytes long, and with padding the swap event was longer. So use some
> > creative packing to get all the bits we want transmitted. Requires a
> > proto version bump.
> > ---
> > configure.ac |2 +-
> > glxproto.h | 13 +
> > 2 files changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index d88e6df..a3047e4 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -1,5 +1,5 @@
> > AC_PREREQ([2.60])
> > -AC_INIT([GLProto], [1.4.12],
> > [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg])
> > +AC_INIT([GLProto], [1.4.13],
> > [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg])
> > AM_INIT_AUTOMAKE([foreign dist-bzip2])
> > AM_MAINTAINER_MODE
> >
> > diff --git a/glxproto.h b/glxproto.h
> > index 0ff44e3..4a583c1 100644
> > --- a/glxproto.h
> > +++ b/glxproto.h
> > @@ -1370,18 +1370,23 @@ typedef struct {
> > CARD32 unused2 B32;
> > } xGLXPbufferClobberEvent;
> >
> > +/* Note, this struct is too large for an Xevent, I fail -- jbarnes
> > + * So sbc_lo won't ever be sent. We can use a generic event though without
> > + * size restrictions, thus xGLXBufferSwapComplete2.
> > + */
>
> This comment doesn't seem to match the change.
double fail. will fix.
--
Jesse Barnes, Intel Open Source Technology Center
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] Pack swap complete bits into an XEvent
On Thu, 28 Apr 2011 14:33:58 -0700
Eric Anholt wrote:
> On Thu, 28 Apr 2011 13:27:19 -0700, Jesse Barnes virtuousgeek.org> wrote:
> > The defintion of the swap complete event was wrong; XEvents are only 32
> > bytes long, and with padding the swap event was longer. So use some
> > creative packing to get all the bits we want transmitted. Requires a
> > proto version bump.
> > ---
> > configure.ac |2 +-
> > glxproto.h | 13 +
> > 2 files changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index d88e6df..a3047e4 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -1,5 +1,5 @@
> > AC_PREREQ([2.60])
> > -AC_INIT([GLProto], [1.4.12],
> > [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg])
> > +AC_INIT([GLProto], [1.4.13],
> > [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg])
> > AM_INIT_AUTOMAKE([foreign dist-bzip2])
> > AM_MAINTAINER_MODE
> >
> > diff --git a/glxproto.h b/glxproto.h
> > index 0ff44e3..4a583c1 100644
> > --- a/glxproto.h
> > +++ b/glxproto.h
> > @@ -1370,18 +1370,23 @@ typedef struct {
> > CARD32 unused2 B32;
> > } xGLXPbufferClobberEvent;
> >
> > +/* Note, this struct is too large for an Xevent, I fail -- jbarnes
> > + * So sbc_lo won't ever be sent. We can use a generic event though without
> > + * size restrictions, thus xGLXBufferSwapComplete2.
> > + */
>
> This comment doesn't seem to match the change.
double fail. will fix.
--
Jesse Barnes, Intel Open Source Technology Center
Re: [PATCH] Pack swap complete bits into an XEvent
On Thu, 28 Apr 2011 13:27:19 -0700, Jesse Barnes
wrote:
> The defintion of the swap complete event was wrong; XEvents are only 32
> bytes long, and with padding the swap event was longer. So use some
> creative packing to get all the bits we want transmitted. Requires a
> proto version bump.
> ---
> configure.ac |2 +-
> glxproto.h | 13 +
> 2 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index d88e6df..a3047e4 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1,5 +1,5 @@
> AC_PREREQ([2.60])
> -AC_INIT([GLProto], [1.4.12],
> [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg])
> +AC_INIT([GLProto], [1.4.13],
> [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg])
> AM_INIT_AUTOMAKE([foreign dist-bzip2])
> AM_MAINTAINER_MODE
>
> diff --git a/glxproto.h b/glxproto.h
> index 0ff44e3..4a583c1 100644
> --- a/glxproto.h
> +++ b/glxproto.h
> @@ -1370,18 +1370,23 @@ typedef struct {
> CARD32 unused2 B32;
> } xGLXPbufferClobberEvent;
>
> +/* Note, this struct is too large for an Xevent, I fail -- jbarnes
> + * So sbc_lo won't ever be sent. We can use a generic event though without
> + * size restrictions, thus xGLXBufferSwapComplete2.
> + */
This comment doesn't seem to match the change.
> typedef struct {
> BYTE type;
> -BYTE pad;
> +BYTE sbc_lo0;
> CARD16 sequenceNumber B16;
> -CARD16 event_type B16;
> -CARD32 drawable;
> +CARD8 event_type;
> +CARD8 sbc_lo8;
> +CARD16 sbc_lo16 B16;
> +CARD32 drawable B32;
> CARD32 ust_hi B32;
> CARD32 ust_lo B32;
> CARD32 msc_hi B32;
> CARD32 msc_lo B32;
> CARD32 sbc_hi B32;
> -CARD32 sbc_lo B32;
> } xGLXBufferSwapComplete;
pgpz2tVrK7Muj.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] Pack swap complete bits into an XEvent
On Thu, 28 Apr 2011 13:27:19 -0700, Jesse Barnes
wrote:
> The defintion of the swap complete event was wrong; XEvents are only 32
> bytes long, and with padding the swap event was longer. So use some
> creative packing to get all the bits we want transmitted. Requires a
> proto version bump.
> ---
> configure.ac |2 +-
> glxproto.h | 13 +
> 2 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index d88e6df..a3047e4 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1,5 +1,5 @@
> AC_PREREQ([2.60])
> -AC_INIT([GLProto], [1.4.12],
> [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg])
> +AC_INIT([GLProto], [1.4.13],
> [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg])
> AM_INIT_AUTOMAKE([foreign dist-bzip2])
> AM_MAINTAINER_MODE
>
> diff --git a/glxproto.h b/glxproto.h
> index 0ff44e3..4a583c1 100644
> --- a/glxproto.h
> +++ b/glxproto.h
> @@ -1370,18 +1370,23 @@ typedef struct {
> CARD32 unused2 B32;
> } xGLXPbufferClobberEvent;
>
> +/* Note, this struct is too large for an Xevent, I fail -- jbarnes
> + * So sbc_lo won't ever be sent. We can use a generic event though without
> + * size restrictions, thus xGLXBufferSwapComplete2.
> + */
This comment doesn't seem to match the change.
> typedef struct {
> BYTE type;
> -BYTE pad;
> +BYTE sbc_lo0;
> CARD16 sequenceNumber B16;
> -CARD16 event_type B16;
> -CARD32 drawable;
> +CARD8 event_type;
> +CARD8 sbc_lo8;
> +CARD16 sbc_lo16 B16;
> +CARD32 drawable B32;
> CARD32 ust_hi B32;
> CARD32 ust_lo B32;
> CARD32 msc_hi B32;
> CARD32 msc_lo B32;
> CARD32 sbc_hi B32;
> -CARD32 sbc_lo B32;
> } xGLXBufferSwapComplete;
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL:
<http://lists.freedesktop.org/archives/dri-devel/attachments/20110428/224366ee/attachment.pgp>
Re: [PATCH 1/3] drm: Send pending vblank events before disabling vblank.
On Thu, 28 Apr 2011 13:46:30 -0700 Jesse Barnes wrote: > On Thu, 28 Apr 2011 18:09:58 +1000 > Christopher James Halse Rogers > wrote: > > Ok. This appears to be surprisingly difficult. Particularly, the > > vblank code indexes crtcs based on the driver-private numbering, and > > there doesn't appear to be a generic interface to grab this number; > > Intel uses the DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID ioctl, radeon uses > > something different. > > > > I'll see what I can come up with, but I don't think I'm sufficiently > > familiar with the kms code to quickly come up with a nice API. > > Yeah, unfortunately the vblank code pre-dates the KMS code by quite a > bit, so the IDs don't match. > > But with the events emitted as in your previous patch, this should be a > harder case to hit, but it sounds like we need to make sure the flip > waits are dealt with too to avoid this from triggering at all on > Intel. Or did you see other cases where the refcount was nonzero at > this point? Actually it looks like we wait for any outstanding flips before disabling so that seems correct (though I didn't check the locking, it's possible we could race a completion and a new flip if the flip ioctl and the dpms code don't exclude each other). -- Jesse Barnes, Intel Open Source Technology Center ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/3] drm: Send pending vblank events before disabling vblank.
On Thu, 28 Apr 2011 13:46:30 -0700 Jesse Barnes wrote: > On Thu, 28 Apr 2011 18:09:58 +1000 > Christopher James Halse Rogers > wrote: > > Ok. This appears to be surprisingly difficult. Particularly, the > > vblank code indexes crtcs based on the driver-private numbering, and > > there doesn't appear to be a generic interface to grab this number; > > Intel uses the DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID ioctl, radeon uses > > something different. > > > > I'll see what I can come up with, but I don't think I'm sufficiently > > familiar with the kms code to quickly come up with a nice API. > > Yeah, unfortunately the vblank code pre-dates the KMS code by quite a > bit, so the IDs don't match. > > But with the events emitted as in your previous patch, this should be a > harder case to hit, but it sounds like we need to make sure the flip > waits are dealt with too to avoid this from triggering at all on > Intel. Or did you see other cases where the refcount was nonzero at > this point? Actually it looks like we wait for any outstanding flips before disabling so that seems correct (though I didn't check the locking, it's possible we could race a completion and a new flip if the flip ioctl and the dpms code don't exclude each other). -- Jesse Barnes, Intel Open Source Technology Center
Re: [PATCH 1/3] drm: Send pending vblank events before disabling vblank.
On Thu, 28 Apr 2011 18:09:58 +1000 Christopher James Halse Rogers wrote: > Ok. This appears to be surprisingly difficult. Particularly, the > vblank code indexes crtcs based on the driver-private numbering, and > there doesn't appear to be a generic interface to grab this number; > Intel uses the DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID ioctl, radeon uses > something different. > > I'll see what I can come up with, but I don't think I'm sufficiently > familiar with the kms code to quickly come up with a nice API. Yeah, unfortunately the vblank code pre-dates the KMS code by quite a bit, so the IDs don't match. But with the events emitted as in your previous patch, this should be a harder case to hit, but it sounds like we need to make sure the flip waits are dealt with too to avoid this from triggering at all on Intel. Or did you see other cases where the refcount was nonzero at this point? Thanks, -- Jesse Barnes, Intel Open Source Technology Center ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/3] drm: Send pending vblank events before disabling vblank.
On Thu, 28 Apr 2011 18:09:58 +1000 Christopher James Halse Rogers wrote: > Ok. This appears to be surprisingly difficult. Particularly, the > vblank code indexes crtcs based on the driver-private numbering, and > there doesn't appear to be a generic interface to grab this number; > Intel uses the DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID ioctl, radeon uses > something different. > > I'll see what I can come up with, but I don't think I'm sufficiently > familiar with the kms code to quickly come up with a nice API. Yeah, unfortunately the vblank code pre-dates the KMS code by quite a bit, so the IDs don't match. But with the events emitted as in your previous patch, this should be a harder case to hit, but it sounds like we need to make sure the flip waits are dealt with too to avoid this from triggering at all on Intel. Or did you see other cases where the refcount was nonzero at this point? Thanks, -- Jesse Barnes, Intel Open Source Technology Center
Re: [PATCH 1/3] drm: Send pending vblank events before disabling vblank.
On Wed, 27 Apr 2011 10:32:36 +0200 Michel Dänzer wrote: > On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rog...@canonical.com > wrote: > > From: Christopher James Halse Rogers > > > > > > This is the least-bad behaviour. It means that we signal the > > vblank event before it actually happens, but since we're disabling > > vblanks there's no guarantee that it will *ever* happen otherwise. > > This may indeed be the best we can do for events that are pending when > the CRTC is disabled[0], but I can't see anything that would prevent new > events from getting scheduled (or synchronous vblank waits from timing > out) while the CRTC is disabled? > > [0] Though it might unnecessarily send events prematurely when the CRTC > is just disabled temporarily, e.g. as part of a modeset. We should return -EINVAL in that case from drm_wait_vblank due to drm_vblank_get failing (i.e. the driver enable_vblank hook should fail if the corresponding crtc is off). At least that's how it's supposed to work. -- Jesse Barnes, Intel Open Source Technology Center ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/3] drm: Send pending vblank events before disabling vblank.
On Wed, 27 Apr 2011 10:32:36 +0200 Michel D?nzer wrote: > On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers at canonical.com > wrote: > > From: Christopher James Halse Rogers > canonical.com> > > > > This is the least-bad behaviour. It means that we signal the > > vblank event before it actually happens, but since we're disabling > > vblanks there's no guarantee that it will *ever* happen otherwise. > > This may indeed be the best we can do for events that are pending when > the CRTC is disabled[0], but I can't see anything that would prevent new > events from getting scheduled (or synchronous vblank waits from timing > out) while the CRTC is disabled? > > [0] Though it might unnecessarily send events prematurely when the CRTC > is just disabled temporarily, e.g. as part of a modeset. We should return -EINVAL in that case from drm_wait_vblank due to drm_vblank_get failing (i.e. the driver enable_vblank hook should fail if the corresponding crtc is off). At least that's how it's supposed to work. -- Jesse Barnes, Intel Open Source Technology Center
Re: [PATCH 3/3] drm: Factor-out drm_emit_vblank_event code.
On Wed, 27 Apr 2011 16:10:59 +1000 christopher.halse.rog...@canonical.com wrote: > From: Christopher James Halse Rogers > > Signed-off-by: Christopher James Halse Rogers > > --- > drivers/gpu/drm/drm_irq.c | 39 --- > 1 files changed, 16 insertions(+), 23 deletions(-) > Oh I see you already addressed my last comment. :) I think Michel is right about the put; the event queue holds the ref and the emit should drop it, so keeping them together seems good to me. Other than that, Reviewed-by: Jesse Barnes same for the first patch now that you've addressed my cleanup issue. -- Jesse Barnes, Intel Open Source Technology Center ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 3/3] drm: Factor-out drm_emit_vblank_event code.
On Wed, 27 Apr 2011 16:10:59 +1000 christopher.halse.rogers at canonical.com wrote: > From: Christopher James Halse Rogers canonical.com> > > Signed-off-by: Christopher James Halse Rogers canonical.com> > --- > drivers/gpu/drm/drm_irq.c | 39 --- > 1 files changed, 16 insertions(+), 23 deletions(-) > Oh I see you already addressed my last comment. :) I think Michel is right about the put; the event queue holds the ref and the emit should drop it, so keeping them together seems good to me. Other than that, Reviewed-by: Jesse Barnes same for the first patch now that you've addressed my cleanup issue. -- Jesse Barnes, Intel Open Source Technology Center
Re: [PATCH 1/3] drm: Send pending vblank events before disabling vblank.
On Wed, 27 Apr 2011 16:10:57 +1000
christopher.halse.rog...@canonical.com wrote:
> From: Christopher James Halse Rogers
>
> This is the least-bad behaviour. It means that we signal the
> vblank event before it actually happens, but since we're disabling
> vblanks there's no guarantee that it will *ever* happen otherwise.
>
> This prevents GL applications which use WaitMSC from hanging
> indefinitely.
>
> Signed-off-by: Christopher James Halse Rogers
>
> ---
> drivers/gpu/drm/drm_irq.c | 23 +++
> 1 files changed, 23 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 741457b..a1f12cb 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -932,11 +932,34 @@ EXPORT_SYMBOL(drm_vblank_put);
>
> void drm_vblank_off(struct drm_device *dev, int crtc)
> {
> + struct drm_pending_vblank_event *e, *t;
> + struct timeval now;
> unsigned long irqflags;
> + unsigned int seq;
>
> spin_lock_irqsave(&dev->vbl_lock, irqflags);
> vblank_disable_and_save(dev, crtc);
> DRM_WAKEUP(&dev->vbl_queue[crtc]);
> +
> + /* Send any queued vblank events, lest the natives grow disquiet */
> + seq = drm_vblank_count_and_time(dev, crtc, &now);
> + list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) {
> + if (e->pipe != crtc)
> + continue;
> + DRM_DEBUG("Sending premature vblank event on disable: \
> + wanted %d, current %d\n",
> + e->event.sequence, seq);
> +
> + e->event.sequence = seq;
> + e->event.tv_sec = now.tv_sec;
> + e->event.tv_usec = now.tv_usec;
> + drm_vblank_put(dev, e->pipe);
> + list_move_tail(&e->base.link, &e->base.file_priv->event_list);
> + wake_up_interruptible(&e->base.file_priv->event_wait);
> + trace_drm_vblank_event_delivered(e->base.pid, e->pipe,
> + e->event.sequence);
> + }
> +
> spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> }
> EXPORT_SYMBOL(drm_vblank_off);
Yeah, this matches what we do for the blocking waits, and it's probably
a good idea. Userspace can decide what to do with apps running against
a disabled CRTC.
Would be nice to share the code with drm_handle_vblank_events though
somehow. Looks like
e->event.sequence = seq;
e->event.tv_sec = now.tv_sec;
e->event.tv_usec = now.tv_usec;
drm_vblank_put(dev, e->pipe);
list_move_tail(&e->base.link, &e->base.file_priv->event_list);
wake_up_interruptible(&e->base.file_priv->event_wait);
trace_drm_vblank_event_delivered(e->base.pid, e->pipe,
e->event.sequence);
could be pulled out into a separate function for both to use.
--
Jesse Barnes, Intel Open Source Technology Center
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/3] drm: Send pending vblank events before disabling vblank.
On Wed, 27 Apr 2011 16:10:57 +1000
christopher.halse.rogers at canonical.com wrote:
> From: Christopher James Halse Rogers canonical.com>
>
> This is the least-bad behaviour. It means that we signal the
> vblank event before it actually happens, but since we're disabling
> vblanks there's no guarantee that it will *ever* happen otherwise.
>
> This prevents GL applications which use WaitMSC from hanging
> indefinitely.
>
> Signed-off-by: Christopher James Halse Rogers canonical.com>
> ---
> drivers/gpu/drm/drm_irq.c | 23 +++
> 1 files changed, 23 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 741457b..a1f12cb 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -932,11 +932,34 @@ EXPORT_SYMBOL(drm_vblank_put);
>
> void drm_vblank_off(struct drm_device *dev, int crtc)
> {
> + struct drm_pending_vblank_event *e, *t;
> + struct timeval now;
> unsigned long irqflags;
> + unsigned int seq;
>
> spin_lock_irqsave(&dev->vbl_lock, irqflags);
> vblank_disable_and_save(dev, crtc);
> DRM_WAKEUP(&dev->vbl_queue[crtc]);
> +
> + /* Send any queued vblank events, lest the natives grow disquiet */
> + seq = drm_vblank_count_and_time(dev, crtc, &now);
> + list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) {
> + if (e->pipe != crtc)
> + continue;
> + DRM_DEBUG("Sending premature vblank event on disable: \
> + wanted %d, current %d\n",
> + e->event.sequence, seq);
> +
> + e->event.sequence = seq;
> + e->event.tv_sec = now.tv_sec;
> + e->event.tv_usec = now.tv_usec;
> + drm_vblank_put(dev, e->pipe);
> + list_move_tail(&e->base.link, &e->base.file_priv->event_list);
> + wake_up_interruptible(&e->base.file_priv->event_wait);
> + trace_drm_vblank_event_delivered(e->base.pid, e->pipe,
> + e->event.sequence);
> + }
> +
> spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> }
> EXPORT_SYMBOL(drm_vblank_off);
Yeah, this matches what we do for the blocking waits, and it's probably
a good idea. Userspace can decide what to do with apps running against
a disabled CRTC.
Would be nice to share the code with drm_handle_vblank_events though
somehow. Looks like
e->event.sequence = seq;
e->event.tv_sec = now.tv_sec;
e->event.tv_usec = now.tv_usec;
drm_vblank_put(dev, e->pipe);
list_move_tail(&e->base.link, &e->base.file_priv->event_list);
wake_up_interruptible(&e->base.file_priv->event_wait);
trace_drm_vblank_event_delivered(e->base.pid, e->pipe,
e->event.sequence);
could be pulled out into a separate function for both to use.
--
Jesse Barnes, Intel Open Source Technology Center
[PATCH] DRI2/GLX: fix swap event handling
Use the new swap event structure packing and send it to the client if
possible. This means tracking client version information when clients
connect. If they don't support the new packing, they'll get the old
bits and fill junk into their sbc values when they receive the event.
If they do support the new packing, send off the right data.
---
configure.ac|4 +-
glx/glxdri2.c | 28 +++-
hw/xfree86/dri2/dri2.c | 57 ++-
hw/xfree86/dri2/dri2ext.c | 14 ++-
include/protocol-versions.h |2 +-
5 files changed, 98 insertions(+), 7 deletions(-)
diff --git a/configure.ac b/configure.ac
index 6eb780c..87194a5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -771,11 +771,11 @@ RECORDPROTO="recordproto >= 1.13.99.1"
SCRNSAVERPROTO="scrnsaverproto >= 1.1"
RESOURCEPROTO="resourceproto"
DRIPROTO="xf86driproto >= 2.1.0"
-DRI2PROTO="dri2proto >= 2.3"
+DRI2PROTO="dri2proto >= 2.4"
XINERAMAPROTO="xineramaproto"
BIGFONTPROTO="xf86bigfontproto >= 1.2.0"
DGAPROTO="xf86dgaproto >= 2.0.99.1"
-GLPROTO="glproto >= 1.4.10"
+GLPROTO="glproto >= 1.4.13"
DMXPROTO="dmxproto >= 2.2.99.1"
VIDMODEPROTO="xf86vidmodeproto >= 2.2.99.1"
WINDOWSWMPROTO="windowswmproto"
diff --git a/glx/glxdri2.c b/glx/glxdri2.c
index d979717..654b4ae 100644
--- a/glx/glxdri2.c
+++ b/glx/glxdri2.c
@@ -192,8 +192,17 @@ __glXdriSwapEvent(ClientPtr client, void *data, int type,
CARD64 ust,
wire.ust_lo = ust & 0x;
wire.msc_hi = msc >> 32;
wire.msc_lo = msc & 0x;
-wire.sbc_hi = sbc >> 32;
-wire.sbc_lo = sbc & 0x;
+wire.sbc_hi = 0;
+
+if (DRI2ClientSupportsSBC(client)) {
+ wire.sbc_lo0 = sbc & 0xff;
+ wire.sbc_lo8 = (sbc >> 8) & 0xff;
+ wire.sbc_lo16 = (sbc >> 16) & 0xff;
+} else {
+ wire.sbc_lo0 = 0;
+ wire.sbc_lo8 = 0;
+ wire.sbc_lo16 = 0;
+}
WriteEventsToClient(client, 1, (xEvent *) &wire);
}
@@ -228,6 +237,18 @@ __glXDRIdrawableSwapBuffers(ClientPtr client,
__GLXdrawable *drawable)
return TRUE;
}
+static void
+__glXDRIclientGone(CallbackListPtr *list,
+ pointer closure,
+ pointer data)
+{
+NewClientInfoRec *clientinfo = (NewClientInfoRec *) data;
+ClientPtr pClient = clientinfo->client;
+
+if (pClient->clientState == ClientStateGone)
+ DRI2ClientGone(client);
+}
+
static int
__glXDRIdrawableSwapInterval(__GLXdrawable *drawable, int interval)
{
@@ -769,6 +790,9 @@ __glXDRIscreenProbe(ScreenPtr pScreen)
screen->leaveVT = pScrn->LeaveVT;
pScrn->LeaveVT = glxDRILeaveVT;
+if (!AddCallback (&ClientStateCallback, __glXDRIclientGone, 0))
+ return;
+
LogMessage(X_INFO,
"AIGLX: Loaded and initialized %s\n", driverName);
diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index 5c42a51..9b5eab2 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -60,6 +60,9 @@ static DevPrivateKeyRec dri2WindowPrivateKeyRec;
static DevPrivateKeyRec dri2PixmapPrivateKeyRec;
#define dri2PixmapPrivateKey (&dri2PixmapPrivateKeyRec)
+static DevPrivateKeyRec dri2ClientPrivateKeyRec;
+#define dri2ClientPrivateKey (&dri2ClientPrivateKeyRec)
+
static RESTYPE dri2DrawableRes;
typedef struct _DRI2Screen *DRI2ScreenPtr;
@@ -107,6 +110,11 @@ typedef struct _DRI2Screen {
ConfigNotifyProcPtr ConfigNotify;
} DRI2ScreenRec;
+typedef struct _DRI2Client {
+CARD32 major;
+CARD32 minor;
+} DRI2ClientRec, *DRI2ClientPtr;
+
static DRI2ScreenPtr
DRI2GetScreen(ScreenPtr pScreen)
{
@@ -131,6 +139,12 @@ DRI2GetDrawable(DrawablePtr pDraw)
}
}
+static DRI2ClientPtr
+DRI2GetClient(ClientPtr client)
+{
+return dixLookupPrivate(&client->devPrivates, dri2ClientPrivateKey);
+}
+
static unsigned long
DRI2DrawableSerial(DrawablePtr pDraw)
{
@@ -190,6 +204,44 @@ DRI2AllocateDrawable(DrawablePtr pDraw)
return pPriv;
}
+void
+DRI2InitClient(ClientPtr client, CARD32 major, CARD32 minor)
+{
+DRI2ClientPtr pPriv;
+
+pPriv = malloc(sizeof *pPriv);
+if (!pPriv)
+ return;
+
+pPriv->major = major;
+pPriv->minor = minor;
+
+dixSetPrivate(&client->devPrivates, dri2ClientPrivateKey, pPriv);
+}
+
+void
+DRI2ClientGone(ClientPtr client)
+{
+DRI2ClientPtr pPriv = DRI2GetClient(client);
+
+if (pPriv)
+ free(pPriv);
+}
+
+Bool
+DRI2ClientSupportsSBC(ClientPtr client)
+{
+DRI2ClientPtr pPriv = DRI2GetClient(client);
+
+if (!pPriv)
+ return FALSE;
+
+if (pPriv->major > 1 || (pPriv->major == 1 && pPriv->minor > 3))
+ return TRUE;
+
+return FALSE;
+}
+
typedef struct DRI2DrawableRefRec {
XID id;
XID dri2_id;
@@ -1097,6 +1149,9 @@ DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info)
if (!dixRegisterPrivateKey(&dri2PixmapPrivateKeyRec, PRIVATE_PIXMAP, 0))
[RFC] swap event handling fixes
I obviously failed to count the swap event structure size after adding and removing fields a few times, and didn't even account for padding. The end result is that clients today won't receive the sbc_lo field at all, and so will likely stuff junk into that field on the client side (or zero at best). This patchset fixes up the structure definitions, bumps the proto levels, and adds server and client handling code for it all. It should be forward and backward compatible, but as always review and testing appreciated. I think the event_type checking on the client side still needs work; the field is split now so I need to check the right one on old servers. I'll also add swap support for the new requests in case people ever want to run this stuff between big and little endian machines. Thanks, Jesse ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] Fix swap complete event size
XEvents are limited to 32 bytes, so use some creative stuffing to fit
all the bits we'd like to supply.
---
configure.ac |2 +-
dri2proto.h |9 +
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/configure.ac b/configure.ac
index 5b78d6b..9505f56 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1,5 +1,5 @@
AC_PREREQ([2.60])
-AC_INIT([DRI2Proto], [2.3],
[https://bugs.freedesktop.org/enter_bug.cgi?product=xorg])
+AC_INIT([DRI2Proto], [2.4],
[https://bugs.freedesktop.org/enter_bug.cgi?product=xorg])
AM_INIT_AUTOMAKE([foreign dist-bzip2])
AM_MAINTAINER_MODE
diff --git a/dri2proto.h b/dri2proto.h
index 9708a4a..e568c91 100644
--- a/dri2proto.h
+++ b/dri2proto.h
@@ -35,7 +35,7 @@
#define DRI2_NAME "DRI2"
#define DRI2_MAJOR 1
-#define DRI2_MINOR 3
+#define DRI2_MINOR 4
#define DRI2NumberErrors 0
#define DRI2NumberEvents 2
@@ -287,16 +287,17 @@ typedef struct {
typedef struct {
CARD8 type;
-CARD8 pad;
+CARD8 sbc_lo0;
CARD16 sequenceNumber B16;
-CARD16 event_type B16;
+CARD8 event_type;
+CARD8 sbc_lo8;
+CARD16 sbc_lo16 B16;
CARD32 drawable B32;
CARD32 ust_hi B32;
CARD32 ust_lo B32;
CARD32 msc_hi B32;
CARD32 msc_lo B32;
CARD32 sbc_hi B32;
-CARD32 sbc_lo B32;
} xDRI2BufferSwapComplete;
#define sz_xDRI2BufferSwapComplete 32
--
1.7.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] GLX/DRI2: fix swap complete handling
The swap complete event wasn't being handled fully; because XEvents are
only 32 bytes long, we were getting junk for the sbc_lo value. If the
server supports it, unpack the new structure, otherwise just return 0
for the sbc value instead of garbage.
---
configure.ac|4 ++--
src/glx/dri2.c |7 ++-
src/glx/dri2_glx.c | 13 +
src/glx/glxclient.h |1 +
src/glx/glxext.c|8 +++-
5 files changed, 29 insertions(+), 4 deletions(-)
diff --git a/configure.ac b/configure.ac
index 3b05ca3..94fb6f7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -21,8 +21,8 @@ dnl Versions for external dependencies
LIBDRM_REQUIRED=2.4.24
LIBDRM_RADEON_REQUIRED=2.4.24
LIBDRM_INTEL_REQUIRED=2.4.24
-DRI2PROTO_REQUIRED=2.1
-GLPROTO_REQUIRED=1.4.11
+DRI2PROTO_REQUIRED=2.4
+GLPROTO_REQUIRED=1.4.13
LIBDRM_XORG_REQUIRED=2.4.24
LIBKMS_XORG_REQUIRED=1.0.0
diff --git a/src/glx/dri2.c b/src/glx/dri2.c
index adfd3d1..864c941 100644
--- a/src/glx/dri2.c
+++ b/src/glx/dri2.c
@@ -124,7 +124,12 @@ DRI2WireToEvent(Display *dpy, XEvent *event, xEvent *wire)
}
aevent->ust = ((CARD64)awire->ust_hi << 32) | awire->ust_lo;
aevent->msc = ((CARD64)awire->msc_hi << 32) | awire->msc_lo;
- aevent->sbc = ((CARD64)awire->sbc_hi << 32) | awire->sbc_lo;
+ if (dri2ServerSupportsSBC(dpy)) {
+aevent->sbc = ((CARD64)awire->sbc_hi << 32) | (awire->sbc_lo16 << 16) |
+ (awire->sbc_lo8 << 8) | awire->sbc_lo0;
+ } else {
+aevent->sbc = 0;
+ }
return True;
}
#endif
diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
index fc0237a..be96ec6 100644
--- a/src/glx/dri2_glx.c
+++ b/src/glx/dri2_glx.c
@@ -791,6 +791,19 @@ static const struct glx_screen_vtable dri2_screen_vtable =
{
dri2_create_context
};
+int
+dri2ServerSupportsSBC(Display *dpy)
+{
+ struct glx_display *dpyPriv = __glXInitialize(dpy);
+ struct dri2_display *pdp =
+ (struct dri2_display *) dpyPriv->dri2Display;
+
+ if (pdp->driMajor > 1 || (pdp->driMajor == 1 && pdp->driMinor > 3))
+ return 1;
+
+ return 0;
+}
+
static struct glx_screen *
dri2CreateScreen(int screen, struct glx_display * priv)
{
diff --git a/src/glx/glxclient.h b/src/glx/glxclient.h
index 2b6966f..acba82c 100644
--- a/src/glx/glxclient.h
+++ b/src/glx/glxclient.h
@@ -149,6 +149,7 @@ extern __GLXDRIdisplay *driCreateDisplay(Display * dpy);
extern __GLXDRIdisplay *dri2CreateDisplay(Display * dpy);
extern void dri2InvalidateBuffers(Display *dpy, XID drawable);
extern unsigned dri2GetSwapEventType(Display *dpy, XID drawable);
+extern int dri2ServerSupportsSBC(Display *dpy);
/*
diff --git a/src/glx/glxext.c b/src/glx/glxext.c
index 278c719..7393efc 100644
--- a/src/glx/glxext.c
+++ b/src/glx/glxext.c
@@ -138,7 +138,13 @@ __glXWireToEvent(Display *dpy, XEvent *event, xEvent *wire)
aevent->drawable = awire->drawable;
aevent->ust = ((CARD64)awire->ust_hi << 32) | awire->ust_lo;
aevent->msc = ((CARD64)awire->msc_hi << 32) | awire->msc_lo;
- aevent->sbc = ((CARD64)awire->sbc_hi << 32) | awire->sbc_lo;
+ if (glx_dpy->majorVersion > 1 || (glx_dpy->majorVersion == 1 &&
+ glx_dpy->minorVersion > 4)) {
+aevent->sbc = ((CARD64)awire->sbc_hi << 32) | (awire->sbc_lo16 << 16) |
+ (awire->sbc_lo8 << 8) | awire->sbc_lo0;
+ } else {
+aevent->sbc = 0;
+ }
return True;
}
default:
--
1.7.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] Pack swap complete bits into an XEvent
The defintion of the swap complete event was wrong; XEvents are only 32
bytes long, and with padding the swap event was longer. So use some
creative packing to get all the bits we want transmitted. Requires a
proto version bump.
---
configure.ac |2 +-
glxproto.h | 13 +
2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/configure.ac b/configure.ac
index d88e6df..a3047e4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1,5 +1,5 @@
AC_PREREQ([2.60])
-AC_INIT([GLProto], [1.4.12],
[https://bugs.freedesktop.org/enter_bug.cgi?product=xorg])
+AC_INIT([GLProto], [1.4.13],
[https://bugs.freedesktop.org/enter_bug.cgi?product=xorg])
AM_INIT_AUTOMAKE([foreign dist-bzip2])
AM_MAINTAINER_MODE
diff --git a/glxproto.h b/glxproto.h
index 0ff44e3..4a583c1 100644
--- a/glxproto.h
+++ b/glxproto.h
@@ -1370,18 +1370,23 @@ typedef struct {
CARD32 unused2 B32;
} xGLXPbufferClobberEvent;
+/* Note, this struct is too large for an Xevent, I fail -- jbarnes
+ * So sbc_lo won't ever be sent. We can use a generic event though without
+ * size restrictions, thus xGLXBufferSwapComplete2.
+ */
typedef struct {
BYTE type;
-BYTE pad;
+BYTE sbc_lo0;
CARD16 sequenceNumber B16;
-CARD16 event_type B16;
-CARD32 drawable;
+CARD8 event_type;
+CARD8 sbc_lo8;
+CARD16 sbc_lo16 B16;
+CARD32 drawable B32;
CARD32 ust_hi B32;
CARD32 ust_lo B32;
CARD32 msc_hi B32;
CARD32 msc_lo B32;
CARD32 sbc_hi B32;
-CARD32 sbc_lo B32;
} xGLXBufferSwapComplete;
//
--
1.7.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] DRI2/GLX: fix swap event handling
Use the new swap event structure packing and send it to the client if
possible. This means tracking client version information when clients
connect. If they don't support the new packing, they'll get the old
bits and fill junk into their sbc values when they receive the event.
If they do support the new packing, send off the right data.
---
configure.ac|4 +-
glx/glxdri2.c | 28 +++-
hw/xfree86/dri2/dri2.c | 57 ++-
hw/xfree86/dri2/dri2ext.c | 14 ++-
include/protocol-versions.h |2 +-
5 files changed, 98 insertions(+), 7 deletions(-)
diff --git a/configure.ac b/configure.ac
index 6eb780c..87194a5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -771,11 +771,11 @@ RECORDPROTO="recordproto >= 1.13.99.1"
SCRNSAVERPROTO="scrnsaverproto >= 1.1"
RESOURCEPROTO="resourceproto"
DRIPROTO="xf86driproto >= 2.1.0"
-DRI2PROTO="dri2proto >= 2.3"
+DRI2PROTO="dri2proto >= 2.4"
XINERAMAPROTO="xineramaproto"
BIGFONTPROTO="xf86bigfontproto >= 1.2.0"
DGAPROTO="xf86dgaproto >= 2.0.99.1"
-GLPROTO="glproto >= 1.4.10"
+GLPROTO="glproto >= 1.4.13"
DMXPROTO="dmxproto >= 2.2.99.1"
VIDMODEPROTO="xf86vidmodeproto >= 2.2.99.1"
WINDOWSWMPROTO="windowswmproto"
diff --git a/glx/glxdri2.c b/glx/glxdri2.c
index d979717..654b4ae 100644
--- a/glx/glxdri2.c
+++ b/glx/glxdri2.c
@@ -192,8 +192,17 @@ __glXdriSwapEvent(ClientPtr client, void *data, int type,
CARD64 ust,
wire.ust_lo = ust & 0x;
wire.msc_hi = msc >> 32;
wire.msc_lo = msc & 0x;
-wire.sbc_hi = sbc >> 32;
-wire.sbc_lo = sbc & 0x;
+wire.sbc_hi = 0;
+
+if (DRI2ClientSupportsSBC(client)) {
+ wire.sbc_lo0 = sbc & 0xff;
+ wire.sbc_lo8 = (sbc >> 8) & 0xff;
+ wire.sbc_lo16 = (sbc >> 16) & 0xff;
+} else {
+ wire.sbc_lo0 = 0;
+ wire.sbc_lo8 = 0;
+ wire.sbc_lo16 = 0;
+}
WriteEventsToClient(client, 1, (xEvent *) &wire);
}
@@ -228,6 +237,18 @@ __glXDRIdrawableSwapBuffers(ClientPtr client,
__GLXdrawable *drawable)
return TRUE;
}
+static void
+__glXDRIclientGone(CallbackListPtr *list,
+ pointer closure,
+ pointer data)
+{
+NewClientInfoRec *clientinfo = (NewClientInfoRec *) data;
+ClientPtr pClient = clientinfo->client;
+
+if (pClient->clientState == ClientStateGone)
+ DRI2ClientGone(client);
+}
+
static int
__glXDRIdrawableSwapInterval(__GLXdrawable *drawable, int interval)
{
@@ -769,6 +790,9 @@ __glXDRIscreenProbe(ScreenPtr pScreen)
screen->leaveVT = pScrn->LeaveVT;
pScrn->LeaveVT = glxDRILeaveVT;
+if (!AddCallback (&ClientStateCallback, __glXDRIclientGone, 0))
+ return;
+
LogMessage(X_INFO,
"AIGLX: Loaded and initialized %s\n", driverName);
diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index 5c42a51..9b5eab2 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -60,6 +60,9 @@ static DevPrivateKeyRec dri2WindowPrivateKeyRec;
static DevPrivateKeyRec dri2PixmapPrivateKeyRec;
#define dri2PixmapPrivateKey (&dri2PixmapPrivateKeyRec)
+static DevPrivateKeyRec dri2ClientPrivateKeyRec;
+#define dri2ClientPrivateKey (&dri2ClientPrivateKeyRec)
+
static RESTYPE dri2DrawableRes;
typedef struct _DRI2Screen *DRI2ScreenPtr;
@@ -107,6 +110,11 @@ typedef struct _DRI2Screen {
ConfigNotifyProcPtr ConfigNotify;
} DRI2ScreenRec;
+typedef struct _DRI2Client {
+CARD32 major;
+CARD32 minor;
+} DRI2ClientRec, *DRI2ClientPtr;
+
static DRI2ScreenPtr
DRI2GetScreen(ScreenPtr pScreen)
{
@@ -131,6 +139,12 @@ DRI2GetDrawable(DrawablePtr pDraw)
}
}
+static DRI2ClientPtr
+DRI2GetClient(ClientPtr client)
+{
+return dixLookupPrivate(&client->devPrivates, dri2ClientPrivateKey);
+}
+
static unsigned long
DRI2DrawableSerial(DrawablePtr pDraw)
{
@@ -190,6 +204,44 @@ DRI2AllocateDrawable(DrawablePtr pDraw)
return pPriv;
}
+void
+DRI2InitClient(ClientPtr client, CARD32 major, CARD32 minor)
+{
+DRI2ClientPtr pPriv;
+
+pPriv = malloc(sizeof *pPriv);
+if (!pPriv)
+ return;
+
+pPriv->major = major;
+pPriv->minor = minor;
+
+dixSetPrivate(&client->devPrivates, dri2ClientPrivateKey, pPriv);
+}
+
+void
+DRI2ClientGone(ClientPtr client)
+{
+DRI2ClientPtr pPriv = DRI2GetClient(client);
+
+if (pPriv)
+ free(pPriv);
+}
+
+Bool
+DRI2ClientSupportsSBC(ClientPtr client)
+{
+DRI2ClientPtr pPriv = DRI2GetClient(client);
+
+if (!pPriv)
+ return FALSE;
+
+if (pPriv->major > 1 || (pPriv->major == 1 && pPriv->minor > 3))
+ return TRUE;
+
+return FALSE;
+}
+
typedef struct DRI2DrawableRefRec {
XID id;
XID dri2_id;
@@ -1097,6 +1149,9 @@ DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info)
if (!dixRegisterPrivateKey(&dri2PixmapPrivateKeyRec, PRIVATE_PIXMAP, 0))
return
[PATCH] GLX/DRI2: fix swap complete handling
The swap complete event wasn't being handled fully; because XEvents are
only 32 bytes long, we were getting junk for the sbc_lo value. If the
server supports it, unpack the new structure, otherwise just return 0
for the sbc value instead of garbage.
---
configure.ac|4 ++--
src/glx/dri2.c |7 ++-
src/glx/dri2_glx.c | 13 +
src/glx/glxclient.h |1 +
src/glx/glxext.c|8 +++-
5 files changed, 29 insertions(+), 4 deletions(-)
diff --git a/configure.ac b/configure.ac
index 3b05ca3..94fb6f7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -21,8 +21,8 @@ dnl Versions for external dependencies
LIBDRM_REQUIRED=2.4.24
LIBDRM_RADEON_REQUIRED=2.4.24
LIBDRM_INTEL_REQUIRED=2.4.24
-DRI2PROTO_REQUIRED=2.1
-GLPROTO_REQUIRED=1.4.11
+DRI2PROTO_REQUIRED=2.4
+GLPROTO_REQUIRED=1.4.13
LIBDRM_XORG_REQUIRED=2.4.24
LIBKMS_XORG_REQUIRED=1.0.0
diff --git a/src/glx/dri2.c b/src/glx/dri2.c
index adfd3d1..864c941 100644
--- a/src/glx/dri2.c
+++ b/src/glx/dri2.c
@@ -124,7 +124,12 @@ DRI2WireToEvent(Display *dpy, XEvent *event, xEvent *wire)
}
aevent->ust = ((CARD64)awire->ust_hi << 32) | awire->ust_lo;
aevent->msc = ((CARD64)awire->msc_hi << 32) | awire->msc_lo;
- aevent->sbc = ((CARD64)awire->sbc_hi << 32) | awire->sbc_lo;
+ if (dri2ServerSupportsSBC(dpy)) {
+aevent->sbc = ((CARD64)awire->sbc_hi << 32) | (awire->sbc_lo16 << 16) |
+ (awire->sbc_lo8 << 8) | awire->sbc_lo0;
+ } else {
+aevent->sbc = 0;
+ }
return True;
}
#endif
diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
index fc0237a..be96ec6 100644
--- a/src/glx/dri2_glx.c
+++ b/src/glx/dri2_glx.c
@@ -791,6 +791,19 @@ static const struct glx_screen_vtable dri2_screen_vtable =
{
dri2_create_context
};
+int
+dri2ServerSupportsSBC(Display *dpy)
+{
+ struct glx_display *dpyPriv = __glXInitialize(dpy);
+ struct dri2_display *pdp =
+ (struct dri2_display *) dpyPriv->dri2Display;
+
+ if (pdp->driMajor > 1 || (pdp->driMajor == 1 && pdp->driMinor > 3))
+ return 1;
+
+ return 0;
+}
+
static struct glx_screen *
dri2CreateScreen(int screen, struct glx_display * priv)
{
diff --git a/src/glx/glxclient.h b/src/glx/glxclient.h
index 2b6966f..acba82c 100644
--- a/src/glx/glxclient.h
+++ b/src/glx/glxclient.h
@@ -149,6 +149,7 @@ extern __GLXDRIdisplay *driCreateDisplay(Display * dpy);
extern __GLXDRIdisplay *dri2CreateDisplay(Display * dpy);
extern void dri2InvalidateBuffers(Display *dpy, XID drawable);
extern unsigned dri2GetSwapEventType(Display *dpy, XID drawable);
+extern int dri2ServerSupportsSBC(Display *dpy);
/*
diff --git a/src/glx/glxext.c b/src/glx/glxext.c
index 278c719..7393efc 100644
--- a/src/glx/glxext.c
+++ b/src/glx/glxext.c
@@ -138,7 +138,13 @@ __glXWireToEvent(Display *dpy, XEvent *event, xEvent *wire)
aevent->drawable = awire->drawable;
aevent->ust = ((CARD64)awire->ust_hi << 32) | awire->ust_lo;
aevent->msc = ((CARD64)awire->msc_hi << 32) | awire->msc_lo;
- aevent->sbc = ((CARD64)awire->sbc_hi << 32) | awire->sbc_lo;
+ if (glx_dpy->majorVersion > 1 || (glx_dpy->majorVersion == 1 &&
+ glx_dpy->minorVersion > 4)) {
+aevent->sbc = ((CARD64)awire->sbc_hi << 32) | (awire->sbc_lo16 << 16) |
+ (awire->sbc_lo8 << 8) | awire->sbc_lo0;
+ } else {
+aevent->sbc = 0;
+ }
return True;
}
default:
--
1.7.1
[PATCH] Fix swap complete event size
XEvents are limited to 32 bytes, so use some creative stuffing to fit
all the bits we'd like to supply.
---
configure.ac |2 +-
dri2proto.h |9 +
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/configure.ac b/configure.ac
index 5b78d6b..9505f56 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1,5 +1,5 @@
AC_PREREQ([2.60])
-AC_INIT([DRI2Proto], [2.3],
[https://bugs.freedesktop.org/enter_bug.cgi?product=xorg])
+AC_INIT([DRI2Proto], [2.4],
[https://bugs.freedesktop.org/enter_bug.cgi?product=xorg])
AM_INIT_AUTOMAKE([foreign dist-bzip2])
AM_MAINTAINER_MODE
diff --git a/dri2proto.h b/dri2proto.h
index 9708a4a..e568c91 100644
--- a/dri2proto.h
+++ b/dri2proto.h
@@ -35,7 +35,7 @@
#define DRI2_NAME "DRI2"
#define DRI2_MAJOR 1
-#define DRI2_MINOR 3
+#define DRI2_MINOR 4
#define DRI2NumberErrors 0
#define DRI2NumberEvents 2
@@ -287,16 +287,17 @@ typedef struct {
typedef struct {
CARD8 type;
-CARD8 pad;
+CARD8 sbc_lo0;
CARD16 sequenceNumber B16;
-CARD16 event_type B16;
+CARD8 event_type;
+CARD8 sbc_lo8;
+CARD16 sbc_lo16 B16;
CARD32 drawable B32;
CARD32 ust_hi B32;
CARD32 ust_lo B32;
CARD32 msc_hi B32;
CARD32 msc_lo B32;
CARD32 sbc_hi B32;
-CARD32 sbc_lo B32;
} xDRI2BufferSwapComplete;
#define sz_xDRI2BufferSwapComplete 32
--
1.7.1
[PATCH] Pack swap complete bits into an XEvent
The defintion of the swap complete event was wrong; XEvents are only 32
bytes long, and with padding the swap event was longer. So use some
creative packing to get all the bits we want transmitted. Requires a
proto version bump.
---
configure.ac |2 +-
glxproto.h | 13 +
2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/configure.ac b/configure.ac
index d88e6df..a3047e4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1,5 +1,5 @@
AC_PREREQ([2.60])
-AC_INIT([GLProto], [1.4.12],
[https://bugs.freedesktop.org/enter_bug.cgi?product=xorg])
+AC_INIT([GLProto], [1.4.13],
[https://bugs.freedesktop.org/enter_bug.cgi?product=xorg])
AM_INIT_AUTOMAKE([foreign dist-bzip2])
AM_MAINTAINER_MODE
diff --git a/glxproto.h b/glxproto.h
index 0ff44e3..4a583c1 100644
--- a/glxproto.h
+++ b/glxproto.h
@@ -1370,18 +1370,23 @@ typedef struct {
CARD32 unused2 B32;
} xGLXPbufferClobberEvent;
+/* Note, this struct is too large for an Xevent, I fail -- jbarnes
+ * So sbc_lo won't ever be sent. We can use a generic event though without
+ * size restrictions, thus xGLXBufferSwapComplete2.
+ */
typedef struct {
BYTE type;
-BYTE pad;
+BYTE sbc_lo0;
CARD16 sequenceNumber B16;
-CARD16 event_type B16;
-CARD32 drawable;
+CARD8 event_type;
+CARD8 sbc_lo8;
+CARD16 sbc_lo16 B16;
+CARD32 drawable B32;
CARD32 ust_hi B32;
CARD32 ust_lo B32;
CARD32 msc_hi B32;
CARD32 msc_lo B32;
CARD32 sbc_hi B32;
-CARD32 sbc_lo B32;
} xGLXBufferSwapComplete;
//
--
1.7.1
[RFC] swap event handling fixes
I obviously failed to count the swap event structure size after adding and removing fields a few times, and didn't even account for padding. The end result is that clients today won't receive the sbc_lo field at all, and so will likely stuff junk into that field on the client side (or zero at best). This patchset fixes up the structure definitions, bumps the proto levels, and adds server and client handling code for it all. It should be forward and backward compatible, but as always review and testing appreciated. I think the event_type checking on the client side still needs work; the field is split now so I need to check the right one on old servers. I'll also add swap support for the new requests in case people ever want to run this stuff between big and little endian machines. Thanks, Jesse
i915: irq nobody cared
Hi, while watching flash video in a browser I got: irq 42: nobody cared (try booting with the "irqpoll" option) Pid: 0, comm: swapper Tainted: GW 2.6.39-rc3-mm1_64+ #1423 Call Trace: [] __report_bad_irq+0x31/0xc0 [] note_interrupt+0x194/0x1d0 [] handle_irq_event_percpu+0xe8/0x160 [] handle_irq_event+0x35/0x60 [] handle_edge_irq+0x6f/0x110 [] handle_irq+0x1d/0x30 [] do_IRQ+0x58/0xe0 [] common_interrupt+0x13/0x13 [] ? __switch_to+0x260/0x2c0 [] ? tick_nohz_stop_sched_tick+0x28e/0x3d0 [] ? default_idle+0x24/0x40 [] cpu_idle+0x36/0xb0 [] rest_init+0x68/0x70 [] start_kernel+0x35c/0x367 [] x86_64_start_reservations+0x132/0x136 [] x86_64_start_kernel+0xf1/0xf8 handlers: [] (i915_driver_irq_handler+0x0/0x420) Disabling IRQ #42 Now the interrupt is polled, so that the graphics is obviously sluggish. I have also /sys/kernel/debug/dri contents if you want anything of that. Going to reboot now. # lspci -vvnnxxxs 00:02.0 00:02.0 VGA compatible controller [0300]: Intel Corporation 82G33/G31 Express Integrated Graphics Controller [8086:29c2] (rev 02) (prog-if 00 [VGA controller]) Subsystem: Intel Corporation 82G33/G31 Express Integrated Graphics Controller [8086:29c2] Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+ Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- SERR- Latency: 0 Interrupt: pin A routed to IRQ 42 Region 0: Memory at feb8 (32-bit, non-prefetchable) [size=512K] Region 1: I/O ports at ec00 [size=8] Region 2: Memory at d000 (32-bit, prefetchable) [size=256M] Region 3: Memory at fea0 (32-bit, non-prefetchable) [size=1M] Expansion ROM at [disabled] Capabilities: [90] MSI: Enable+ Count=1/1 Maskable- 64bit- Address: fee0300c Data: 4161 Capabilities: [d0] Power Management version 2 Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-) Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME- Kernel driver in use: i915 00: 86 80 c2 29 07 04 90 00 02 00 00 03 00 00 00 00 10: 00 00 b8 fe 01 ec 00 00 08 00 00 d0 00 00 a0 fe 20: 00 00 00 00 00 00 00 00 00 00 00 00 86 80 c2 29 30: 00 00 00 00 90 00 00 00 00 00 00 00 05 01 00 00 40: 09 00 0b 01 00 00 00 00 01 00 00 00 00 00 00 00 50: 00 00 30 02 c9 03 00 00 00 00 00 00 00 00 80 af 60: 00 00 02 02 00 00 00 00 00 00 00 00 00 00 00 00 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 90: 05 d0 01 00 0c 30 e0 fe 61 41 00 00 00 00 00 00 a0: 11 11 00 00 00 00 06 03 00 00 00 00 00 00 00 00 b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 d0: 01 00 22 00 00 00 00 00 00 00 00 00 00 01 02 00 e0: 00 00 00 00 00 00 00 00 00 80 00 00 00 00 00 00 f0: 10 00 00 00 00 00 00 00 90 0f 03 00 e4 e0 5b af regards, -- js ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC] drm: add overlays as first class KMS objects
On Mon, Apr 25, 2011 at 5:12 PM, Jesse Barnes
wrote:
> Looking for comments on this. ?Obviously if we're going to add a new type
> of KMS object, we'd better get the ioctl more or less right to begin with,
> which means having all the attributes we'd like to track, plus some
> padding, available from the outset.
>
> So I'd like comments on this; the whole approach may be broken for things
> like OMAP; if so I'd like to hear about it now. ?Overall, overlays are
> treated a little like CRTCs, but without associated modes our encoder
> trees hanging off of them. ?That is, they can be enabled with a specific
> fb attached at a specific location, but they don't have to worry about
> mode setting, per se (though they do need to have an associated CRTC to
> actually pump their pixels out, post-blend).
>
> Flipping could be done either with the existing ioctl by updating the fb
> base pointer, or through a new flip ioctl similar to what we have already,
> but taking an overlay object instead of a CRTC.
One thing I am wondering about is how to synchronize overlay position
w/ flipping in the primary gfx layer? Assuming you actually are
flipping in primary layer you'd want a new set of overlay
position/size to take effect on the same vblank that the flip in the
gfx layer happens, because you are probably relying on some
transparent pixels (or colorkey, if anyone still uses that) to be
drawn in the UI layer.
BR,
-R
> Overlays are a bit like half-CRTCs. ?They have a location and fb, but
> don't drive outputs directly. ?Add support for handling them to the core
> KMS code.
>
> Signed-off-by: Jesse Barnes
> ---
> ?drivers/gpu/drm/drm_crtc.c | ?219
>
> ?include/drm/drm.h ? ? ? ? ?| ? ?3 +
> ?include/drm/drm_crtc.h ? ? | ? 61
> ?include/drm/drm_mode.h ? ? | ? 39
> ?4 files changed, 322 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 799e149..77ff9e0 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -533,6 +533,34 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
> ?}
> ?EXPORT_SYMBOL(drm_encoder_cleanup);
>
> +void drm_overlay_init(struct drm_device *dev, struct drm_overlay *overlay,
> + ? ? ? ? ? ? ? ? ? ? const struct drm_overlay_funcs *funcs)
> +{
> + ? ? ? mutex_lock(&dev->mode_config.mutex);
> +
> + ? ? ? overlay->dev = dev;
> + ? ? ? drm_mode_object_get(dev, &overlay->base, DRM_MODE_OBJECT_OVERLAY);
> + ? ? ? overlay->funcs = funcs;
> +
> + ? ? ? list_add_tail(&overlay->head, &dev->mode_config.overlay_list);
> + ? ? ? dev->mode_config.num_overlay++;
> +
> + ? ? ? mutex_unlock(&dev->mode_config.mutex);
> +}
> +EXPORT_SYMBOL(drm_overlay_init);
> +
> +void drm_overlay_cleanup(struct drm_overlay *overlay)
> +{
> + ? ? ? struct drm_device *dev = overlay->dev;
> +
> + ? ? ? mutex_lock(&dev->mode_config.mutex);
> + ? ? ? drm_mode_object_put(dev, &overlay->base);
> + ? ? ? list_del(&overlay->head);
> + ? ? ? dev->mode_config.num_overlay--;
> + ? ? ? mutex_unlock(&dev->mode_config.mutex);
> +}
> +EXPORT_SYMBOL(drm_overlay_cleanup);
> +
> ?/**
> ?* drm_mode_create - create a new display mode
> ?* @dev: DRM device
> @@ -864,6 +892,7 @@ void drm_mode_config_init(struct drm_device *dev)
> ? ? ? ?INIT_LIST_HEAD(&dev->mode_config.encoder_list);
> ? ? ? ?INIT_LIST_HEAD(&dev->mode_config.property_list);
> ? ? ? ?INIT_LIST_HEAD(&dev->mode_config.property_blob_list);
> + ? ? ? INIT_LIST_HEAD(&dev->mode_config.overlay_list);
> ? ? ? ?idr_init(&dev->mode_config.crtc_idr);
>
> ? ? ? ?mutex_lock(&dev->mode_config.mutex);
> @@ -1467,6 +1496,196 @@ out:
> ?}
>
> ?/**
> + * drm_mode_getoverlay_res - get overlay info
> + * @dev: DRM device
> + * @data: ioctl data
> + * @file_priv: DRM file info
> + *
> + * Return an overlay count and set of IDs.
> + */
> +int drm_mode_getoverlay_res(struct drm_device *dev, void *data,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? struct drm_file *file_priv)
> +{
> + ? ? ? struct drm_mode_get_overlay_res *overlay_resp = data;
> + ? ? ? struct drm_mode_config *config;
> + ? ? ? struct drm_overlay *overlay;
> + ? ? ? uint32_t __user *overlay_ptr;
> + ? ? ? int copied = 0, ret = 0;
> +
> + ? ? ? if (!drm_core_check_feature(dev, DRIVER_MODESET))
> + ? ? ? ? ? ? ? return -EINVAL;
> +
> + ? ? ? mutex_lock(&dev->mode_config.mutex);
> + ? ? ? config = &dev->mode_config;
> +
> + ? ? ? /*
> + ? ? ? ?* This ioctl is called twice, once to determine how much space is
> + ? ? ? ?* needed, and the 2nd time to fill it.
> + ? ? ? ?*/
> + ? ? ? if (config->num_overlay &&
> + ? ? ? ? ? (overlay_resp->count_overlays >= config->num_overlay)) {
> + ? ? ? ? ? ? ? overlay_ptr = (uint32_t *)(unsigned
> long)overlay_resp->overlay_id_ptr;
> +
> + ? ? ? ? ? ? ? list_for_each_entry(overlay, &config->overlay_list, head) {
> + ? ? ? ? ? ? ? ? ? ? ? if (put_user(overlay->base.id, overlay_ptr + copied))
> {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ret = -EFAULT;
> + ? ? ? ? ? ? ? ? ? ? ? ?
[RFC] drm: add overlays as first class KMS objects
On Tue, Apr 26, 2011 at 5:01 AM, Alan Cox wrote: >> A lot of older hardware had one overlay that could be sourced to any >> crtc, just not simultaneously. ?The tricky part is the formats and >> capabilities: alpha blending, color/chromakey, gamma correction, etc. >> Even the current crtc gamma stuff is somewhat lacking in in terms of >> what hardware is capable of (PWL vs. LUT, user defined conversion >> matrices, gamut remapping, etc.). > > Rather than re-inventing enough wheels to run a large truck would it not > make sense to make hardware sourced overlays Video4Linux objects in their > entirity so you can just say "attach V4L object A as overlay B" One thing I'd like to be able to do is use a CRTC as either an overlay or an primary framebuffer.. For example on omap4, if we have one display plugged in, then we have 3 pipes that can be used for overlay. If a second display is plugged in, one of those overlay pipes gets used as the primary gfx layer on the 2nd display. The ideal situation would let us share buffers w/ x11 with dri2 (although we need more than just front/back buffer.. you might have > 16 buffers for video playback). And then in the xorg driver decide whether to use an overlay or GPU blit depending on what hw resources are not already in use. So I'm not completely sure about the v4l2/drm coexistance for overlays. (Although for capture devices, it makes 100% sense to have a way of sharing GEM buffers w/ v4l2.) On the other hand, flipping video buffers is kind of (if you squint slightly) just like flipping buffers in a 3d gl app. BR, -R > That would provide format definitions, provide control interfaces for > the surface (eg for overlays of cameras such as on some of the Intel > embedded and non PC devices), give you an existing well understood API. > > For some hardware you are going to need this integration anyway so that > you can do things like move a GEM object which is currently a DMA target > of a capture device (as well as to fence it). > > For a software surface you could either expose it as a V4L object that > is GEM or fb memory backed or at least use the same descriptions so that > the kernel has a consistent set of descriptions for formats and we don't > have user libraries doing adhoc format translation crap. > > A lot of capture hardware would map very nicely onto GEM objects I > suspect and if you want to merge live video into Wayland it seems a > logical path ? > > Alan > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
[RFC] drm: add overlays as first class KMS objects
2011/4/25 St?phane Marchesin : > On Mon, Apr 25, 2011 at 16:22, Jesse Barnes > wrote: >> On Mon, 25 Apr 2011 16:16:18 -0700 >> Keith Packard wrote: >> >>> On Mon, 25 Apr 2011 15:12:20 -0700, Jesse Barnes >> virtuousgeek.org> wrote: >>> >>> > Overlays are a bit like half-CRTCs. ?They have a location and fb, but >>> > don't drive outputs directly. ?Add support for handling them to the core >>> > KMS code. >>> >>> Are overlays/underlays not associated with a specific CRTC? To my mind, >>> overlays are another scanout buffer associated with a specific CRTC, so >>> you'd create a scanout buffer and attach that to a specific scanout slot >>> in a crtc, with the 'default' slot being the usual graphics plane. >> >> Yes, that matches my understanding as well. ?I've deliberately made the >> implementation flexible there though, under the assumption that some >> hardware allows a plane to be directed at more than one CRTC (though >> probably not simultaneously). >> >> Arguably, this is something we should have done when the >> connector/encoder split was done (making planes in general first class >> objects). ?But with today's code, treating a CRTC as a pixel pump and a >> primary plane seems fine, with overlays tacked onto the side as >> secondary pixel sources but tied to a specific CRTC. >> > > What is the plan for supporting multiple formats? When I looked at > this for nouveau it ended up growing out of control when adding > support for all the YUV (planar, packed, 12 or 16 bpp formats) and RGB > format combinations. maybe a dumb idea, but since all the GEM buffer allocation is already done thru driver specific ioctl, couldn't the color format (and the one or more plane pointers) be something that the DRM overlay infrastructure doesn't have to care about. I mean, I guess it is somehow analogous to various tiled formats that you might have. If the layout of the bytes is a property of the actual buffer object, then wouldn't it be ok for DRM overlay infrastructure to ignore it and the individual driver implementations just do the right thing based on some private driver properties of the bo? Maybe I'm over-simplifying or overlooking something, though.. BR, -R > St?phane > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
Re: [RFC] drm: add overlays as first class KMS objects
On Thu, Apr 28, 2011 at 12:03:32PM -0500, Rob Clark wrote: > On Mon, Apr 25, 2011 at 5:12 PM, Jesse Barnes > wrote: > > Looking for comments on this. Obviously if we're going to add a new type > > of KMS object, we'd better get the ioctl more or less right to begin with, > > which means having all the attributes we'd like to track, plus some > > padding, available from the outset. > > > > So I'd like comments on this; the whole approach may be broken for things > > like OMAP; if so I'd like to hear about it now. Overall, overlays are > > treated a little like CRTCs, but without associated modes our encoder > > trees hanging off of them. That is, they can be enabled with a specific > > fb attached at a specific location, but they don't have to worry about > > mode setting, per se (though they do need to have an associated CRTC to > > actually pump their pixels out, post-blend). > > > > Flipping could be done either with the existing ioctl by updating the fb > > base pointer, or through a new flip ioctl similar to what we have already, > > but taking an overlay object instead of a CRTC. > > One thing I am wondering about is how to synchronize overlay position > w/ flipping in the primary gfx layer? Assuming you actually are > flipping in primary layer you'd want a new set of overlay > position/size to take effect on the same vblank that the flip in the > gfx layer happens, because you are probably relying on some > transparent pixels (or colorkey, if anyone still uses that) to be > drawn in the UI layer. That's a good reason to aim for an OpenWF Display type of API where you can commit the whole device state atomically. -- Ville Syrjälä syrj...@sci.fi http://www.sci.fi/~syrjala/ ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC] drm: add overlays as first class KMS objects
On Wed, Apr 27, 2011 at 11:12 PM, Jesse Barnes wrote:
> On Wed, 27 Apr 2011 14:19:05 +0200
> Daniel Vetter wrote:
>
>> Hi Jesse,
>>
>> I like it. It's a bit of a chicken-egg api design problem, but if a
>> proof-of-concept
>> implementation exists for an embedded chip plus something to check whether
>> it's good enough to implement Xv on ancient hw (intel overlay or for the qemu
>> kms driver, if that could do this), that should be good enough.
>>
>> A few comments on the ioctl interface.
>
> Thanks for checking it out.
>
>> > +/* Overlays blend with or override other bits on the CRTC */
>> > +struct drm_mode_set_overlay {
>> > + __u32 overlay_id;
>> > + __u32 crtc_id;
>> > + __u32 fb_id; /* contains surface format type */
>> > +
>> > + __u32 crtc_x, crtc_y;
>> > + __u32 x, y;
>> > +
>> > + /* FIXME: color key/mask, scaling, z-order, other? */
>> > +};
>>
>> I think scaling is required, you can't implement Xv without that. The
>> problem is some arbitraray
>> hw range restrictions, but returning -EINVAL if they're violated looks okay.
>> So
>>
>> float scale_x, scale_y;
>
> Ok, I'll collect more fields based on this thread and make this
> structure bigger. Still not sure how to handle arbitrary blend and
> z-order restrictions without passing a tree around though...
What we send over the ioctl interface (in vmwgfx) to control the
overlays is this:
/**
* struct drm_vmw_control_stream_arg
*
* @stream_id: Stearm to control
* @enabled: If false all following arguments are ignored.
* @handle: Handle to buffer for getting data from.
* @format: Format of the overlay as understood by the host.
* @width: Width of the overlay.
* @height: Height of the overlay.
* @size: Size of the overlay in bytes.
* @pitch: Array of pitches, the two last are only used for YUV12 formats.
* @offset: Offset from start of dma buffer to overlay.
* @src: Source rect, must be within the defined area above.
* @dst: Destination rect, x and y may be negative.
*
* Argument to the DRM_VMW_CONTROL_STREAM Ioctl.
*/
struct drm_vmw_control_stream_arg {
uint32_t stream_id;
uint32_t enabled;
uint32_t flags;
uint32_t color_key;
uint32_t handle;
uint32_t offset;
int32_t format;
uint32_t size;
uint32_t width;
uint32_t height;
uint32_t pitch[3];
uint32_t pad64;
struct drm_vmw_rect src;
struct drm_vmw_rect dst;
};
The command contains information describing the source "framebuffer"
as well as the data for controlling the overlay. The args are by a
amazing coincidence is almost 1:1 what we also send down to the host.
The biggest change here from what you proposed is that we send two
rects describing from where within the buffer we get the data and to
where it should go. My vote is to do that as well (makes my life
easier at least).
Tho I guess that color_key and flags could be made into a property.
Cheers Jakob.
>
>>
>> should be good enough. For the remaining things (color conversion,
>> blending, ...) I don't think
>> there's any generic way to map hw capabilities (without going insane).
>> I think a bunch of
>> properties (with standard stuff for gamma, color key, ...) would be
>> perfect for that. If we
>> set the defaults such that it Just Works for Xv (after setting the
>> color key, if present), that
>> would be great!
>
> Yeah, properties for those is probably the way to go.
>
>>
>> > +struct drm_mode_get_overlay {
>> > + __u64 format_type_ptr;
>> > + __u32 overlay_id;
>> > +
>> > + __u32 crtc_id;
>> > + __u32 fb_id;
>> > +
>> > + __u32 crtc_x, crtc_y;
>> > + __u32 x, y;
>> > +
>> > + __u32 possible_crtcs;
>> > + __u32 gamma_size;
>> > +
>> > + __u32 count_format_types;
>> > +};
>>
>> Imo the real problem with formats is stride restrictions and other hw
>> restrictions (tiling, ...).
>> ARM/v4l people seem to want that to be in the kernel so that they can
>> e.g. dma decoded
>> video streams directly to the gpu (also for other stuff). Perhaps we
>> want to extend the
>> create_dumb_gem_object ioctl for that use case, but I'm not too
>> convinced that this belongs
>> into the kernel.
>
> I think it's a bit like handling tiling formats; for the most part the
> kernel doesn't care because it's not reading or writing the data, but
> it does need to know the format when programming certain regs. So I
> don't think we can avoid having surface format information passed in
> when creating an fb for an overlay. And if we do that we may as well
> enumerate the types we support when overlay info is fetched to make
> portability for app code a little easier.
>
> Jesse
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC] drm: add overlays as first class KMS objects
On Mon, Apr 25, 2011 at 5:12 PM, Jesse Barnes wrote:
> Looking for comments on this. Obviously if we're going to add a new type
> of KMS object, we'd better get the ioctl more or less right to begin with,
> which means having all the attributes we'd like to track, plus some
> padding, available from the outset.
>
> So I'd like comments on this; the whole approach may be broken for things
> like OMAP; if so I'd like to hear about it now. Overall, overlays are
> treated a little like CRTCs, but without associated modes our encoder
> trees hanging off of them. That is, they can be enabled with a specific
> fb attached at a specific location, but they don't have to worry about
> mode setting, per se (though they do need to have an associated CRTC to
> actually pump their pixels out, post-blend).
>
> Flipping could be done either with the existing ioctl by updating the fb
> base pointer, or through a new flip ioctl similar to what we have already,
> but taking an overlay object instead of a CRTC.
One thing I am wondering about is how to synchronize overlay position
w/ flipping in the primary gfx layer? Assuming you actually are
flipping in primary layer you'd want a new set of overlay
position/size to take effect on the same vblank that the flip in the
gfx layer happens, because you are probably relying on some
transparent pixels (or colorkey, if anyone still uses that) to be
drawn in the UI layer.
BR,
-R
> Overlays are a bit like half-CRTCs. They have a location and fb, but
> don't drive outputs directly. Add support for handling them to the core
> KMS code.
>
> Signed-off-by: Jesse Barnes
> ---
> drivers/gpu/drm/drm_crtc.c | 219
>
> include/drm/drm.h | 3 +
> include/drm/drm_crtc.h | 61
> include/drm/drm_mode.h | 39
> 4 files changed, 322 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 799e149..77ff9e0 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -533,6 +533,34 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
> }
> EXPORT_SYMBOL(drm_encoder_cleanup);
>
> +void drm_overlay_init(struct drm_device *dev, struct drm_overlay *overlay,
> + const struct drm_overlay_funcs *funcs)
> +{
> + mutex_lock(&dev->mode_config.mutex);
> +
> + overlay->dev = dev;
> + drm_mode_object_get(dev, &overlay->base, DRM_MODE_OBJECT_OVERLAY);
> + overlay->funcs = funcs;
> +
> + list_add_tail(&overlay->head, &dev->mode_config.overlay_list);
> + dev->mode_config.num_overlay++;
> +
> + mutex_unlock(&dev->mode_config.mutex);
> +}
> +EXPORT_SYMBOL(drm_overlay_init);
> +
> +void drm_overlay_cleanup(struct drm_overlay *overlay)
> +{
> + struct drm_device *dev = overlay->dev;
> +
> + mutex_lock(&dev->mode_config.mutex);
> + drm_mode_object_put(dev, &overlay->base);
> + list_del(&overlay->head);
> + dev->mode_config.num_overlay--;
> + mutex_unlock(&dev->mode_config.mutex);
> +}
> +EXPORT_SYMBOL(drm_overlay_cleanup);
> +
> /**
> * drm_mode_create - create a new display mode
> * @dev: DRM device
> @@ -864,6 +892,7 @@ void drm_mode_config_init(struct drm_device *dev)
> INIT_LIST_HEAD(&dev->mode_config.encoder_list);
> INIT_LIST_HEAD(&dev->mode_config.property_list);
> INIT_LIST_HEAD(&dev->mode_config.property_blob_list);
> + INIT_LIST_HEAD(&dev->mode_config.overlay_list);
> idr_init(&dev->mode_config.crtc_idr);
>
> mutex_lock(&dev->mode_config.mutex);
> @@ -1467,6 +1496,196 @@ out:
> }
>
> /**
> + * drm_mode_getoverlay_res - get overlay info
> + * @dev: DRM device
> + * @data: ioctl data
> + * @file_priv: DRM file info
> + *
> + * Return an overlay count and set of IDs.
> + */
> +int drm_mode_getoverlay_res(struct drm_device *dev, void *data,
> + struct drm_file *file_priv)
> +{
> + struct drm_mode_get_overlay_res *overlay_resp = data;
> + struct drm_mode_config *config;
> + struct drm_overlay *overlay;
> + uint32_t __user *overlay_ptr;
> + int copied = 0, ret = 0;
> +
> + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> + return -EINVAL;
> +
> + mutex_lock(&dev->mode_config.mutex);
> + config = &dev->mode_config;
> +
> + /*
> + * This ioctl is called twice, once to determine how much space is
> + * needed, and the 2nd time to fill it.
> + */
> + if (config->num_overlay &&
> + (overlay_resp->count_overlays >= config->num_overlay)) {
> + overlay_ptr = (uint32_t *)(unsigned
> long)overlay_resp->overlay_id_ptr;
> +
> + list_for_each_entry(overlay, &config->overlay_list, head) {
> + if (put_user(overlay->base.id, overlay_ptr + copied))
> {
> + ret = -EFAULT;
> +
Re: [PATCH 2/3] drm: Warn if vblank state has become inconsistent.
On Apr 28, 2011, at 9:47 AM, Christopher James Halse Rogers wrote: On Wed, 2011-04-27 at 17:50 +0200, Mario Kleiner wrote: On Apr 27, 2011, at 10:58 AM, dri-devel-requ...@lists.freedesktop.org wrote: Message: 5 Date: Wed, 27 Apr 2011 10:38:14 +0200 From: Michel D?nzer Subject: Re: [PATCH 2/3] drm: Warn if vblank state has become inconsistent. To: christopher.halse.rog...@canonical.com Cc: dri-devel@lists.freedesktop.org Message-ID: <1303893494.5633.129.camel@thor.local> Content-Type: text/plain; charset="UTF-8" On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rog...@canonical.com wrote: From: Christopher James Halse Rogers After emitting all the waiting vblank events no-one should hold a vblank reference. Emit a warning if this is not the case. Signed-off-by: Christopher James Halse Rogers --- drivers/gpu/drm/drm_irq.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index a1f12cb..72407fa 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -960,6 +960,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc) e->event.sequence); } + WARN_ON(atomic_read(&dev->vblank_refcount[crtc]) != 0); spin_unlock_irqrestore(&dev->vbl_lock, irqflags); } EXPORT_SYMBOL(drm_vblank_off); Reviewed-by: Michel D?nzer Any pending kms pageflip will also hold a reference on the vblank of a crtc, so having the refcount non-zero there is not really a sign of inconsistency, so i'm not sure if a warning is appropriate there. But at this point the vblank interrupts have been disabled, or at least the driver's disable function has been called. Will that not mean that any pending pageflips will wait indefinitely for a vblank interrupt that's not going to come - ie: exactly the state we're warning against? Ok, if that is the state we're warning against, then the warning makes sense. But it would make more sense to somehow handle this more gracefully. The pageflip completion path does depend on vblank irq's to unpin buffers, release fences and notify user apps of swap completion. Hanging there due to disabled vblank irq sounds bad. Otoh the only caller of drm_vblank_off() i can find in 2.6.38 is the crtc disable code in intel_display.c and those routines protect against pending pageflips - they wait for all pageflips to finish before calling drm_vblank_off(). If this warning is just a precaution against possible future bugs then i happily rest my case. -mario ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC] drm: add overlays as first class KMS objects
On Tue, Apr 26, 2011 at 5:01 AM, Alan Cox wrote: >> A lot of older hardware had one overlay that could be sourced to any >> crtc, just not simultaneously. The tricky part is the formats and >> capabilities: alpha blending, color/chromakey, gamma correction, etc. >> Even the current crtc gamma stuff is somewhat lacking in in terms of >> what hardware is capable of (PWL vs. LUT, user defined conversion >> matrices, gamut remapping, etc.). > > Rather than re-inventing enough wheels to run a large truck would it not > make sense to make hardware sourced overlays Video4Linux objects in their > entirity so you can just say "attach V4L object A as overlay B" One thing I'd like to be able to do is use a CRTC as either an overlay or an primary framebuffer.. For example on omap4, if we have one display plugged in, then we have 3 pipes that can be used for overlay. If a second display is plugged in, one of those overlay pipes gets used as the primary gfx layer on the 2nd display. The ideal situation would let us share buffers w/ x11 with dri2 (although we need more than just front/back buffer.. you might have > 16 buffers for video playback). And then in the xorg driver decide whether to use an overlay or GPU blit depending on what hw resources are not already in use. So I'm not completely sure about the v4l2/drm coexistance for overlays. (Although for capture devices, it makes 100% sense to have a way of sharing GEM buffers w/ v4l2.) On the other hand, flipping video buffers is kind of (if you squint slightly) just like flipping buffers in a 3d gl app. BR, -R > That would provide format definitions, provide control interfaces for > the surface (eg for overlays of cameras such as on some of the Intel > embedded and non PC devices), give you an existing well understood API. > > For some hardware you are going to need this integration anyway so that > you can do things like move a GEM object which is currently a DMA target > of a capture device (as well as to fence it). > > For a software surface you could either expose it as a V4L object that > is GEM or fb memory backed or at least use the same descriptions so that > the kernel has a consistent set of descriptions for formats and we don't > have user libraries doing adhoc format translation crap. > > A lot of capture hardware would map very nicely onto GEM objects I > suspect and if you want to merge live video into Wayland it seems a > logical path ? > > Alan > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC] drm: add overlays as first class KMS objects
2011/4/25 Stéphane Marchesin : > On Mon, Apr 25, 2011 at 16:22, Jesse Barnes wrote: >> On Mon, 25 Apr 2011 16:16:18 -0700 >> Keith Packard wrote: >> >>> On Mon, 25 Apr 2011 15:12:20 -0700, Jesse Barnes >>> wrote: >>> >>> > Overlays are a bit like half-CRTCs. They have a location and fb, but >>> > don't drive outputs directly. Add support for handling them to the core >>> > KMS code. >>> >>> Are overlays/underlays not associated with a specific CRTC? To my mind, >>> overlays are another scanout buffer associated with a specific CRTC, so >>> you'd create a scanout buffer and attach that to a specific scanout slot >>> in a crtc, with the 'default' slot being the usual graphics plane. >> >> Yes, that matches my understanding as well. I've deliberately made the >> implementation flexible there though, under the assumption that some >> hardware allows a plane to be directed at more than one CRTC (though >> probably not simultaneously). >> >> Arguably, this is something we should have done when the >> connector/encoder split was done (making planes in general first class >> objects). But with today's code, treating a CRTC as a pixel pump and a >> primary plane seems fine, with overlays tacked onto the side as >> secondary pixel sources but tied to a specific CRTC. >> > > What is the plan for supporting multiple formats? When I looked at > this for nouveau it ended up growing out of control when adding > support for all the YUV (planar, packed, 12 or 16 bpp formats) and RGB > format combinations. maybe a dumb idea, but since all the GEM buffer allocation is already done thru driver specific ioctl, couldn't the color format (and the one or more plane pointers) be something that the DRM overlay infrastructure doesn't have to care about. I mean, I guess it is somehow analogous to various tiled formats that you might have. If the layout of the bytes is a property of the actual buffer object, then wouldn't it be ok for DRM overlay infrastructure to ignore it and the individual driver implementations just do the right thing based on some private driver properties of the bo? Maybe I'm over-simplifying or overlooking something, though.. BR, -R > Stéphane > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC] drm: add overlays as first class KMS objects
On Wed, Apr 27, 2011 at 11:12 PM, Jesse Barnes wrote: > On Wed, 27 Apr 2011 14:19:05 +0200 Daniel Vetter wrote: >> Imo the real problem with formats is stride restrictions and other hw >> restrictions (tiling, ...). >> ARM/v4l people seem to want that to be in the kernel so that they can >> e.g. dma decoded >> video streams directly to the gpu (also for other stuff). Perhaps we >> want to extend the >> create_dumb_gem_object ioctl for that use case, but I'm not too >> convinced that this belongs >> into the kernel. > > I think it's a bit like handling tiling formats; for the most part the > kernel doesn't care because it's not reading or writing the data, but > it does need to know the format when programming certain regs. ?So I > don't think we can avoid having surface format information passed in > when creating an fb for an overlay. ?And if we do that we may as well > enumerate the types we support when overlay info is fetched to make > portability for app code a little easier. I agree with your design ;-) My above comment was just in the light of the ARM unified memory management discussion where some people seem to want to move a complete buffer format description beast into the kernel (like v4l already has). I think for gpus that'd get out of hand quickly and format/layout arbitrage code should life in userspace. -Daniel -- Daniel Vetter daniel.vetter at ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch
[PATCH] fix mesa tarball creation again
On 19 April 2011 16:35, Brian wrote: >>> Pushed, thanks. >> >> Can you know commit this one that fixes missing files in the generated >> tarball >> so that one can build mesa out of the tarball? >> Thx > > I'll commit it soon. ?Thanks. Hi The following patch fix tarball creation again (removed files and one missing Makefile) -- next part -- A non-text attachment was scrubbed... Name: fix-tarball-again.diff Type: text/x-patch Size: 808 bytes Desc: not available URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20110428/e9cc31d2/attachment.bin>
[PATCH 2/3] drm: Warn if vblank state has become inconsistent.
On Wed, 2011-04-27 at 17:50 +0200, Mario Kleiner wrote: > On Apr 27, 2011, at 10:58 AM, dri-devel-request at lists.freedesktop.org > wrote: > > Message: 5 > > Date: Wed, 27 Apr 2011 10:38:14 +0200 > > From: Michel D?nzer > > Subject: Re: [PATCH 2/3] drm: Warn if vblank state has become > > inconsistent. > > To: christopher.halse.rogers at canonical.com > > Cc: dri-devel at lists.freedesktop.org > > Message-ID: <1303893494.5633.129.camel at thor.local> > > Content-Type: text/plain; charset="UTF-8" > > > > On Mit, 2011-04-27 at 16:10 +1000, > > christopher.halse.rogers at canonical.com wrote: > >> From: Christopher James Halse Rogers > >> > >> > >> After emitting all the waiting vblank events no-one should hold > >> a vblank reference. Emit a warning if this is not the case. > >> > >> Signed-off-by: Christopher James Halse Rogers > >> > >> --- > >> drivers/gpu/drm/drm_irq.c |1 + > >> 1 files changed, 1 insertions(+), 0 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > >> index a1f12cb..72407fa 100644 > >> --- a/drivers/gpu/drm/drm_irq.c > >> +++ b/drivers/gpu/drm/drm_irq.c > >> @@ -960,6 +960,7 @@ void drm_vblank_off(struct drm_device *dev, > >> int crtc) > >> e->event.sequence); > >>} > >> > >> + WARN_ON(atomic_read(&dev->vblank_refcount[crtc]) != 0); > >>spin_unlock_irqrestore(&dev->vbl_lock, irqflags); > >> } > >> EXPORT_SYMBOL(drm_vblank_off); > > > > Reviewed-by: Michel D?nzer > > > > > Any pending kms pageflip will also hold a reference on the vblank of > a crtc, so having the refcount non-zero there is not really a sign of > inconsistency, so i'm not sure if a warning is appropriate there. But at this point the vblank interrupts have been disabled, or at least the driver's disable function has been called. Will that not mean that any pending pageflips will wait indefinitely for a vblank interrupt that's not going to come - ie: exactly the state we're warning against? -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 490 bytes Desc: This is a digitally signed message part URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20110428/31165f16/attachment.pgp>
[PATCH] drm/nouveau: Remove interrupt handler around suspend/resume
On Thu, 2011-04-28 at 15:54 +1000, Dave Airlie wrote: > On Wed, 2011-04-27 at 23:20 -0600, Alex Williamson wrote: > > We're often using a shared interrupt line for nouveau, so we have > > to be prepared that it could be called at any point in time. If > > we've suspended the device via vga switcheroo and get a stray > > interrupt on the line from another device, we'll read back -1 from > > the device and head down all sorts of strange paths, most of which > > eventually lock the system. > > > > On my system (Asus UL30VT) the interrupt line is shared with USB. > > Attempting to disable the USB bluetooth device seems to trigger > > a stray interrupt that ends up in nv04_fifo_isr() where we > > eventually hit the "PFIFO still angry after 100 spins, halt", > > which kills the system. > > > > Using free_irq/request_irq around the suspend seems to be a > > reliable fix. Attempting to flag the device state in > > nouvea_irq_handler(), similar to the intel_lid_notify() fix > > is too racy since we can power off the device as an interrupt > > is being processed. > > The actual solution is to check if we read back all Fs and return from > the irq handler. Robust irq handlers are generally considered a good > idea esp around race conditions at suspend/resume time. The trouble I found in trying to do that is that we can still race, having the device be disabled while and interrupt is still being processed. It seems impractical to check every device read through the interrupt path for -1 and back out. Adding a spinlock to the interrupt handler seemed expensive, while this has no additional runtime interrupt overhead. Thanks, Alex
Re: [PATCH] drm/nouveau: Remove interrupt handler around suspend/resume
On Thu, 2011-04-28 at 15:54 +1000, Dave Airlie wrote: > On Wed, 2011-04-27 at 23:20 -0600, Alex Williamson wrote: > > We're often using a shared interrupt line for nouveau, so we have > > to be prepared that it could be called at any point in time. If > > we've suspended the device via vga switcheroo and get a stray > > interrupt on the line from another device, we'll read back -1 from > > the device and head down all sorts of strange paths, most of which > > eventually lock the system. > > > > On my system (Asus UL30VT) the interrupt line is shared with USB. > > Attempting to disable the USB bluetooth device seems to trigger > > a stray interrupt that ends up in nv04_fifo_isr() where we > > eventually hit the "PFIFO still angry after 100 spins, halt", > > which kills the system. > > > > Using free_irq/request_irq around the suspend seems to be a > > reliable fix. Attempting to flag the device state in > > nouvea_irq_handler(), similar to the intel_lid_notify() fix > > is too racy since we can power off the device as an interrupt > > is being processed. > > The actual solution is to check if we read back all Fs and return from > the irq handler. Robust irq handlers are generally considered a good > idea esp around race conditions at suspend/resume time. The trouble I found in trying to do that is that we can still race, having the device be disabled while and interrupt is still being processed. It seems impractical to check every device read through the interrupt path for -1 and back out. Adding a spinlock to the interrupt handler seemed expensive, while this has no additional runtime interrupt overhead. Thanks, Alex ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[git pull] drm fixes
Nothing major, everyone must be on Easter holidays, the i915 one is actually a fix for dual-gpu laptops where i915 caused radeon to do something bad, the Kconfig one is because I see distros don't enable this and its really needed for dual-gpu functionality to work at all. Otherwise, just a couple of minor updates from Alex. Dave. The following changes since commit 8e10cd74342c7f5ce259cceca36f6eba084f5d58: Linux 2.6.39-rc5 (2011-04-26 20:48:50 -0700) are available in the git repository at: ssh://master.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6.git drm-fixes Alex Deucher (2): drm/radeon/kms: add missing safe regs for 6xx/7xx drm/radeon/kms: add info query for tile pipes Dave Airlie (2): drm: select FRAMEBUFFER_CONSOLE_PRIMARY if we have FRAMEBUFFER_CONSOLE drm/i915: restore only the mode of this driver on lastclose (v2) drivers/gpu/drm/Kconfig |1 + drivers/gpu/drm/drm_fb_helper.c | 27 --- drivers/gpu/drm/i915/i915_dma.c |2 +- drivers/gpu/drm/i915/intel_drv.h |1 + drivers/gpu/drm/i915/intel_fb.c | 10 ++ drivers/gpu/drm/radeon/radeon_kms.c | 13 + drivers/gpu/drm/radeon/reg_srcs/r600 |1 + include/drm/drm_fb_helper.h |1 + include/drm/radeon_drm.h |1 + 9 files changed, 49 insertions(+), 8 deletions(-)
Re: [PATCH 1/3] drm: Send pending vblank events before disabling vblank.
On Wed, 2011-04-27 at 11:08 +0200, Michel Dänzer wrote: > On Mit, 2011-04-27 at 18:58 +1000, Christopher James Halse Rogers > wrote: > > On Wed, 2011-04-27 at 10:32 +0200, Michel Dänzer wrote: > > > On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rog...@canonical.com > > > wrote: > > > > From: Christopher James Halse Rogers > > > > > > > > > > > > This is the least-bad behaviour. It means that we signal the > > > > vblank event before it actually happens, but since we're disabling > > > > vblanks there's no guarantee that it will *ever* happen otherwise. > > > > > > This may indeed be the best we can do for events that are pending when > > > the CRTC is disabled[0], but I can't see anything that would prevent new > > > events from getting scheduled (or synchronous vblank waits from timing > > > out) while the CRTC is disabled? > > > > > > [0] Though it might unnecessarily send events prematurely when the CRTC > > > is just disabled temporarily, e.g. as part of a modeset. > > > > > > > > > Also, this patch won't seem to help at all for other drivers which don't > > > call drm_vblank_off() directly when disabling a CRTC. > > > > This is true. On the other hand, the other drivers don't wedge the > > vblank code into a state where vblanks cannot be re-enabled. So it's > > only a problem when disabling one of 2+ monitors on those drivers, > > And with DPMS? > > > whereas it's easily triggerable on single monitor systems on intel. > > Anyway, this patch is probably good at least as a preliminary fix for > the inconsistent vblank state with the intel driver. > > > > > Maybe it would be possible to move those calls to core code, and/or only > > > force sending out events when the CRTC isn't just getting disabled > > > temporarily. > > > > > > > As in: have the core modesetting code call drm_vblank_off before making > > the driver-specific calls when disabling a crtc? I'll look into it - > > that would appear to be a more general solution. > > Yeah, something like that, thanks. > Ok. This appears to be surprisingly difficult. Particularly, the vblank code indexes crtcs based on the driver-private numbering, and there doesn't appear to be a generic interface to grab this number; Intel uses the DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID ioctl, radeon uses something different. I'll see what I can come up with, but I don't think I'm sufficiently familiar with the kms code to quickly come up with a nice API. signature.asc Description: This is a digitally signed message part ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 2/2] drm/nouveau: Use pci_dma_mapping_error()
... instead of comparing with DMA_ERROR_CODE, which will only work on
powerpc/sparc/x86.
Signed-off-by: Aurelien Jarno
---
drivers/gpu/drm/nouveau/nouveau_sgdma.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_sgdma.c
b/drivers/gpu/drm/nouveau/nouveau_sgdma.c
index 4bce801..6a04ac9 100644
--- a/drivers/gpu/drm/nouveau/nouveau_sgdma.c
+++ b/drivers/gpu/drm/nouveau/nouveau_sgdma.c
@@ -42,7 +42,8 @@ nouveau_sgdma_populate(struct ttm_backend *be, unsigned long
num_pages,
nvbe->nr_pages = 0;
while (num_pages--) {
- if (dma_addrs[nvbe->nr_pages] != DMA_ERROR_CODE) {
+ if (!pci_dma_mapping_error(dev->pdev,
+ dma_addrs[nvbe->nr_pages])) {
nvbe->pages[nvbe->nr_pages] =
dma_addrs[nvbe->nr_pages];
nvbe->ttm_alloced[nvbe->nr_pages] = true;
--
1.7.2.3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 1/2] drm/radeon: Use pci_dma_mapping_error()
... instead of comparing with DMA_ERROR_CODE, which will only work on
powerpc/sparc/x86.
Signed-off-by: Aurelien Jarno
---
drivers/gpu/drm/radeon/radeon_gart.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_gart.c
b/drivers/gpu/drm/radeon/radeon_gart.c
index 8a955bb..4de3fb9 100644
--- a/drivers/gpu/drm/radeon/radeon_gart.c
+++ b/drivers/gpu/drm/radeon/radeon_gart.c
@@ -183,7 +183,7 @@ int radeon_gart_bind(struct radeon_device *rdev, unsigned
offset,
for (i = 0; i < pages; i++, p++) {
/* On TTM path, we only use the DMA API if TTM_PAGE_FLAG_DMA32
* is requested. */
- if (dma_addr[i] != DMA_ERROR_CODE) {
+ if (!pci_dma_mapping_error(rdev->pdev, dma_addr[i])) {
rdev->gart.ttm_alloced[p] = true;
rdev->gart.pages_addr[p] = dma_addr[i];
} else {
--
1.7.2.3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/radeon: Use pci_dma_mapping_error()
On Wed, Apr 27, 2011 at 05:49:50PM +1000, Dave Airlie wrote: > On Wed, Apr 27, 2011 at 3:34 PM, Aurelien Jarno wrote: > > ... instead of comparing with DMA_ERROR_CODE, which will only work on > > powerpc/sparc/x86. > > > > So you wrote a patch that breaks it everwhere? It seems I inverted the condition yes. > You might want to actually boot this sort of thing before I do, or > read the interface for pci_dma_mapping_error, it doesn't seem to > return what you seem to think it does. I tried, but I don't have such a card, so it doesn't trigger the bug. I'll send a v2 soon. -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurel...@aurel32.net http://www.aurel32.net ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm: Warn if vblank state has become inconsistent.
On Wed, 2011-04-27 at 17:50 +0200, Mario Kleiner wrote: > On Apr 27, 2011, at 10:58 AM, dri-devel-requ...@lists.freedesktop.org > wrote: > > Message: 5 > > Date: Wed, 27 Apr 2011 10:38:14 +0200 > > From: Michel D?nzer > > Subject: Re: [PATCH 2/3] drm: Warn if vblank state has become > > inconsistent. > > To: christopher.halse.rog...@canonical.com > > Cc: dri-devel@lists.freedesktop.org > > Message-ID: <1303893494.5633.129.camel@thor.local> > > Content-Type: text/plain; charset="UTF-8" > > > > On Mit, 2011-04-27 at 16:10 +1000, > > christopher.halse.rog...@canonical.com wrote: > >> From: Christopher James Halse Rogers > >> > >> > >> After emitting all the waiting vblank events no-one should hold > >> a vblank reference. Emit a warning if this is not the case. > >> > >> Signed-off-by: Christopher James Halse Rogers > >> > >> --- > >> drivers/gpu/drm/drm_irq.c |1 + > >> 1 files changed, 1 insertions(+), 0 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > >> index a1f12cb..72407fa 100644 > >> --- a/drivers/gpu/drm/drm_irq.c > >> +++ b/drivers/gpu/drm/drm_irq.c > >> @@ -960,6 +960,7 @@ void drm_vblank_off(struct drm_device *dev, > >> int crtc) > >> e->event.sequence); > >>} > >> > >> + WARN_ON(atomic_read(&dev->vblank_refcount[crtc]) != 0); > >>spin_unlock_irqrestore(&dev->vbl_lock, irqflags); > >> } > >> EXPORT_SYMBOL(drm_vblank_off); > > > > Reviewed-by: Michel D?nzer > > > > > Any pending kms pageflip will also hold a reference on the vblank of > a crtc, so having the refcount non-zero there is not really a sign of > inconsistency, so i'm not sure if a warning is appropriate there. But at this point the vblank interrupts have been disabled, or at least the driver's disable function has been called. Will that not mean that any pending pageflips will wait indefinitely for a vblank interrupt that's not going to come - ie: exactly the state we're warning against? signature.asc Description: This is a digitally signed message part ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
