| Message ID | 1e8f9768f2d638dfa1fc72f80f0d7391c4a48bbb.1726235647.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: clean up deps on intel_display.h a bit | expand |
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 ? > @@ -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 >
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. 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 >>
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? But right, right now it looks like this place is better indeed. Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > 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
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
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 @@ -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"
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(-)