Direct Rendering Infrastructure

It's a horrible hack, basically.  I used to understand it but now no 
longer want to.  I think the original DRI documentation will have some 
information on it.

I think the original motiviation was for the gamma (and actually lots of 
subsequent drivers), you end up encoding the window position and 
cliprects in the command stream.  And this lock somehow protected those 
command streams from being submitted to hardware with out-of-date window 
positions.

In fact if you go right back, the DRM supported multiple software 
command stream where commands from the X server could be prioritized 
ahead of commands from 3d clients, so you could get the window-move 
blits being submitted to hardware ahead of 3d drawing commands to the 
old window position.

This created all sorts of amazing problems which have subsequently been 
avoided by simply submitting all commands to hardware in the order they 
are generated.

Right now, no driver uses the spinlock directly or explicitly, but they 
all use it when requesting their cliprects from the X server.

So basically the sequence goes:
	
	Client gets dri lock
	Client sees timestamp changed
	Client wants to talk to the X server to get new cliprects, but because 
client is holding the lock, the X server is unable to respond to the 
client.  So the client needs to release the lock to let the X server reply.
		--> But! (some set of circumstances might apply here), so there's a 
need for this funny other lock, that the client grabs before it releases 
the main dri lock, to stop the cliprects getting changed again in the 
meantime.

I don't see why you couldn't just do something like

	get lock
	while (timestamp mismatch) {
		release lock
		request new cliprects and timestamp
		get lock
	}

Note that is the contended case only.  What's the worst that could 
happen - somebody's wizzing windows around and our 3d client sits in 
this loop for the duration.  Note that the loop includes X server 
communication so it's not going to suck up the cpu or anything drastic.

In some respects it's like the code in the radeon DDX driver which uses 
a software path to perform depth buffer copying on window moves - well 
intentioned perhaps, but not actually useful or desirable.

Keith

Thomas Hellstrom wrote:
> Hi!
> 
> Does anybody have a clear understanding of the drawable spinlock?
> 
>  From my reading of code in the X server and dri_utilities.c it is ment 
> to be used to stop anyone but the context holding the lock to touch the 
> dri drawables in a way that would change their timestamp.
> 
> The X server has a very inefficient way of checking whether a client 
> died while holding the drawable spinlock. It waits for 10 seconds and 
> then grabs it by force.
> 
> Also the usage is dri_util.c is beyond my understanding. Basically, to 
> lock and validate drawable info, the following happens:
> 
> get_heavyweight_lock;
> 
> while drawable_stamps_mismatch {
>   release_heavyweight_lock;
>   get_drawable_spinlock;
>    //In dri_util.c
>   do_some_minor_processing_that_can_be_done_elsewhere;
>   release_drawable_spinlock;
>   call_X_to_update_drawable_info;
>   get_drawable_spinlock;
>   //In driver.
>   release_drawable_spinlock;
> }
> 
> Basically no driver seems to be using it for anything, except possibly 
> the gamma driver, which I figure is outdated anyway?
> 
> I have found some use for it in XvMC clients: To run the scaling engine 
> to render to a drawable without holding the heavyweight lock for 
> prolonged periods, but I strongly dislike the idea of freezing the X 
> server for 10 secs if the XvMC client accidently dies.
> 
> Proposed changes:
> 
> 1). Could we replace the locking value (which now seems to be 1) with 
> the context number | _DRM_LOCK_HELD. In this way the drm can detect when 
> the drawable lock is held by a killed client and release it.
> 
> 2). Could we replace the drawable_spinlock with a futex-like lock 
> similar to what's used in the via drm to reserve certain chip functions. 
> The purpose would be to sched_yield() if the lock is contended, with an 
> immediate wakeup when the lock is released. This would not be backwards 
> binary compatible with other drivers, but it seems no up-to-date drivers 
> is using this lock anyway.
> 
> /Thomas
> 
> 
> 
> _______________________________________________
> xorg mailing list
> xorg@...
> http://lists.freedesktop.org/mailman/listinfo/xorg
>

