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

[Jerome Glisse]

On Tue, Jul 17, 2012 at 5:50 AM, Christian König
<deathsimple at vodafone.de> wrote:
> 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 at 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.

Still my earlier remark matter, i would rather not see cross comment
reference it's confusing especially if code move. I rather see the
same comment 2 times.

Cheers,
Jerome