Re: [PATCH 3/3] drm/radeon: fix const IB handling

["Christian König" <deathsimple@vodafone.de>]

On 13.07.2012 16:17, Tom Stellard wrote:
> On Fri, Jul 13, 2012 at 04:08:15PM +0200, Christian König wrote:
>> Const IBs are executed on the CE not the CP, so we can't
>> fence them in the normal way.
>>
>> So submit them directly before the IB instead, just as
>> the documentation says.
>>
>> Signed-off-by: Christian König <deathsimple@vodafone.de>
>> ---
>>   drivers/gpu/drm/radeon/r100.c        |    2 +-
>>   drivers/gpu/drm/radeon/r600.c        |    2 +-
>>   drivers/gpu/drm/radeon/radeon.h      |    3 ++-
>>   drivers/gpu/drm/radeon/radeon_cs.c   |   25 +++++++++++--------------
>>   drivers/gpu/drm/radeon/radeon_ring.c |   10 +++++++++-
>>   5 files changed, 24 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c
>> index e0f5ae8..4ee5a74 100644
>> --- a/drivers/gpu/drm/radeon/r100.c
>> +++ b/drivers/gpu/drm/radeon/r100.c
>> @@ -3693,7 +3693,7 @@ int r100_ib_test(struct radeon_device *rdev, struct radeon_ring *ring)
>>   	ib.ptr[6] = PACKET2(0);
>>   	ib.ptr[7] = PACKET2(0);
>>   	ib.length_dw = 8;
>> -	r = radeon_ib_schedule(rdev, &ib);
>> +	r = radeon_ib_schedule(rdev, &ib, NULL);
>>   	if (r) {
>>   		radeon_scratch_free(rdev, scratch);
>>   		radeon_ib_free(rdev, &ib);
>> diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
>> index 3156d25..c2e5069 100644
>> --- a/drivers/gpu/drm/radeon/r600.c
>> +++ b/drivers/gpu/drm/radeon/r600.c
>> @@ -2619,7 +2619,7 @@ int r600_ib_test(struct radeon_device *rdev, struct radeon_ring *ring)
>>   	ib.ptr[1] = ((scratch - PACKET3_SET_CONFIG_REG_OFFSET) >> 2);
>>   	ib.ptr[2] = 0xDEADBEEF;
>>   	ib.length_dw = 3;
>> -	r = radeon_ib_schedule(rdev, &ib);
>> +	r = radeon_ib_schedule(rdev, &ib, NULL);
>>   	if (r) {
>>   		radeon_scratch_free(rdev, scratch);
>>   		radeon_ib_free(rdev, &ib);
>> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
>> index 2cb355b..2d7f06c 100644
>> --- a/drivers/gpu/drm/radeon/radeon.h
>> +++ b/drivers/gpu/drm/radeon/radeon.h
>> @@ -751,7 +751,8 @@ struct si_rlc {
>>   int radeon_ib_get(struct radeon_device *rdev, int ring,
>>   		  struct radeon_ib *ib, unsigned size);
>>   void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib);
>> -int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib);
>> +int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib,
>> +		       struct radeon_ib *const_ib);
>>   int radeon_ib_pool_init(struct radeon_device *rdev);
>>   void radeon_ib_pool_fini(struct radeon_device *rdev);
>>   int radeon_ib_ring_tests(struct radeon_device *rdev);
>> diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c
>> index 553da67..d0be5d5 100644
>> --- a/drivers/gpu/drm/radeon/radeon_cs.c
>> +++ b/drivers/gpu/drm/radeon/radeon_cs.c
>> @@ -354,7 +354,7 @@ static int radeon_cs_ib_chunk(struct radeon_device *rdev,
>>   	}
>>   	radeon_cs_sync_rings(parser);
>>   	parser->ib.vm_id = 0;
>> -	r = radeon_ib_schedule(rdev, &parser->ib);
>> +	r = radeon_ib_schedule(rdev, &parser->ib, NULL);
>>   	if (r) {
>>   		DRM_ERROR("Failed to schedule IB !\n");
>>   	}
>> @@ -452,25 +452,22 @@ static int radeon_cs_ib_vm_chunk(struct radeon_device *rdev,
>>   	}
>>   	radeon_cs_sync_rings(parser);
>>   
>> +	parser->ib.vm_id = vm->id;
>> +	/* ib pool is bind at 0 in virtual address space,
>> +	 * so gpu_addr is the offset inside the pool bo
>> +	 */
>> +	parser->ib.gpu_addr = parser->ib.sa_bo->soffset;
>> +
>>   	if ((rdev->family >= CHIP_TAHITI) &&
>>   	    (parser->chunk_const_ib_idx != -1)) {
>>   		parser->const_ib.vm_id = vm->id;
>> -		/* ib pool is bind at 0 in virtual address space to gpu_addr is the
>> -		 * offset inside the pool bo
>> -		 */
>> +		/* same reason as above */
>>   		parser->const_ib.gpu_addr = parser->const_ib.sa_bo->soffset;
>> -		r = radeon_ib_schedule(rdev, &parser->const_ib);
>> -		if (r)
>> -			goto out;
>> +		r = radeon_ib_schedule(rdev, &parser->ib, &parser->const_ib);
>> +	} else {
>> +		r = radeon_ib_schedule(rdev, &parser->ib, NULL);
>>   	}
>>   
>> -	parser->ib.vm_id = vm->id;
>> -	/* ib pool is bind at 0 in virtual address space to gpu_addr is the
>> -	 * offset inside the pool bo
>> -	 */
>> -	parser->ib.gpu_addr = parser->ib.sa_bo->soffset;
>> -	parser->ib.is_const_ib = false;
>> -	r = radeon_ib_schedule(rdev, &parser->ib);
>>   out:
>>   	if (!r) {
>>   		if (vm->fence) {
>> diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c
>> index 75cbe46..c48c354 100644
>> --- a/drivers/gpu/drm/radeon/radeon_ring.c
>> +++ b/drivers/gpu/drm/radeon/radeon_ring.c
>> @@ -74,7 +74,8 @@ void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib)
>>   	radeon_fence_unref(&ib->fence);
>>   }
>>
>> -int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib)
>> +int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib,
>> +		       struct radeon_ib *const_ib)
> Since you are modifying an uncommented function, comments should be
> added, per the new documentation rules.
>
> I don't mean to be picky, but there is no point having documentation
> rules if they aren't enforced.
Oh no, please be picky about it. Otherwise I wouldn't learn it.

For this particular change Alex already had appropriate documentation in 
the pipeline and I wanted to rather change them instead of adding 
documentation in this patch.

Christian.

>
>>   {
>>   	struct radeon_ring *ring = &rdev->ring[ib->ring];
>>   	bool need_sync = false;
>> @@ -105,6 +106,10 @@ int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib)
>>   	if (!need_sync) {
>>   		radeon_semaphore_free(rdev, &ib->semaphore, NULL);
>>   	}
>> +	if (const_ib) {
>> +		radeon_ring_ib_execute(rdev, const_ib->ring, const_ib);
>> +		radeon_semaphore_free(rdev, &const_ib->semaphore, NULL);
>> +	}
>>   	radeon_ring_ib_execute(rdev, ib->ring, ib);
>>   	r = radeon_fence_emit(rdev, &ib->fence, ib->ring);
>>   	if (r) {
>> @@ -112,6 +117,9 @@ int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib)
>>   		radeon_ring_unlock_undo(rdev, ring);
>>   		return r;
>>   	}
>> +	if (const_ib) {
>> +		const_ib->fence = radeon_fence_ref(ib->fence);
>> +	}
>>   	radeon_ring_unlock_commit(rdev, ring);
>>   	return 0;
>>   }
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>