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

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

On 17.07.2012 16:25, Jerome Glisse wrote:
> On Tue, Jul 17, 2012 at 5:50 AM, Christian König
> <deathsimple@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@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.
That was in the other patch, and yeah your right I changed that one.

Christian.

>
> Cheers,
> Jerome
>