| Message ID | 0b9459da6c8cba0f74bf2781d69182fa6801cd97.1709727127.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: better high level abstraction for display | expand |
On Wed, Mar 06, 2024 at 02:24:36PM +0200, Jani Nikula wrote: > Convert various pointers to struct intel_display * using _Generic(). > > Add some macro magic to make adding new conversions easier, and somewhat > abstract the need to cast each generic association. The cast is required > because all associations needs to compile, regardless of the type and > the generic selection. > > The use of *p in the generic selection assignment expression removes the > need to add separate associations for const pointers. > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > .../drm/i915/display/intel_display_types.h | 46 +++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > index e67cd5b02e84..3f63a5a74d77 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -2183,4 +2183,50 @@ static inline int to_bpp_x16(int bpp) > return bpp << 4; > } > > +/* > + * Conversion functions/macros from various pointer types to struct > + * intel_display pointer. > + */ > +static inline struct intel_display *__drm_device_to_intel_display(const struct drm_device *drm) > +{ > + /* > + * Assume there's a pointer to struct intel_display in memory right > + * after struct drm_device. > + */ > + struct intel_display **p = (struct intel_display **)(drm + 1); at least this patch and the first one should be together to help the 'magic' to be understood and confirmed safe. > + > + return *p; > +} > + > +#define __intel_connector_to_intel_display(p) \ > + __drm_device_to_intel_display((p)->base.dev) > +#define __intel_crtc_to_intel_display(p) \ > + __drm_device_to_intel_display((p)->base.dev) > +#define __intel_crtc_state_to_intel_display(p) \ > + __drm_device_to_intel_display((p)->uapi.crtc->dev) > +#define __intel_digital_port_to_intel_display(p) \ > + __drm_device_to_intel_display((p)->base.base.dev) > +#define __intel_encoder_to_intel_display(p) \ > + __drm_device_to_intel_display((p)->base.dev) > +#define __intel_hdmi_to_intel_display(p) \ > + __drm_device_to_intel_display(hdmi_to_dig_port(p)->base.base.dev) > +#define __intel_dp_to_intel_display(p) \ > + __drm_device_to_intel_display(dp_to_dig_port(p)->base.base.dev) > + > +/* Helper for generic association. Map types to conversion functions/macros. */ > +#define __assoc(type, p) \ > + struct type: __##type##_to_intel_display((struct type *)(p)) > + > +/* Convert various pointer types to struct intel_display pointer. */ > +#define to_intel_display(p) \ For a moment I almost complained of this because of the generic magic, but mostly because I had guc related code in mind where you can never find the definition, but here it is different. the used 'to_intel_display' can easily be searched by cscope/ctags and then you are able to see below what are the accepted cases, so I ended up liking it. > + _Generic(*p, \ > + __assoc(intel_connector, p), \ > + __assoc(intel_crtc, p), \ > + __assoc(intel_crtc_state, p), \ > + __assoc(intel_digital_port, p), \ > + __assoc(intel_encoder, p), \ > + __assoc(intel_hdmi, p), \ > + __assoc(intel_dp, p), \ > + __assoc(drm_device, p)) > + > #endif /* __INTEL_DISPLAY_TYPES_H__ */ > -- > 2.39.2 >
On Wed, 06 Mar 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > On Wed, Mar 06, 2024 at 02:24:36PM +0200, Jani Nikula wrote: >> Convert various pointers to struct intel_display * using _Generic(). >> >> Add some macro magic to make adding new conversions easier, and somewhat >> abstract the need to cast each generic association. The cast is required >> because all associations needs to compile, regardless of the type and >> the generic selection. >> >> The use of *p in the generic selection assignment expression removes the >> need to add separate associations for const pointers. >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> .../drm/i915/display/intel_display_types.h | 46 +++++++++++++++++++ >> 1 file changed, 46 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h >> index e67cd5b02e84..3f63a5a74d77 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_types.h >> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h >> @@ -2183,4 +2183,50 @@ static inline int to_bpp_x16(int bpp) >> return bpp << 4; >> } >> >> +/* >> + * Conversion functions/macros from various pointer types to struct >> + * intel_display pointer. >> + */ >> +static inline struct intel_display *__drm_device_to_intel_display(const struct drm_device *drm) >> +{ >> + /* >> + * Assume there's a pointer to struct intel_display in memory right >> + * after struct drm_device. >> + */ >> + struct intel_display **p = (struct intel_display **)(drm + 1); > > at least this patch and the first one should be together to help the > 'magic' to be understood and confirmed safe. Yes. I just kept them separate while still juggling the whole thing. And it occurs to me we could make *this* the first patch in the series, by making the above function: static inline struct intel_display *__drm_device_to_intel_display(const struct drm_device *drm) { return &to_i915(drm)->display; } Then we could only add the struct drm_device *drm backpointer in struct intel_display from patch 1, and proceed with patches 3-4, avoiding the whole magic thing for starters. It would unblock a whole lot of refactoring as the first step. > >> + >> + return *p; >> +} >> + >> +#define __intel_connector_to_intel_display(p) \ >> + __drm_device_to_intel_display((p)->base.dev) >> +#define __intel_crtc_to_intel_display(p) \ >> + __drm_device_to_intel_display((p)->base.dev) >> +#define __intel_crtc_state_to_intel_display(p) \ >> + __drm_device_to_intel_display((p)->uapi.crtc->dev) >> +#define __intel_digital_port_to_intel_display(p) \ >> + __drm_device_to_intel_display((p)->base.base.dev) >> +#define __intel_encoder_to_intel_display(p) \ >> + __drm_device_to_intel_display((p)->base.dev) >> +#define __intel_hdmi_to_intel_display(p) \ >> + __drm_device_to_intel_display(hdmi_to_dig_port(p)->base.base.dev) >> +#define __intel_dp_to_intel_display(p) \ >> + __drm_device_to_intel_display(dp_to_dig_port(p)->base.base.dev) >> + >> +/* Helper for generic association. Map types to conversion functions/macros. */ >> +#define __assoc(type, p) \ >> + struct type: __##type##_to_intel_display((struct type *)(p)) >> + >> +/* Convert various pointer types to struct intel_display pointer. */ >> +#define to_intel_display(p) \ > > For a moment I almost complained of this because of the generic magic, > but mostly because I had guc related code in mind where you can never > find the definition, but here it is different. the used 'to_intel_display' > can easily be searched by cscope/ctags and then you are able to see > below what are the accepted cases, so I ended up liking it. Yay! I also tried to put effort into making this easily extensible. Add a __<FROM-STRUCT>_to_intel_display(p) macro or function, and __assoc(<FROM-STRUCT>, p) in the association list below, and it just works. BR, Jani. > >> + _Generic(*p, \ >> + __assoc(intel_connector, p), \ >> + __assoc(intel_crtc, p), \ >> + __assoc(intel_crtc_state, p), \ >> + __assoc(intel_digital_port, p), \ >> + __assoc(intel_encoder, p), \ >> + __assoc(intel_hdmi, p), \ >> + __assoc(intel_dp, p), \ >> + __assoc(drm_device, p)) >> + >> #endif /* __INTEL_DISPLAY_TYPES_H__ */ >> -- >> 2.39.2 >>
On Thu, Mar 07, 2024 at 01:28:57PM +0200, Jani Nikula wrote: > On Wed, 06 Mar 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > > On Wed, Mar 06, 2024 at 02:24:36PM +0200, Jani Nikula wrote: > >> Convert various pointers to struct intel_display * using _Generic(). > >> > >> Add some macro magic to make adding new conversions easier, and somewhat > >> abstract the need to cast each generic association. The cast is required > >> because all associations needs to compile, regardless of the type and > >> the generic selection. > >> > >> The use of *p in the generic selection assignment expression removes the > >> need to add separate associations for const pointers. > >> > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > >> --- > >> .../drm/i915/display/intel_display_types.h | 46 +++++++++++++++++++ > >> 1 file changed, 46 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > >> index e67cd5b02e84..3f63a5a74d77 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_display_types.h > >> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > >> @@ -2183,4 +2183,50 @@ static inline int to_bpp_x16(int bpp) > >> return bpp << 4; > >> } > >> > >> +/* > >> + * Conversion functions/macros from various pointer types to struct > >> + * intel_display pointer. > >> + */ > >> +static inline struct intel_display *__drm_device_to_intel_display(const struct drm_device *drm) > >> +{ > >> + /* > >> + * Assume there's a pointer to struct intel_display in memory right > >> + * after struct drm_device. > >> + */ > >> + struct intel_display **p = (struct intel_display **)(drm + 1); > > > > at least this patch and the first one should be together to help the > > 'magic' to be understood and confirmed safe. > > Yes. I just kept them separate while still juggling the whole thing. > > And it occurs to me we could make *this* the first patch in the series, > by making the above function: > > static inline struct intel_display *__drm_device_to_intel_display(const struct drm_device *drm) > { > return &to_i915(drm)->display; > } > > Then we could only add the struct drm_device *drm backpointer in struct > intel_display from patch 1, and proceed with patches 3-4, avoiding the > whole magic thing for starters. It would unblock a whole lot of > refactoring as the first step. sounds like a good plan > > > > >> + > >> + return *p; > >> +} > >> + > >> +#define __intel_connector_to_intel_display(p) \ > >> + __drm_device_to_intel_display((p)->base.dev) > >> +#define __intel_crtc_to_intel_display(p) \ > >> + __drm_device_to_intel_display((p)->base.dev) > >> +#define __intel_crtc_state_to_intel_display(p) \ > >> + __drm_device_to_intel_display((p)->uapi.crtc->dev) > >> +#define __intel_digital_port_to_intel_display(p) \ > >> + __drm_device_to_intel_display((p)->base.base.dev) > >> +#define __intel_encoder_to_intel_display(p) \ > >> + __drm_device_to_intel_display((p)->base.dev) > >> +#define __intel_hdmi_to_intel_display(p) \ > >> + __drm_device_to_intel_display(hdmi_to_dig_port(p)->base.base.dev) > >> +#define __intel_dp_to_intel_display(p) \ > >> + __drm_device_to_intel_display(dp_to_dig_port(p)->base.base.dev) > >> + > >> +/* Helper for generic association. Map types to conversion functions/macros. */ > >> +#define __assoc(type, p) \ > >> + struct type: __##type##_to_intel_display((struct type *)(p)) > >> + > >> +/* Convert various pointer types to struct intel_display pointer. */ > >> +#define to_intel_display(p) \ > > > > For a moment I almost complained of this because of the generic magic, > > but mostly because I had guc related code in mind where you can never > > find the definition, but here it is different. the used 'to_intel_display' > > can easily be searched by cscope/ctags and then you are able to see > > below what are the accepted cases, so I ended up liking it. > > Yay! > > I also tried to put effort into making this easily extensible. Add a > __<FROM-STRUCT>_to_intel_display(p) macro or function, and > __assoc(<FROM-STRUCT>, p) in the association list below, and it just > works. as long as we don't lose the ability to use cscope/ctags to find these definitions I would be okay. right now a cscope on intel_crtc would show the _assoc(intel_crtc... but if we lose that we kind of go to the dark side of the macro indirections. > > BR, > Jani. > > > > >> + _Generic(*p, \ > >> + __assoc(intel_connector, p), \ > >> + __assoc(intel_crtc, p), \ > >> + __assoc(intel_crtc_state, p), \ > >> + __assoc(intel_digital_port, p), \ > >> + __assoc(intel_encoder, p), \ > >> + __assoc(intel_hdmi, p), \ > >> + __assoc(intel_dp, p), \ > >> + __assoc(drm_device, p)) > >> + > >> #endif /* __INTEL_DISPLAY_TYPES_H__ */ > >> -- > >> 2.39.2 > >> > > -- > Jani Nikula, Intel
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index e67cd5b02e84..3f63a5a74d77 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -2183,4 +2183,50 @@ static inline int to_bpp_x16(int bpp) return bpp << 4; } +/* + * Conversion functions/macros from various pointer types to struct + * intel_display pointer. + */ +static inline struct intel_display *__drm_device_to_intel_display(const struct drm_device *drm) +{ + /* + * Assume there's a pointer to struct intel_display in memory right + * after struct drm_device. + */ + struct intel_display **p = (struct intel_display **)(drm + 1); + + return *p; +} + +#define __intel_connector_to_intel_display(p) \ + __drm_device_to_intel_display((p)->base.dev) +#define __intel_crtc_to_intel_display(p) \ + __drm_device_to_intel_display((p)->base.dev) +#define __intel_crtc_state_to_intel_display(p) \ + __drm_device_to_intel_display((p)->uapi.crtc->dev) +#define __intel_digital_port_to_intel_display(p) \ + __drm_device_to_intel_display((p)->base.base.dev) +#define __intel_encoder_to_intel_display(p) \ + __drm_device_to_intel_display((p)->base.dev) +#define __intel_hdmi_to_intel_display(p) \ + __drm_device_to_intel_display(hdmi_to_dig_port(p)->base.base.dev) +#define __intel_dp_to_intel_display(p) \ + __drm_device_to_intel_display(dp_to_dig_port(p)->base.base.dev) + +/* Helper for generic association. Map types to conversion functions/macros. */ +#define __assoc(type, p) \ + struct type: __##type##_to_intel_display((struct type *)(p)) + +/* Convert various pointer types to struct intel_display pointer. */ +#define to_intel_display(p) \ + _Generic(*p, \ + __assoc(intel_connector, p), \ + __assoc(intel_crtc, p), \ + __assoc(intel_crtc_state, p), \ + __assoc(intel_digital_port, p), \ + __assoc(intel_encoder, p), \ + __assoc(intel_hdmi, p), \ + __assoc(intel_dp, p), \ + __assoc(drm_device, p)) + #endif /* __INTEL_DISPLAY_TYPES_H__ */
Convert various pointers to struct intel_display * using _Generic(). Add some macro magic to make adding new conversions easier, and somewhat abstract the need to cast each generic association. The cast is required because all associations needs to compile, regardless of the type and the generic selection. The use of *p in the generic selection assignment expression removes the need to add separate associations for const pointers. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- .../drm/i915/display/intel_display_types.h | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+)