Re: [PATCH] drm/i915: Remove unused for_each_uabi_class_engine

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 02/11/2023 09:24, Jani Nikula wrote:
> On Wed, 01 Nov 2023, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
> > On 01/11/2023 10:06, Jani Nikula wrote:
> > > On Wed, 01 Nov 2023, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
> > > > From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > > >
> > > > Unused macro after 99919be74aa3 ("drm/i915/gem: Zap the i915_gem_object_blt code")
> > > > removed some code.
> > > >
> > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > >
> > > \o/
> > >
> > > Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> > >
> > > 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.

Okay, sent.

For for_each_uabi_engine&co problem is how do we define what is what. 
Historically we weren't saying that everything not display is GEM, and 
uabi engines are not per GT. Even though engines themselves are, just 
that historically we were putting stuff into GT which operates on a GT. 
Perhaps with factoring out the display goal the requirements change a 
bit and old boundaries/placement rules need tweaking. Or the sore point 
will go away as/when display code is better isolated (less hacks, more 
interfaces) from both i915 and xe. Anyway, for now I don't see a nice 
and easy place to move them to, which wouldn't be wrong from some aspect.

Regards,

Tvrtko