Hi!

Keith Whitwell wrote:

>
>     get lock
>     while (timestamp mismatch) {
>         release lock
>         request new cliprects and timestamp
>         get lock
>     }
>
> Note that is the contended case only.  What's the worst that could 
> happen - somebody's wizzing windows around and our 3d client sits in 
> this loop for the duration.  Note that the loop includes X server 
> communication so it's not going to suck up the cpu or anything drastic.

This is basically what I'm doing right now. The problem is as the code 
continues:


    get lock
    while (timestamp mismatch) {
        release lock
        request new cliprects and timestamp
        get lock
    }
    wait_for_device()
    render_to_scale_buffer()
    wait_for_device()
    render_to_back_buffer()
    wiat_for_device()
    blit_to_screen()
    release_lock()

And, to avoid holding the lock while waiting for the device, since that 
blocks use of the decoder while I'm doing scaling operations, I'd like to

mark_scaling_device_busy()
get_drawable_lock()
get lock
 while (timestamp mismatch) {
    release lock
    release_drawable_lock()
    request new cliprects and timestamp
    get_drawable_lock
    get lock
  }
release_lock()
wait_for_device()
get_lock()
render_to_scale_buffer()
release_lock()
wait_for_device()
get_lock()
render_to_back_buffer()
release_lock()
wait_for_device()
get_lock()
blit_to_screen()
release_lock()
mark_scaling_device_free()

/Thomas

Thomas Hellstr=F6m wrote:
> Hi!
>=20
> Keith Whitwell wrote:
>=20
>>
>>     get lock
>>     while (timestamp mismatch) {
>>         release lock
>>         request new cliprects and timestamp
>>         get lock
>>     }
>>
>> Note that is the contended case only.  What's the worst that could=20
>> happen - somebody's wizzing windows around and our 3d client sits in=20
>> this loop for the duration.  Note that the loop includes X server=20
>> communication so it's not going to suck up the cpu or anything drastic=
.
>=20
>=20
> This is basically what I'm doing right now. The problem is as the code=20
> continues:
>=20
>=20
>    get lock
>    while (timestamp mismatch) {
>        release lock
>        request new cliprects and timestamp
>        get lock
>    }
>    wait_for_device()
>    render_to_scale_buffer()
>    wait_for_device()
>    render_to_back_buffer()
>    wiat_for_device()
>    blit_to_screen()
>    release_lock()
>=20
> And, to avoid holding the lock while waiting for the device, since that=
=20
> blocks use of the decoder while I'm doing scaling operations, I'd like =
to
>=20
> mark_scaling_device_busy()
> get_drawable_lock()
> get lock
> while (timestamp mismatch) {
>    release lock
>    release_drawable_lock()
>    request new cliprects and timestamp
>    get_drawable_lock
>    get lock
>  }
> release_lock()
> wait_for_device()
> get_lock()
> render_to_scale_buffer()
> release_lock()
> wait_for_device()
> get_lock()
> render_to_back_buffer()
> release_lock()
> wait_for_device()
> get_lock()
> blit_to_screen()
> release_lock()
> mark_scaling_device_free()

And then release_drawable_lock()?

What semantics are you hoping for from the drawable lock in your=20
scenario above?  Just that the cliprects won't change while it is held?

Keith

Keith Whitwell wrote:

