Re: [PATCH 2/2] drm/i915/display: move enum i9xx_plane_id to intel_display_limits.h

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

On Mon, 16 Sep 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On Sat, Sep 14, 2024 at 12:32:13AM +0300, Jani Nikula wrote:
>> On Fri, 13 Sep 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>> > On Fri, Sep 13, 2024 at 04:54:39PM +0300, Jani Nikula wrote:
>> >> Move enum i9xx_plane_id from intel_display.h to intel_display_limits.h
>> >> to be able to reduce dependencies on intel_display.h.
>> >> 
>> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/display/intel_display.h        | 10 ----------
>> >>  drivers/gpu/drm/i915/display/intel_display_limits.h | 10 ++++++++++
>> >>  drivers/gpu/drm/i915/gvt/cmd_parser.c               |  1 -
>> >>  3 files changed, 10 insertions(+), 11 deletions(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
>> >> index d10608526eee..4bdb48084cab 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_display.h
>> >> +++ b/drivers/gpu/drm/i915/display/intel_display.h
>> >> @@ -95,16 +95,6 @@ static inline bool transcoder_is_dsi(enum transcoder transcoder)
>> >>  	return transcoder == TRANSCODER_DSI_A || transcoder == TRANSCODER_DSI_C;
>> >>  }
>> >>  
>> >> -/*
>> >> - * Global legacy plane identifier. Valid only for primary/sprite
>> >> - * planes on pre-g4x, and only for primary planes on g4x-bdw.
>> >> - */
>> >> -enum i9xx_plane_id {
>> >> -	PLANE_A,
>> >> -	PLANE_B,
>> >> -	PLANE_C,
>> >> -};
>> >> -
>> >>  #define plane_name(p) ((p) + 'A')
>> >>  
>> >>  #define for_each_plane_id_on_crtc(__crtc, __p) \
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_limits.h b/drivers/gpu/drm/i915/display/intel_display_limits.h
>> >> index c4775c99dc83..f0fa27e365ab 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_display_limits.h
>> >> +++ b/drivers/gpu/drm/i915/display/intel_display_limits.h
>> >
>> > why here and not in somewhere like:
>> > drivers/gpu/drm/i915/display/i9xx_plane.h
>> > ?
>> 
>> Here it's next to enum plane_id. This entire file exists to provide a
>> minimal header for the enums.
>> 
>> I'm not sure what the right thing is, but putting this to i9xx_plane.h
>> means including that file in more places, just for this thing, while
>> they have no use at all for the interfaces provided by i9xx_plane.h.
>
> hmm perhaps we could create a small one for plane_types like Xe style?

I'm actually not fond of the xe (or i915 gem) style for headers. In many
places you have APIs in foo.h, which goes on to include foo_types.h,
even though forward declarations would suffice. foo_types.h goes on to
include bar_types.h, and so on, and so on. And everything needs
everything.

Like, why does a change in xe/guc_klvs_abi.h cause the rebuild of
everything, including display?

Sometimes you do have static inlines in the API headers, which actually
look at the guts of the implementation and do need the _types.h. I
basically think static inlines should be be restricted to things that a)
don't depend on additional headers, and b) actually have a performance
justification for not being a regular function.

I'm not saying i915 display is a shining example either, but I don't
want to start moving it to a direction that I don't think actually
reduces header dependencies. Whatever the direction, it needs to reduce
header dependencies.

</rant>

> But right, right now it looks like this place is better indeed.
>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Thanks, pushed to drm-intel-next.

BR,
Jani.

>
>> 
>> BR,
>> Jani.
>> 
>> 
>> 
>> >
>> >> @@ -49,6 +49,16 @@ enum transcoder {
>> >>  	I915_MAX_TRANSCODERS
>> >>  };
>> >>  
>> >> +/*
>> >> + * Global legacy plane identifier. Valid only for primary/sprite
>> >> + * planes on pre-g4x, and only for primary planes on g4x-bdw.
>> >> + */
>> >> +enum i9xx_plane_id {
>> >> +	PLANE_A,
>> >> +	PLANE_B,
>> >> +	PLANE_C,
>> >> +};
>> >> +
>> >>  /*
>> >>   * Per-pipe plane identifier.
>> >>   * I915_MAX_PLANES in the enum below is the maximum (across all platforms)
>> >> diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
>> >> index 2f4c9c66b40b..81d67a46cd9e 100644
>> >> --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
>> >> +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
>> >> @@ -50,7 +50,6 @@
>> >>  #include "trace.h"
>> >>  
>> >>  #include "display/i9xx_plane_regs.h"
>> >> -#include "display/intel_display.h"
>> >>  #include "display/intel_sprite_regs.h"
>> >>  #include "gem/i915_gem_context.h"
>> >>  #include "gem/i915_gem_pm.h"
>> >> -- 
>> >> 2.39.2
>> >> 
>> 
>> -- 
>> Jani Nikula, Intel

-- 
Jani Nikula, Intel