[v3,1/9] drm/vc4: Switch to container_of_const

Commit Message

Maxime Ripard March 6, 2023, 10:46 a.m. UTC

container_of_const() allows to preserve the pointer constness and is
thus more flexible than inline functions.

Let's switch all our instances of container_of() to
container_of_const().

Signed-off-by: Maxime Ripard <[e-mail]>
---
 drivers/gpu/drm/vc4/tests/vc4_mock.h        |  3 ++
 drivers/gpu/drm/vc4/tests/vc4_mock_output.c |  4 +-
 drivers/gpu/drm/vc4/vc4_dpi.c               |  7 +---
 drivers/gpu/drm/vc4/vc4_drv.h               | 65 +++++++++--------------------
 drivers/gpu/drm/vc4/vc4_dsi.c               | 17 +++-----
 drivers/gpu/drm/vc4/vc4_hdmi.h              | 16 +++----
 drivers/gpu/drm/vc4/vc4_kms.c               | 16 +++----
 drivers/gpu/drm/vc4/vc4_plane.c             |  3 +-
 drivers/gpu/drm/vc4/vc4_txp.c               | 12 ++----
 drivers/gpu/drm/vc4/vc4_vec.c               | 14 ++-----
 10 files changed, 51 insertions(+), 106 deletions(-)

Comments

Javier Martinez Canillas April 22, 2023, 5:26 a.m. UTC | #1

Maxime Ripard <[e-mail]> writes:

Hello Maxime,

> container_of_const() allows to preserve the pointer constness and is
> thus more flexible than inline functions.
>
> Let's switch all our instances of container_of() to
> container_of_const().
>
> Signed-off-by: Maxime Ripard <[e-mail]>
> ---

[...]

> -static inline struct vc4_dpi *
> -to_vc4_dpi(struct drm_encoder *encoder)
> -{
> -	return container_of(encoder, struct vc4_dpi, encoder.base);
> -}
> +#define to_vc4_dpi(_encoder)						\
> +	container_of_const(_encoder, struct vc4_dpi, encoder.base)
>

A disadvantage of this approach though is that the type checking is lost.
Since you already had these, I would probably had changed them to return
a const pointer and just replace container_of() for container_of_const().

But I see that there are a lot of patches from Greg all over the kernel
that do exactly this, dropping static inline functions in favor of using
container_of_const() directly. So it seems the convention is what you do.

Reviewed-by: Javier Martinez Canillas <[e-mail]>

Maxime Ripard April 25, 2023, 7:38 a.m. UTC | #2

Hi Javier,

On Sat, Apr 22, 2023 at 07:26:13AM +0200, Javier Martinez Canillas wrote:
> Maxime Ripard <[e-mail]> writes:
> > container_of_const() allows to preserve the pointer constness and is
> > thus more flexible than inline functions.
> >
> > Let's switch all our instances of container_of() to
> > container_of_const().
> >
> > Signed-off-by: Maxime Ripard <[e-mail]>
> > ---
> 
> [...]
> 
> > -static inline struct vc4_dpi *
> > -to_vc4_dpi(struct drm_encoder *encoder)
> > -{
> > -	return container_of(encoder, struct vc4_dpi, encoder.base);
> > -}
> > +#define to_vc4_dpi(_encoder)						\
> > +	container_of_const(_encoder, struct vc4_dpi, encoder.base)
> >
> 
> A disadvantage of this approach though is that the type checking is lost.

Not entirely, the argument is still type-checked, but yeah, it's true
for the returned value.

> Since you already had these, I would probably had changed them to return
> a const pointer and just replace container_of() for container_of_const().
> 
> But I see that there are a lot of patches from Greg all over the kernel
> that do exactly this, dropping static inline functions in favor of using
> container_of_const() directly. So it seems the convention is what you do.

More importantly, container_of_const() isn't always returning a const
pointer or always taking a const argument, it's returning the pointer
with the same const-ness than the argument.

This is why it makes sense to remove the inline function entirely,
because it removes the main benefit it brings.

> Reviewed-by: Javier Martinez Canillas <[e-mail]>

Thanks, I've applied this series

Maxime

Javier Martinez Canillas April 25, 2023, 8:21 a.m. UTC | #3

Maxime Ripard <[e-mail]> writes:

> Hi Javier,
>
> On Sat, Apr 22, 2023 at 07:26:13AM +0200, Javier Martinez Canillas wrote:
>> Maxime Ripard <[e-mail]> writes:
>> > container_of_const() allows to preserve the pointer constness and is
>> > thus more flexible than inline functions.
>> >
>> > Let's switch all our instances of container_of() to
>> > container_of_const().
>> >
>> > Signed-off-by: Maxime Ripard <[e-mail]>
>> > ---
>> 
>> [...]
>> 
>> > -static inline struct vc4_dpi *
>> > -to_vc4_dpi(struct drm_encoder *encoder)
>> > -{
>> > -	return container_of(encoder, struct vc4_dpi, encoder.base);
>> > -}
>> > +#define to_vc4_dpi(_encoder)						\
>> > +	container_of_const(_encoder, struct vc4_dpi, encoder.base)
>> >
>> 
>> A disadvantage of this approach though is that the type checking is lost.
>
> Not entirely, the argument is still type-checked, but yeah, it's true
> for the returned value.
>
>> Since you already had these, I would probably had changed them to return
>> a const pointer and just replace container_of() for container_of_const().
>> 
>> But I see that there are a lot of patches from Greg all over the kernel
>> that do exactly this, dropping static inline functions in favor of using
>> container_of_const() directly. So it seems the convention is what you do.
>
> More importantly, container_of_const() isn't always returning a const
> pointer or always taking a const argument, it's returning the pointer
> with the same const-ness than the argument.
>
> This is why it makes sense to remove the inline function entirely,
> because it removes the main benefit it brings.
>

Got it. Thanks for the explanations.

Patch

diff --git a/drivers/gpu/drm/vc4/tests/vc4_mock.h b/drivers/gpu/drm/vc4/tests/vc4_mock.h
index db8e9a141ef8..2d0b339bd9f3 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_mock.h
+++ b/drivers/gpu/drm/vc4/tests/vc4_mock.h
@@ -43,6 +43,9 @@  struct vc4_dummy_output {
 	struct drm_connector connector;
 };
 
+#define encoder_to_vc4_dummy_output(_enc)				\
+	container_of_const(_enc, struct vc4_dummy_output, encoder.base)
+
 struct vc4_dummy_output *vc4_dummy_output(struct kunit *test,
 					  struct drm_device *drm,
 					  struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/vc4/tests/vc4_mock_output.c b/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
index 8d33be828d9a..6e11fcc9ef45 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
@@ -80,7 +80,7 @@  int vc4_mock_atomic_add_output(struct kunit *test,
 	crtc = vc4_find_crtc_for_encoder(test, drm, encoder);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc);
 
-	output = container_of(encoder, struct vc4_dummy_output, encoder.base);
+	output = encoder_to_vc4_dummy_output(encoder);
 	conn = &output->connector;
 	conn_state = drm_atomic_get_connector_state(state, conn);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);
@@ -126,7 +126,7 @@  int vc4_mock_atomic_del_output(struct kunit *test,
 	ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
-	output = container_of(encoder, struct vc4_dummy_output, encoder.base);
+	output = encoder_to_vc4_dummy_output(encoder);
 	conn = &output->connector;
 	conn_state = drm_atomic_get_connector_state(state, conn);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);
diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c
index f518d6e59ed6..e68c07d86040 100644
--- a/drivers/gpu/drm/vc4/vc4_dpi.c
+++ b/drivers/gpu/drm/vc4/vc4_dpi.c
@@ -97,11 +97,8 @@  struct vc4_dpi {
 	struct debugfs_regset32 regset;
 };
 
-static inline struct vc4_dpi *
-to_vc4_dpi(struct drm_encoder *encoder)
-{
-	return container_of(encoder, struct vc4_dpi, encoder.base);
-}
+#define to_vc4_dpi(_encoder)						\
+	container_of_const(_encoder, struct vc4_dpi, encoder.base)
 
 #define DPI_READ(offset)								\
 	({										\
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 95069bb16821..e23084f3d6c2 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -232,11 +232,8 @@  struct vc4_dev {
 	struct kref bin_bo_kref;
 };
 
-static inline struct vc4_dev *
-to_vc4_dev(const struct drm_device *dev)
-{
-	return container_of(dev, struct vc4_dev, base);
-}
+#define to_vc4_dev(_dev)			\
+	container_of_const(_dev, struct vc4_dev, base)
 
 struct vc4_bo {
 	struct drm_gem_dma_object base;
@@ -285,11 +282,8 @@  struct vc4_bo {
 	struct mutex madv_lock;
 };
 
-static inline struct vc4_bo *
-to_vc4_bo(const struct drm_gem_object *bo)
-{
-	return container_of(to_drm_gem_dma_obj(bo), struct vc4_bo, base);
-}
+#define to_vc4_bo(_bo)							\
+	container_of_const(to_drm_gem_dma_obj(_bo), struct vc4_bo, base)
 
 struct vc4_fence {
 	struct dma_fence base;
@@ -298,11 +292,8 @@  struct vc4_fence {
 	uint64_t seqno;
 };
 
-static inline struct vc4_fence *
-to_vc4_fence(const struct dma_fence *fence)
-{
-	return container_of(fence, struct vc4_fence, base);
-}
+#define to_vc4_fence(_fence)					\
+	container_of_const(_fence, struct vc4_fence, base)
 
 struct vc4_seqno_cb {
 	struct work_struct work;
@@ -368,11 +359,8 @@  struct vc4_hvs_state {
 	} fifo_state[HVS_NUM_CHANNELS];
 };
 
-static inline struct vc4_hvs_state *
-to_vc4_hvs_state(const struct drm_private_state *priv)
-{
-	return container_of(priv, struct vc4_hvs_state, base);
-}
+#define to_vc4_hvs_state(_state)				\
+	container_of_const(_state, struct vc4_hvs_state, base)
 
 struct vc4_hvs_state *vc4_hvs_get_global_state(struct drm_atomic_state *state);
 struct vc4_hvs_state *vc4_hvs_get_old_global_state(const struct drm_atomic_state *state);
@@ -382,11 +370,8 @@  struct vc4_plane {
 	struct drm_plane base;
 };
 
-static inline struct vc4_plane *
-to_vc4_plane(const struct drm_plane *plane)
-{
-	return container_of(plane, struct vc4_plane, base);
-}
+#define to_vc4_plane(_plane)					\
+	container_of_const(_plane, struct vc4_plane, base)
 
 enum vc4_scaling_mode {
 	VC4_SCALING_NONE,
@@ -458,11 +443,8 @@  struct vc4_plane_state {
 	u64 membus_load;
 };
 
-static inline struct vc4_plane_state *
-to_vc4_plane_state(const struct drm_plane_state *state)
-{
-	return container_of(state, struct vc4_plane_state, base);
-}
+#define to_vc4_plane_state(_state)				\
+	container_of_const(_state, struct vc4_plane_state, base)
 
 enum vc4_encoder_type {
 	VC4_ENCODER_TYPE_NONE,
@@ -489,11 +471,8 @@  struct vc4_encoder {
 	void (*post_crtc_powerdown)(struct drm_encoder *encoder, struct drm_atomic_state *state);
 };
 
-static inline struct vc4_encoder *
-to_vc4_encoder(const struct drm_encoder *encoder)
-{
-	return container_of(encoder, struct vc4_encoder, base);
-}
+#define to_vc4_encoder(_encoder)				\
+	container_of_const(_encoder, struct vc4_encoder, base)
 
 static inline
 struct drm_encoder *vc4_find_encoder_by_type(struct drm_device *drm,
@@ -591,11 +570,8 @@  struct vc4_crtc {
 	unsigned int current_hvs_channel;
 };
 
-static inline struct vc4_crtc *
-to_vc4_crtc(const struct drm_crtc *crtc)
-{
-	return container_of(crtc, struct vc4_crtc, base);
-}
+#define to_vc4_crtc(_crtc)					\
+	container_of_const(_crtc, struct vc4_crtc, base)
 
 static inline const struct vc4_crtc_data *
 vc4_crtc_to_vc4_crtc_data(const struct vc4_crtc *crtc)
@@ -608,7 +584,7 @@  vc4_crtc_to_vc4_pv_data(const struct vc4_crtc *crtc)
 {
 	const struct vc4_crtc_data *data = vc4_crtc_to_vc4_crtc_data(crtc);
 
-	return container_of(data, struct vc4_pv_data, base);
+	return container_of_const(data, struct vc4_pv_data, base);
 }
 
 struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc,
@@ -636,11 +612,8 @@  struct vc4_crtc_state {
 
 #define VC4_HVS_CHANNEL_DISABLED ((unsigned int)-1)
 
-static inline struct vc4_crtc_state *
-to_vc4_crtc_state(const struct drm_crtc_state *crtc_state)
-{
-	return container_of(crtc_state, struct vc4_crtc_state, base);
-}
+#define to_vc4_crtc_state(_state)				\
+	container_of_const(_state, struct vc4_crtc_state, base)
 
 #define V3D_READ(offset)								\
 	({										\
diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index a5c075f802e4..9e0c355b236f 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -600,19 +600,14 @@  struct vc4_dsi {
 	struct debugfs_regset32 regset;
 };
 
-#define host_to_dsi(host) container_of(host, struct vc4_dsi, dsi_host)
+#define host_to_dsi(host)					\
+	container_of_const(host, struct vc4_dsi, dsi_host)
 
-static inline struct vc4_dsi *
-to_vc4_dsi(struct drm_encoder *encoder)
-{
-	return container_of(encoder, struct vc4_dsi, encoder.base);
-}
+#define to_vc4_dsi(_encoder)					\
+	container_of_const(_encoder, struct vc4_dsi, encoder.base)
 
-static inline struct vc4_dsi *
-bridge_to_vc4_dsi(struct drm_bridge *bridge)
-{
-	return container_of(bridge, struct vc4_dsi, bridge);
-}
+#define bridge_to_vc4_dsi(_bridge)				\
+	container_of_const(_bridge, struct vc4_dsi, bridge)
 
 static inline void
 dsi_dma_workaround_write(struct vc4_dsi *dsi, u32 offset, u32 val)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index dc3ccd8002a0..5d249ac54cd1 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -223,17 +223,14 @@  struct vc4_hdmi {
 	enum vc4_hdmi_output_format output_format;
 };
 
-static inline struct vc4_hdmi *
-connector_to_vc4_hdmi(struct drm_connector *connector)
-{
-	return container_of(connector, struct vc4_hdmi, connector);
-}
+#define connector_to_vc4_hdmi(_connector)				\
+	container_of_const(_connector, struct vc4_hdmi, connector)
 
 static inline struct vc4_hdmi *
 encoder_to_vc4_hdmi(struct drm_encoder *encoder)
 {
 	struct vc4_encoder *_encoder = to_vc4_encoder(encoder);
-	return container_of(_encoder, struct vc4_hdmi, encoder);
+	return container_of_const(_encoder, struct vc4_hdmi, encoder);
 }
 
 struct vc4_hdmi_connector_state {
@@ -243,11 +240,8 @@  struct vc4_hdmi_connector_state {
 	enum vc4_hdmi_output_format	output_format;
 };
 
-static inline struct vc4_hdmi_connector_state *
-conn_state_to_vc4_hdmi_conn_state(struct drm_connector_state *conn_state)
-{
-	return container_of(conn_state, struct vc4_hdmi_connector_state, base);
-}
+#define conn_state_to_vc4_hdmi_conn_state(_state)			\
+	container_of_const(_state, struct vc4_hdmi_connector_state, base)
 
 void vc4_hdmi_phy_init(struct vc4_hdmi *vc4_hdmi,
 		       struct vc4_hdmi_connector_state *vc4_conn_state);
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index a7e3d47c50f4..5495f2a94fa9 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -31,11 +31,8 @@  struct vc4_ctm_state {
 	int fifo;
 };
 
-static struct vc4_ctm_state *
-to_vc4_ctm_state(const struct drm_private_state *priv)
-{
-	return container_of(priv, struct vc4_ctm_state, base);
-}
+#define to_vc4_ctm_state(_state)				\
+	container_of_const(_state, struct vc4_ctm_state, base)
 
 struct vc4_load_tracker_state {
 	struct drm_private_state base;
@@ -43,11 +40,8 @@  struct vc4_load_tracker_state {
 	u64 membus_load;
 };
 
-static struct vc4_load_tracker_state *
-to_vc4_load_tracker_state(const struct drm_private_state *priv)
-{
-	return container_of(priv, struct vc4_load_tracker_state, base);
-}
+#define to_vc4_load_tracker_state(_state)				\
+	container_of_const(_state, struct vc4_load_tracker_state, base)
 
 static struct vc4_ctm_state *vc4_get_ctm_state(struct drm_atomic_state *state,
 					       struct drm_private_obj *manager)
@@ -717,7 +711,7 @@  static void vc4_hvs_channels_destroy_state(struct drm_private_obj *obj,
 static void vc4_hvs_channels_print_state(struct drm_printer *p,
 					 const struct drm_private_state *state)
 {
-	struct vc4_hvs_state *hvs_state = to_vc4_hvs_state(state);
+	const struct vc4_hvs_state *hvs_state = to_vc4_hvs_state(state);
 	unsigned int i;
 
 	drm_printf(p, "HVS State\n");
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 97c84a3f5a46..00e713faecd5 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -1334,8 +1334,7 @@  u32 vc4_plane_write_dlist(struct drm_plane *plane, u32 __iomem *dlist)
 
 u32 vc4_plane_dlist_size(const struct drm_plane_state *state)
 {
-	const struct vc4_plane_state *vc4_state =
-		container_of(state, typeof(*vc4_state), base);
+	const struct vc4_plane_state *vc4_state = to_vc4_plane_state(state);
 
 	return vc4_state->dlist_count;
 }
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index ef5cab2a3aa9..c5abdec03103 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -168,15 +168,11 @@  struct vc4_txp {
 	void __iomem *regs;
 };
 
-static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
-{
-	return container_of(encoder, struct vc4_txp, encoder.base);
-}
+#define encoder_to_vc4_txp(_encoder)					\
+	container_of_const(_encoder, struct vc4_txp, encoder.base)
 
-static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn)
-{
-	return container_of(conn, struct vc4_txp, connector.base);
-}
+#define connector_to_vc4_txp(_connector)				\
+	container_of_const(_connector, struct vc4_txp, connector.base)
 
 static const struct debugfs_reg32 txp_regs[] = {
 	VC4_REG32(TXP_DST_PTR),
diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
index a3782d05cd66..d6e6a1a22eba 100644
--- a/drivers/gpu/drm/vc4/vc4_vec.c
+++ b/drivers/gpu/drm/vc4/vc4_vec.c
@@ -219,17 +219,11 @@  struct vc4_vec {
 		writel(val, vec->regs + (offset));					\
 	} while (0)
 
-static inline struct vc4_vec *
-encoder_to_vc4_vec(struct drm_encoder *encoder)
-{
-	return container_of(encoder, struct vc4_vec, encoder.base);
-}
+#define encoder_to_vc4_vec(_encoder)					\
+	container_of_const(_encoder, struct vc4_vec, encoder.base)
 
-static inline struct vc4_vec *
-connector_to_vc4_vec(struct drm_connector *connector)
-{
-	return container_of(connector, struct vc4_vec, connector);
-}
+#define connector_to_vc4_vec(_connector)				\
+	container_of_const(_connector, struct vc4_vec, connector)
 
 enum vc4_vec_tv_mode_id {
 	VC4_VEC_TV_MODE_NTSC,