> Thomas Hellstr=F6m wrote:
>
>> Hi!
>>
>> Keith Whitwell wrote:
>>
>>>
>>>     get lock
>>>     while (timestamp mismatch) {
>>>         release lock
>>>         request new cliprects and timestamp
>>>         get lock
>>>     }
>>>
>>> Note that is the contended case only.  What's the worst that could=20
>>> happen - somebody's wizzing windows around and our 3d client sits in=20
>>> this loop for the duration.  Note that the loop includes X server=20
>>> communication so it's not going to suck up the cpu or anything drasti=
c.
>>
>>
>>
>> This is basically what I'm doing right now. The problem is as the=20
>> code continues:
>>
>>
>>    get lock
>>    while (timestamp mismatch) {
>>        release lock
>>        request new cliprects and timestamp
>>        get lock
>>    }
>>    wait_for_device()
>>    render_to_scale_buffer()
>>    wait_for_device()
>>    render_to_back_buffer()
>>    wiat_for_device()
>>    blit_to_screen()
>>    release_lock()
>>
>> And, to avoid holding the lock while waiting for the device, since=20
>> that blocks use of the decoder while I'm doing scaling operations,=20
>> I'd like to
>>
>> mark_scaling_device_busy()
>> get_drawable_lock()
>> get lock
>> while (timestamp mismatch) {
>>    release lock
>>    release_drawable_lock()
>>    request new cliprects and timestamp
>>    get_drawable_lock
>>    get lock
>>  }
>> release_lock()
>> wait_for_device()
>> get_lock()
>> render_to_scale_buffer()
>> release_lock()
>> wait_for_device()
>> get_lock()
>> render_to_back_buffer()
>> release_lock()
>> wait_for_device()
>> get_lock()
>> blit_to_screen()
>> release_lock()
>> mark_scaling_device_free()
>
>
> And then release_drawable_lock()?
>
> What semantics are you hoping for from the drawable lock in your=20
> scenario above?  Just that the cliprects won't change while it is held?

Exactly on both points, except the drawable_lock would have to be=20
released before mark_scaling_device_free() to avoid deadlocks.

>
> Keith

Thomas Hellstrom wrote:
> Hi!
> 
> Does anybody have a clear understanding of the drawable spinlock?
> 
>  From my reading of code in the X server and dri_utilities.c it is ment 
> to be used to stop anyone but the context holding the lock to touch the 
> dri drawables in a way that would change their timestamp.
> 
> The X server has a very inefficient way of checking whether a client 
> died while holding the drawable spinlock. It waits for 10 seconds and 
> then grabs it by force.
> 
> Also the usage is dri_util.c is beyond my understanding. Basically, to 
> lock and validate drawable info, the following happens:
> 
> get_heavyweight_lock;
> 
> while drawable_stamps_mismatch {
>   release_heavyweight_lock;
>   get_drawable_spinlock;
>    //In dri_util.c
>   do_some_minor_processing_that_can_be_done_elsewhere;
>   release_drawable_spinlock;
>   call_X_to_update_drawable_info;
>   get_drawable_spinlock;
>   //In driver.
>   release_drawable_spinlock;
> }
> 
> Basically no driver seems to be using it for anything, except possibly 
> the gamma driver, which I figure is outdated anyway?
> 
> I have found some use for it in XvMC clients: To run the scaling engine 
> to render to a drawable without holding the heavyweight lock for 
> prolonged periods, but I strongly dislike the idea of freezing the X 
> server for 10 secs if the XvMC client accidently dies.
> 
> Proposed changes:
> 
> 1). Could we replace the locking value (which now seems to be 1) with 
> the context number | _DRM_LOCK_HELD. In this way the drm can detect when 
> the drawable lock is held by a killed client and release it.

This seems like a reasonable thing to do now.  Note that the X server 
could also be the one responsible for freeing the lock, which might be 
cleaner if the DRM is currenly unaware of this beast.

> 2). Could we replace the drawable_spinlock with a futex-like lock 
> similar to what's used in the via drm to reserve certain chip functions. 
> The purpose would be to sched_yield() if the lock is contended, with an 
> immediate wakeup when the lock is released. This would not be backwards 
> binary compatible with other drivers, but it seems no up-to-date drivers 
> is using this lock anyway.

Maybe this is something to put off to be part of Ian's forthcoming 
binary-compatibility breakages for X.org 6.7?

Keith

