[PATCH 5/6] drm/amdgpu: add timeline support in amdgpu CS

[Zhou, David(ChunMing)]

> -----Original Message-----
> From: Nicolai Hähnle <nhaehnle at gmail.com>
> Sent: Wednesday, September 26, 2018 5:06 PM
> To: Zhou, David(ChunMing) <David1.Zhou at amd.com>; dri-
> devel at lists.freedesktop.org
> Cc: amd-gfx at lists.freedesktop.org
> Subject: Re: [PATCH 5/6] drm/amdgpu: add timeline support in amdgpu CS
> 
> Hey Chunming,
> 
> On 20.09.2018 13:03, Chunming Zhou wrote:
> > @@ -1113,48 +1117,91 @@ static int
> amdgpu_syncobj_lookup_and_add_to_sync(struct amdgpu_cs_parser *p,
> >   }
> >
> >   static int amdgpu_cs_process_syncobj_in_dep(struct amdgpu_cs_parser
> *p,
> > -					    struct amdgpu_cs_chunk *chunk)
> > +					    struct amdgpu_cs_chunk *chunk,
> > +					    bool timeline)
> >   {
> >   	unsigned num_deps;
> >   	int i, r;
> > -	struct drm_amdgpu_cs_chunk_sem *deps;
> >
> > -	deps = (struct drm_amdgpu_cs_chunk_sem *)chunk->kdata;
> > -	num_deps = chunk->length_dw * 4 /
> > -		sizeof(struct drm_amdgpu_cs_chunk_sem);
> > +	if (!timeline) {
> > +		struct drm_amdgpu_cs_chunk_sem *deps;
> >
> > -	for (i = 0; i < num_deps; ++i) {
> > -		r = amdgpu_syncobj_lookup_and_add_to_sync(p,
> deps[i].handle);
> > +		deps = (struct drm_amdgpu_cs_chunk_sem *)chunk->kdata;
> > +		num_deps = chunk->length_dw * 4 /
> > +			sizeof(struct drm_amdgpu_cs_chunk_sem);
> > +		for (i = 0; i < num_deps; ++i) {
> > +			r = amdgpu_syncobj_lookup_and_add_to_sync(p,
> deps[i].handle,
> > +								  0, 0);
> >   		if (r)
> >   			return r;
> 
> The indentation looks wrong.
> 
> 
> > +		}
> > +	} else {
> > +		struct drm_amdgpu_cs_chunk_syncobj *syncobj_deps;
> > +
> > +		syncobj_deps = (struct drm_amdgpu_cs_chunk_syncobj
> *)chunk->kdata;
> > +		num_deps = chunk->length_dw * 4 /
> > +			sizeof(struct drm_amdgpu_cs_chunk_syncobj);
> > +		for (i = 0; i < num_deps; ++i) {
> > +			r = amdgpu_syncobj_lookup_and_add_to_sync(p,
> syncobj_deps[i].handle,
> > +
> syncobj_deps[i].point,
> > +
> syncobj_deps[i].flags);
> > +		if (r)
> > +			return r;
> 
> Here as well.
> 
> So I'm wondering a bit about this uapi. Specifically, what happens if you try to
> use timeline syncobjs here as dependencies _without_
> DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT?
> 
> My understanding is, it'll just return -EINVAL without any indication as to
> which syncobj actually failed. What's the caller supposed to do then?

How about adding a print to indicate which syncobj failed?

Thanks,
David Zhou
> 
> Cheers,
> Nicolai
> --
> Lerne, wie die Welt wirklich ist,
> Aber vergiss niemals, wie sie sein sollte.