On Tue, Jan 31, 2012 at 02:13:00PM -0500, Alex Deucher wrote:
> On Tue, Jan 31, 2012 at 2:07 PM, Jerome Glisse <j.glisse at gmail.com> 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
I think the idea was to return u64 value of specific ring and only
enable irq if new wait cs ioctl was call, a little bit like bo wait.
Will try to dig through my mail too.
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 at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