Thomas Hellstr=F6m wrote:
> Keith Whitwell wrote:
>=20
>> Thomas Hellstr=F6m wrote:
>>
>>> Hi!
>>>
>>> Keith Whitwell wrote:
>>>
>>>>
>>>>     get lock
>>>>     while (timestamp mismatch) {
>>>>         release lock
>>>>         request new cliprects and timestamp
>>>>         get lock
>>>>     }
>>>>
>>>> Note that is the contended case only.  What's the worst that could=20
>>>> happen - somebody's wizzing windows around and our 3d client sits in=
=20
>>>> this loop for the duration.  Note that the loop includes X server=20
>>>> communication so it's not going to suck up the cpu or anything drast=
ic.
>>>
>>>
>>>
>>>
>>> This is basically what I'm doing right now. The problem is as the=20
>>> code continues:
>>>
>>>
>>>    get lock
>>>    while (timestamp mismatch) {
>>>        release lock
>>>        request new cliprects and timestamp
>>>        get lock
>>>    }
>>>    wait_for_device()
>>>    render_to_scale_buffer()
>>>    wait_for_device()
>>>    render_to_back_buffer()
>>>    wiat_for_device()
>>>    blit_to_screen()
>>>    release_lock()
>>>
>>> And, to avoid holding the lock while waiting for the device, since=20
>>> that blocks use of the decoder while I'm doing scaling operations,=20
>>> I'd like to
>>>
>>> mark_scaling_device_busy()
>>> get_drawable_lock()
>>> get lock
>>> while (timestamp mismatch) {
>>>    release lock
>>>    release_drawable_lock()
>>>    request new cliprects and timestamp
>>>    get_drawable_lock
>>>    get lock
>>>  }
>>> release_lock()
>>> wait_for_device()
>>> get_lock()
>>> render_to_scale_buffer()
>>> release_lock()
>>> wait_for_device()
>>> get_lock()
>>> render_to_back_buffer()
>>> release_lock()
>>> wait_for_device()
>>> get_lock()
>>> blit_to_screen()
>>> release_lock()
>>> mark_scaling_device_free()
>>
>>
>>
>> And then release_drawable_lock()?
>>
>> What semantics are you hoping for from the drawable lock in your=20
>> scenario above?  Just that the cliprects won't change while it is held=
?
>=20
>=20
> Exactly on both points, except the drawable_lock would have to be=20
> released before mark_scaling_device_free() to avoid deadlocks.


So a few more questions:

1) Why (exactly) is keeping the cliprects from changing a concern?  What=20
happens if they change between steps above?

2) Could the DDX driver blit the contents of these additional buffers=20
(scale, back) at the same time it blits the frontbuffer so that the=20
window change "just works"?

3) I don't think that the drawable lock is a pretty thing, is it worth=20
keeping it around for this?  Would some black areas or incomplete video=20
frames during window moves be so bad?  Note that the next version of=20
this hardware might have a proper command stream that just allows you to=20
submit all those operations to hardware in a single go, and not have to=20
do the waiting in the driver...


Keith

Keith Whitwell wrote:

