On Tue, Jul 26, 2016 at 11:50:23AM +0300, Joonas Lahtinen wrote:
> On ma, 2016-07-25 at 18:32 +0100, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c
> > index 98dc97c8c2bf..b8d541f212ff 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1349,27 +1349,30 @@ int
> > i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj,
> > bool readonly)
> > {
> > + struct drm_i915_gem_request *request;
>
> 'req' is rather de facto. One bad name is better than two names of any
> grade. I see much of your new code is with *request, which direction
> should we have?
>
> >
> > @@ -2383,8 +2386,8 @@ void i915_vma_move_to_active(struct i915_vma *vma,
> > static void
> > i915_gem_object_retire__write(struct drm_i915_gem_object *obj)
> > {
> > - GEM_BUG_ON(!obj->last_write.request);
> > - GEM_BUG_ON(!(obj->active &
> > intel_engine_flag(obj->last_write.request->engine)));
> > + GEM_BUG_ON(!__i915_gem_active_is_busy(&obj->last_write));
> > + GEM_BUG_ON(!(obj->active &
> > intel_engine_flag(i915_gem_active_get_engine(&obj->last_write))));
>
> Already mentioned in previous, long line. You added new functions to
> _gem_active which do useful stuff, then you could nuke the dummy ones?
>
> > @@ -2621,7 +2626,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs
> > *engine)
> > struct drm_i915_gem_object,
> > engine_list[engine->id]);
> >
> > - if (!list_empty(&obj->last_read[engine->id].request->list))
> > + if
> > (!list_empty(&i915_gem_active_peek(&obj->last_read[engine->id])->list))
>
> Long line.
Not touching the long lines in intermediate patches for code that will
be deleted.
> > break;
> >
> > i915_gem_object_retire__read(obj, engine->id);
> > @@ -2754,7 +2759,7 @@ i915_gem_object_flush_active(struct
> > drm_i915_gem_object *obj)
> > for (i = 0; i < I915_NUM_ENGINES; i++) {
> > struct drm_i915_gem_request *req;
> >
> > - req = obj->last_read[i].request;
> > + req = i915_gem_active_peek(&obj->last_read[i]);
> > if (req == NULL)
> > continue;
> >
> > @@ -2794,7 +2799,7 @@ i915_gem_wait_ioctl(struct drm_device *dev, void
> > *data, struct drm_file *file)
> > {
> > struct drm_i915_gem_wait *args = data;
> > struct drm_i915_gem_object *obj;
> > - struct drm_i915_gem_request *req[I915_NUM_ENGINES];
> > + struct drm_i915_gem_request *requests[I915_NUM_ENGINES];
>
> I think this answers my previous question.
>
> <SNIP>
>
> > + struct drm_i915_gem_request *req;
> >
> >
> > n = 0;
> > if (readonly) {
> > - if (obj->last_write.request)
> > - req[n++] = obj->last_write.request;
> > + struct drm_i915_gem_request *req;
> > +
> > + req = i915_gem_active_peek(&obj->last_write);
> > + if (req)
> > + requests[n++] = req;
> > } else {
> > - for (i = 0; i < I915_NUM_ENGINES; i++)
> > - if (obj->last_read[i].request)
> > - req[n++] = obj->last_read[i].request;
> > + for (i = 0; i < I915_NUM_ENGINES; i++) {
> > + struct drm_i915_gem_request *req;
>
> But some consistency is lacking with dem names. How's it going to be?
What I always had was requests for member names, rq for locals. But
apparently rq was too much like run-queue, which is how we use the
requests! In userspace I always used requests for members and rq for
locals.
Where I have been adding functions, I have been using request as the
local as well. Where I have been inheriting the ugly name, it has mostly
stuck.
> > +static inline uint32_t
> > +i915_gem_active_get_seqno(const struct i915_gem_active *active)
> > +{
> > + return i915_gem_request_get_seqno(i915_gem_active_peek(active));
>
> Nuke the i915_gem_request_get_seqno wrapper, it's insanity. Now or an
> another patch.
They are (or will be) only used in a couple of places where the NULL
guard is required.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
