On Tue, Jan 31, 2012 at 2:07 PM, Jerome Glisse <j.glisse@xxxxxxxxx> wrote:
> On Tue, Jan 31, 2012 at 01:55:43PM -0500, David Airlie wrote:
>>
>>
>> Some comments below.
>>
>> > + struct radeon_device *rdev = dev->dev_private;
>> > + struct drm_gem_object *gobj;
>> > + struct radeon_bo *robj;
>> > + void *buffer_data;
>> > + uint32_t *fence_data;
>> > + int r = 0;
>> > + long timeout;
>> > + int ring = RADEON_RING_TYPE_GFX_INDEX;
>> > +
>> > + /* If you're implementing this for other rings, you'll want to
>> > share
>> > + code with radeon_cs_get_ring in radeon_cs.c */
>> > + if (args->ring != RADEON_CS_RING_GFX) {
>> > + return -EINVAL;
>> > + }
>> > +
>> > + gobj = drm_gem_object_lookup(dev, filp, args->handle);
>> > + if (gobj == NULL) {
>> > + return -ENOENT;
>> > + }
>> > + robj = gem_to_radeon_bo(gobj);
>> > +
>> > + if (gobj->size < args->offset) {
>> > + r = -EINVAL;
>> > + goto unreference;
>> > + }
>> > +
>> > + r = radeon_bo_reserve(robj, true);
>> > + if (r) {
>> > + goto unreference;
>> > + }
>> > +
>> > + r = radeon_bo_pin(robj, RADEON_GEM_DOMAIN_GTT, NULL);
>> > + if (r) {
>> > + goto unreserve;
>> > + }
>> > +
>> > + r = radeon_bo_kmap(robj, &buffer_data);
>> > + if (r) {
>> > + goto unpin;
>> > + }
>> > +
>>
>>
>> Do you need to pin it? I think if you have it reserved and you are in here you shouldn't need to. (unless kmapping requires a pin)/
>
> No you don't need to pin, actually it's bad to pin the buffer you might
> want to wait on might be in vram.
>
> Also you never assume that things could go wrong and that your value
> might never be written to the buffer. So it will wait forever in the
> kernel.
>
> As i said in the other mail i would rather as a wait on submited cs
> ioctl and add some return value from cs ioctl. I can't remember the
> outcome of this discusion we had when we were doing virtual memory
> support. Alex ? Michel ? better memory than i do ? :)
>
I vaguely recall the discussion, but I don't remember the details,
I'll have to look through my old mail. Maybe a new CS ioctl flag
requesting EOP irqs for the CS would be a better approach than a
separate ioctl.
Alex
> Cheers,
> Jerome
>
>>
>>
>> > + radeon_irq_kms_sw_irq_get(rdev, ring);
>> > +
>> > + fence_data = (uint32_t*)buffer_data;
>> > + timeout =
>> > an
>> > + * interrupt and the value in the buffer might have changed.
>> > + */
>> > +struct drm_radeon_gem_wait_user_fence {
>> > + uint32_t handle;
>> > + uint32_t ring;
>> > + uint64_t offset;
>> > + uint32_t value;
>> > + uint64_t timeout_usec;
>>
>> Align things here, 64 then 64 then 32 32 32 and a 32 pad.
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