> Thomas Hellstr=F6m wrote:
>
>> Keith Whitwell wrote:
>>
>>> Thomas Hellstr=F6m wrote:
>>>
>>>> Hi!
>>>>
>>>> Keith Whitwell wrote:
>>>>
>>>>>
>>>>>     get lock
>>>>>     while (timestamp mismatch) {
>>>>>         release lock
>>>>>         request new cliprects and timestamp
>>>>>         get lock
>>>>>     }
>>>>>
>>>>> Note that is the contended case only.  What's the worst that could=20
>>>>> happen - somebody's wizzing windows around and our 3d client sits=20
>>>>> in this loop for the duration.  Note that the loop includes X=20
>>>>> server communication so it's not going to suck up the cpu or=20
>>>>> anything drastic.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> This is basically what I'm doing right now. The problem is as the=20
>>>> code continues:
>>>>
>>>>
>>>>    get lock
>>>>    while (timestamp mismatch) {
>>>>        release lock
>>>>        request new cliprects and timestamp
>>>>        get lock
>>>>    }
>>>>    wait_for_device()
>>>>    render_to_scale_buffer()
>>>>    wait_for_device()
>>>>    render_to_back_buffer()
>>>>    wiat_for_device()
>>>>    blit_to_screen()
>>>>    release_lock()
>>>>
>>>> And, to avoid holding the lock while waiting for the device, since=20
>>>> that blocks use of the decoder while I'm doing scaling operations,=20
>>>> I'd like to
>>>>
>>>> mark_scaling_device_busy()
>>>> get_drawable_lock()
>>>> get lock
>>>> while (timestamp mismatch) {
>>>>    release lock
>>>>    release_drawable_lock()
>>>>    request new cliprects and timestamp
>>>>    get_drawable_lock
>>>>    get lock
>>>>  }
>>>> release_lock()
>>>> wait_for_device()
>>>> get_lock()
>>>> render_to_scale_buffer()
>>>> release_lock()
>>>> wait_for_device()
>>>> get_lock()
>>>> render_to_back_buffer()
>>>> release_lock()
>>>> wait_for_device()
>>>> get_lock()
>>>> blit_to_screen()
>>>> release_lock()
>>>> mark_scaling_device_free()
>>>
>>>
>>>
>>>
>>> And then release_drawable_lock()?
>>>
>>> What semantics are you hoping for from the drawable lock in your=20
>>> scenario above?  Just that the cliprects won't change while it is hel=
d?
>>
>>
>>
>> Exactly on both points, except the drawable_lock would have to be=20
>> released before mark_scaling_device_free() to avoid deadlocks.
>
>
>
> So a few more questions:
>
> 1) Why (exactly) is keeping the cliprects from changing a concern? =20
> What happens if they change between steps above?

Not much really. It all boils down to what to do if the per-drawable=20
back-buffer mismatches the drawable. In the simplest case one would=20
simply skip the blit, which might even be better than an attempt to=20
match the old backbuffer to a new drawable size. Problems really only=20
occur if  / when the drawable is resized. But I've considered this and=20
since it's a simple, still working but not perfect solution, I'm still=20
considering it.

>
> 2) Could the DDX driver blit the contents of these additional buffers=20
> (scale, back) at the same time it blits the frontbuffer so that the=20
> window change "just works"?
>
You mean the front blitting during window moves? In this case it doesn't=20
relly apply, since the per-drawable back buffer would still be valid.=20
Resizing would be the only operation causing problems.

> 3) I don't think that the drawable lock is a pretty thing, is it worth=20
> keeping it around for this?  Would some black areas or incomplete=20
> video frames during window moves be so bad?  Note that the next=20
> version of this hardware might have a proper command stream that just=20
> allows you to submit all those operations to hardware in a single go,=20
> and not have to do the waiting in the driver...
>
I can see your point, but on the other hand if the command stream were=20
that smart, it would only in effect implement a continously held=20
heavyweight lock, blocking all dma submissions to the mpeg decoder and=20
2D / 3D engine while the scaling engine is working, which is exactly=20
what I'm trying to avoid.

The problem is really what to do when there are a lot of independent=20
engines on a video chip, with a common command stream, numerous IRQ=20
sources and one global hardware lock. I assume this will be more of a=20
problem in the future. The solution using the drawable lock is not very=20
clean. On the other hand, not being able no to use the engines in=20
parallel is not very efficient and is bad for interactivity.

I'm not sure what's the best design to solve this, but one idea would be=20
having a futex-like lock and a "breadcrumb pool" for each engine,=20
optionally also with an IRQ. This would be sufficient to

    * be able to independently submit DMA commands.
    * wait for engine idle independently on each engine without ever
      needing to wait for DMA quiescent.
    * hold the global hardware lock only during operations that render
      directly to the front buffer, or to a common back-buffer. The
      global lock would then effectively be a "drawable" lock.
    * Keep backwards compatibility, as simple architectures may choose
      to retain only the global lock.

Hmm, maybe for now I'll stick to the simple solution, :)

But I think a design that works around the=20
single-lock-and-command-stream-multiple-engines would bee needed in the=20
not too far future.

/Thomas

>
> Keith