Re: [Intel-gfx] [PATCH] drm/i915: Remove unused for_each_uabi_class_engine

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 01 Nov 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 01/11/2023 10:06, Jani Nikula wrote:
>> On Wed, 01 Nov 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Unused macro after 99919be74aa3 ("drm/i915/gem: Zap the i915_gem_object_blt code")
>>> removed some code.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> 
>> \o/
>> 
>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>> 
>> Could I persuade you to move for_each_engine(),
>> for_each_engine_masked(), rb_to_uabi_engine(), and
>> for_each_uabi_engine() to a more suitable header?
>
> Former to intel_gt.h, but latter a better place is not coming to me. Like:
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.h b/drivers/gpu/drm/i915/gt/intel_engine_pm.h
> index d68675925b79..1d97c435a015 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.h
> @@ -10,6 +10,7 @@
>   #include "i915_request.h"
>   #include "intel_engine_types.h"
>   #include "intel_wakeref.h"
> +#include "intel_gt.h"
>   #include "intel_gt_pm.h"
>   
>   static inline bool
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h
> index 9ffdb05e231e..b0e453e27ea8 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.h
> @@ -171,6 +171,20 @@ void intel_gt_release_all(struct drm_i915_private *i915);
>               (id__)++) \
>                  for_each_if(((gt__) = (i915__)->gt[(id__)]))
>   
> +/* Simple iterator over all initialised engines */
> +#define for_each_engine(engine__, gt__, id__) \
> +       for ((id__) = 0; \
> +            (id__) < I915_NUM_ENGINES; \
> +            (id__)++) \
> +               for_each_if ((engine__) = (gt__)->engine[(id__)])
> +
> +/* Iterator over subset of engines selected by mask */
> +#define for_each_engine_masked(engine__, gt__, mask__, tmp__) \
> +       for ((tmp__) = (mask__) & (gt__)->info.engine_mask; \
> +            (tmp__) ? \
> +            ((engine__) = (gt__)->engine[__mask_next_bit(tmp__)]), 1 : \
> +            0;)
> +
>   void intel_gt_info_print(const struct intel_gt_info *info,
>                           struct drm_printer *p);
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_engines_debugfs.c b/drivers/gpu/drm/i915/gt/intel_gt_engines_debugfs.c
> index 8f9b874fdc9c..3aa1d014c14d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_engines_debugfs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_engines_debugfs.c
> @@ -6,8 +6,8 @@
>   
>   #include <drm/drm_print.h>
>   
> -#include "i915_drv.h" /* for_each_engine! */
>   #include "intel_engine.h"
> +#include "intel_gt.h"
>   #include "intel_gt_debugfs.h"
>   #include "intel_gt_engines_debugfs.h"
>   
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 744c8c4a50fa..3feec04a2b1c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -396,20 +396,6 @@ static inline struct intel_gt *to_gt(const struct drm_i915_private *i915)
>          return i915->gt[0];
>   }
>   
> -/* Simple iterator over all initialised engines */
> -#define for_each_engine(engine__, gt__, id__) \
> -       for ((id__) = 0; \
> -            (id__) < I915_NUM_ENGINES; \
> -            (id__)++) \
> -               for_each_if ((engine__) = (gt__)->engine[(id__)])
> -
> -/* Iterator over subset of engines selected by mask */
> -#define for_each_engine_masked(engine__, gt__, mask__, tmp__) \
> -       for ((tmp__) = (mask__) & (gt__)->info.engine_mask; \
> -            (tmp__) ? \
> -            ((engine__) = (gt__)->engine[__mask_next_bit(tmp__)]), 1 : \
> -            0;)
> -
>   #define rb_to_uabi_engine(rb) \
>          rb_entry_safe(rb, struct intel_engine_cs, uabi_node)
>   
> diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c
> index 7a5f4adc1b8b..c998f15d505c 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_uncore.c
> @@ -24,6 +24,8 @@
>   
>   #include "../i915_selftest.h"
>   
> +#include "gt/intel_gt.h"
> +
>   static int intel_fw_table_check(const struct intel_forcewake_range *ranges,
>                                  unsigned int num_ranges,
>                                  bool is_watertight)
>
> Beneficial?

Yeah, I'd like to have less gem/gt/display in i915_drv.h, and focus on
the generic driver stuff.

BR,
Jani.


>
> Regards,
>
> Tvrtko
>   
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.h | 5 -----
>>>   1 file changed, 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index bfcbe93bd9fe..744c8c4a50fa 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -418,11 +418,6 @@ static inline struct intel_gt *to_gt(const struct drm_i915_private *i915)
>>>   	     (engine__); \
>>>   	     (engine__) = rb_to_uabi_engine(rb_next(&(engine__)->uabi_node)))
>>>   
>>> -#define for_each_uabi_class_engine(engine__, class__, i915__) \
>>> -	for ((engine__) = intel_engine_lookup_user((i915__), (class__), 0); \
>>> -	     (engine__) && (engine__)->uabi_class == (class__); \
>>> -	     (engine__) = rb_to_uabi_engine(rb_next(&(engine__)->uabi_node)))
>>> -
>>>   #define INTEL_INFO(i915)	((i915)->__info)
>>>   #define RUNTIME_INFO(i915)	(&(i915)->__runtime)
>>>   #define DRIVER_CAPS(i915)	(&(i915)->caps)
>> 

-- 
Jani Nikula, Intel