Re: [PATCH libdrm v2 1/2] intel: Add EXEC_OBJECT_SUPPORTS_48B_ADDRESS flag.

[Michel Thierry <michel.thierry@intel.com>]

On 7/1/2015 6:06 PM, Emil Velikov wrote:
> Hi Michel,
>
> Although I cannot comment on the exact implementation I can give you
> general some tips which you might find useful.
>
Hi Emil,

> On 1 July 2015 at 16:28, Michel Thierry <michel.thierry@intel.com> wrote:
>> Gen8+ supports 48-bit virtual addresses, but some objects must always be
>> allocated inside the 32-bit address range.
>>
>> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
>> index ded43b1..426b25c 100644
>> --- a/include/drm/i915_drm.h
>> +++ b/include/drm/i915_drm.h
>> @@ -680,7 +680,8 @@ struct drm_i915_gem_exec_object2 {
>>   #define EXEC_OBJECT_NEEDS_FENCE (1<<0)
>>   #define EXEC_OBJECT_NEEDS_GTT  (1<<1)
>>   #define EXEC_OBJECT_WRITE      (1<<2)
>> -#define __EXEC_OBJECT_UNKNOWN_FLAGS -(EXEC_OBJECT_WRITE<<1)
>> +#define EXEC_OBJECT_SUPPORTS_48B_ADDRESS (1<<3)
>> +#define __EXEC_OBJECT_UNKNOWN_FLAGS -(EXEC_OBJECT_SUPPORTS_48B_ADDRESS<<1)
> Perhaps you already know this but changes like these go in _after_ the
> updated kernel header is part of linux-next or a released kernel
> version.
>

The kernel changes have just been reviewed in drm-intel. I included the 
userland patches (there's a mesa patch too) so more people could see the 
whole picture.

>>          __u64 flags;
>>
>>          __u64 rsvd1;
>> diff --git a/intel/intel_bufmgr.c b/intel/intel_bufmgr.c
>> index 14ea9f9..590a855 100644
>> --- a/intel/intel_bufmgr.c
>> +++ b/intel/intel_bufmgr.c
>> @@ -188,6 +188,18 @@ drm_intel_bufmgr_check_aperture_space(drm_intel_bo ** bo_array, int count)
>>          return bo_array[0]->bufmgr->check_aperture_space(bo_array, count);
>>   }
>>
>> +void drm_intel_bo_set_supports_48b_address(drm_intel_bo *bo)
>> +{
>> +       if (bo->bufmgr->bo_set_supports_48b_address)
>> +               bo->bufmgr->bo_set_supports_48b_address(bo);
>> +}
>> +
>> +void drm_intel_bo_clear_supports_48b_address(drm_intel_bo *bo)
>> +{
>> +       if (bo->bufmgr->bo_clear_supports_48b_address)
>> +               bo->bufmgr->bo_clear_supports_48b_address(bo);
>> +}
>> +
>>   int
>>   drm_intel_bo_flink(drm_intel_bo *bo, uint32_t * name)
>>   {
>> @@ -202,6 +214,18 @@ drm_intel_bo_emit_reloc(drm_intel_bo *bo, uint32_t offset,
>>                          drm_intel_bo *target_bo, uint32_t target_offset,
>>                          uint32_t read_domains, uint32_t write_domain)
>>   {
>> +       drm_intel_bo_clear_supports_48b_address(target_bo);
>> +       return bo->bufmgr->bo_emit_reloc(bo, offset,
>> +                                        target_bo, target_offset,
>> +                                        read_domains, write_domain);
>> +}
>> +
>> +int
>> +drm_intel_bo_emit_reloc_48bit(drm_intel_bo *bo, uint32_t offset,
>> +                       drm_intel_bo *target_bo, uint32_t target_offset,
>> +                       uint32_t read_domains, uint32_t write_domain)
>> +{
>> +       drm_intel_bo_set_supports_48b_address(target_bo);
>>          return bo->bufmgr->bo_emit_reloc(bo, offset,
>>                                           target_bo, target_offset,
>>                                           read_domains, write_domain);
>> diff --git a/intel/intel_bufmgr.h b/intel/intel_bufmgr.h
>> index 285919e..62480cb 100644
>> --- a/intel/intel_bufmgr.h
>> +++ b/intel/intel_bufmgr.h
>> @@ -87,7 +87,8 @@ struct _drm_intel_bo {
>>          /**
>>           * Last seen card virtual address (offset from the beginning of the
>>           * aperture) for the object.  This should be used to fill relocation
>> -        * entries when calling drm_intel_bo_emit_reloc()
>> +        * entries when calling drm_intel_bo_emit_reloc() or
>> +        * drm_intel_bo_emit_reloc_48bit()
>>           */
>>          uint64_t offset64;
>>   };
>> @@ -137,6 +138,8 @@ void drm_intel_bo_wait_rendering(drm_intel_bo *bo);
>>
>>   void drm_intel_bufmgr_set_debug(drm_intel_bufmgr *bufmgr, int enable_debug);
>>   void drm_intel_bufmgr_destroy(drm_intel_bufmgr *bufmgr);
>> +void drm_intel_bo_set_supports_48b_address(drm_intel_bo *bo);
>> +void drm_intel_bo_clear_supports_48b_address(drm_intel_bo *bo);
> Are these two are internal/implementation specific functions ? If so
> please don't include them in this public header and annotate both
> declaration and definition with drm_private.
>
>>   int drm_intel_bo_exec(drm_intel_bo *bo, int used,
>>                        struct drm_clip_rect *cliprects, int num_cliprects, int DR4);
>>   int drm_intel_bo_mrb_exec(drm_intel_bo *bo, int used,
>> @@ -147,6 +150,9 @@ int drm_intel_bufmgr_check_aperture_space(drm_intel_bo ** bo_array, int count);
>>   int drm_intel_bo_emit_reloc(drm_intel_bo *bo, uint32_t offset,
>>                              drm_intel_bo *target_bo, uint32_t target_offset,
>>                              uint32_t read_domains, uint32_t write_domain);
>> +int drm_intel_bo_emit_reloc_48bit(drm_intel_bo *bo, uint32_t offset,
>> +                                 drm_intel_bo *target_bo, uint32_t target_offset,
>> +                                 uint32_t read_domains, uint32_t write_domain);
> Please add new (public) symbols to the test (intel-symbol-check). If
> in doubt make check will give you a lovely message.
>
> Patches that bump the version number are not normally posted to the
> ML, but are committed as part of the release process (see RELEASING
> for more info).

I'll update the intel-symbol-check and follow the release process after 
the kernel patch is merged.

Just to confirm, anyone can push to git://anongit.freedesktop.org/mesa/drm ?

Thanks for the tips,

-Michel
>
> Cheers,
> Emil
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx