[PATCH] drm/mipi-dsi: Add a mipi_dual_dsi_dcs_write_seq() macro

[PATCH] drm/mipi-dsi: Add a mipi_dual_dsi_dcs_write_seq() macro

[Jianhua Lu]

The panels with two dsi connected (sync dual dsi mode) need to transmit
dcs command to the two dsi host simultaneously, let's add
mipi_dual_dsi_dcs_write_seq() macro for this kind of panels.

Signed-off-by: Jianhua Lu <lujianhua000@gmail.com>
---
 include/drm/drm_mipi_dsi.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index c9df0407980c..d0f0f75d4d83 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -336,6 +336,21 @@ int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi,
 		}                                                          \
 	} while (0)
 
+/**
+ * mipi_dsi_dcs_write_seq - transmit a DCS command with payload
+ * @dsi: array of 2 DSI peripheral devices
+ * @cmd: Command
+ * @seq: buffer containing data to be transmitted
+ */
+#define mipi_dual_dsi_dcs_write_seq(dsi, cmd, seq...)                   \
+	do {                                                             \
+		if (ARRAY_SIZE(dsi) > 2)                                 \
+			return -EINVAL;                                  \
+		int i;                                                   \
+		for (i = 0; i < ARRAY_SIZE(dsi); i++)                    \
+			mipi_dsi_dcs_write_seq(dsi[i], cmd, seq);        \
+	} while (0)
+
 /**
  * struct mipi_dsi_driver - DSI driver
  * @driver: device driver model driver
-- 
2.39.2

Re: [PATCH] drm/mipi-dsi: Add a mipi_dual_dsi_dcs_write_seq() macro

[Jani Nikula]

On Fri, 10 Mar 2023, Jianhua Lu <lujianhua000@gmail.com> wrote:
> The panels with two dsi connected (sync dual dsi mode) need to transmit
> dcs command to the two dsi host simultaneously, let's add
> mipi_dual_dsi_dcs_write_seq() macro for this kind of panels.

If we were to add a helper for this case, it should be a proper function
and not a macro like this.

We'd also need to see a user for this upstream.

>
> Signed-off-by: Jianhua Lu <lujianhua000@gmail.com>
> ---
>  include/drm/drm_mipi_dsi.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index c9df0407980c..d0f0f75d4d83 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -336,6 +336,21 @@ int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi,
>  		}                                                          \
>  	} while (0)
>  
> +/**
> + * mipi_dsi_dcs_write_seq - transmit a DCS command with payload
> + * @dsi: array of 2 DSI peripheral devices

This makes the assumption the devices are stored in an array. What if
drivers want to store them differently, for whatever reason? Maybe they
have an array of some container structs that have the devices? Maybe
they just have two struct mipi_dsi_device pointers?

> + * @cmd: Command
> + * @seq: buffer containing data to be transmitted
> + */
> +#define mipi_dual_dsi_dcs_write_seq(dsi, cmd, seq...)                   \
> +	do {                                                             \
> +		if (ARRAY_SIZE(dsi) > 2)                                 \
> +			return -EINVAL;                                  \
> +		int i;                                                   \

I believe this should lead to a warning for mixing code and
declarations.

> +		for (i = 0; i < ARRAY_SIZE(dsi); i++)                    \
> +			mipi_dsi_dcs_write_seq(dsi[i], cmd, seq);        \

This ignores errors.

> +	} while (0)
> +

Without an example user, I'm not yet convinced about the usefulness of
the helper, but I'd imagine something like this would be a more generic
approach, not enforcing the array, and handling errors properly:

ssize_t mipi_dsi_dual_dcs_write(struct mipi_dsi_device *dsi0,
                                struct mipi_dsi_device *dsi1,
                                u8 cmd, const void *data, size_t len)
{
	ssize_t err = 0;

	if (dsi0)
        	err = mipi_dsi_dcs_write(dsi0, cmd, data, len);

	if (dsi1 && !err)
        	err = mipi_dsi_dcs_write(dsi1, cmd, data, len);

	return err;
}

But even that begs the question where does it end? There are a lot of
mipi_dsi_dcs_*() functions as well as mipi_dsi_generic_write(). Dual
wrappers for all of them? :o


BR,
Jani.


>  /**
>   * struct mipi_dsi_driver - DSI driver
>   * @driver: device driver model driver

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH] drm/mipi-dsi: Add a mipi_dual_dsi_dcs_write_seq() macro

[Jianhua Lu]

On Fri, Mar 10, 2023 at 01:54:18PM +0200, Jani Nikula wrote:
> On Fri, 10 Mar 2023, Jianhua Lu <lujianhua000@gmail.com> wrote:
> > The panels with two dsi connected (sync dual dsi mode) need to transmit
> > dcs command to the two dsi host simultaneously, let's add
> > mipi_dual_dsi_dcs_write_seq() macro for this kind of panels.
> 
> If we were to add a helper for this case, it should be a proper function
> and not a macro like this.
> 
> We'd also need to see a user for this upstream.
> 
> >
> > Signed-off-by: Jianhua Lu <lujianhua000@gmail.com>
> > ---
> >  include/drm/drm_mipi_dsi.h | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> > index c9df0407980c..d0f0f75d4d83 100644
> > --- a/include/drm/drm_mipi_dsi.h
> > +++ b/include/drm/drm_mipi_dsi.h
> > @@ -336,6 +336,21 @@ int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi,
> >  		}                                                          \
> >  	} while (0)
> >  
> > +/**
> > + * mipi_dsi_dcs_write_seq - transmit a DCS command with payload
> > + * @dsi: array of 2 DSI peripheral devices
> 
> This makes the assumption the devices are stored in an array. What if
> drivers want to store them differently, for whatever reason? Maybe they
> have an array of some container structs that have the devices? Maybe
> they just have two struct mipi_dsi_device pointers?
This array just store two struct mipi_dsi_device pointers
> 
> > + * @cmd: Command
> > + * @seq: buffer containing data to be transmitted
> > + */
> > +#define mipi_dual_dsi_dcs_write_seq(dsi, cmd, seq...)                   \
> > +	do {                                                             \
> > +		if (ARRAY_SIZE(dsi) > 2)                                 \
> > +			return -EINVAL;                                  \
> > +		int i;                                                   \
> 
> I believe this should lead to a warning for mixing code and
> declarations.
> 
> > +		for (i = 0; i < ARRAY_SIZE(dsi); i++)                    \
> > +			mipi_dsi_dcs_write_seq(dsi[i], cmd, seq);        \
> 
> This ignores errors.
mipi_dsi_dcs_write_seq is also a macro, contains error checks in the body block.
> 
> > +	} while (0)
> > +
> 
> Without an example user, I'm not yet convinced about the usefulness of
> the helper, but I'd imagine something like this would be a more generic
> approach, not enforcing the array, and handling errors properly:
> 
> ssize_t mipi_dsi_dual_dcs_write(struct mipi_dsi_device *dsi0,
>                                 struct mipi_dsi_device *dsi1,
>                                 u8 cmd, const void *data, size_t len)
> {
> 	ssize_t err = 0;
> 
> 	if (dsi0)
>         	err = mipi_dsi_dcs_write(dsi0, cmd, data, len);
> 
> 	if (dsi1 && !err)
>         	err = mipi_dsi_dcs_write(dsi1, cmd, data, len);
> 
> 	return err;
> }
Thanks for your explanation and this looks more reasonable.
> 
> But even that begs the question where does it end? There are a lot of
> mipi_dsi_dcs_*() functions as well as mipi_dsi_generic_write(). Dual
> wrappers for all of them? :o
It's definitly useless to wrap all of them. Please ignore this patch.
> 
> 
> BR,
> Jani.
> 
> 
> >  /**
> >   * struct mipi_dsi_driver - DSI driver
> >   * @driver: device driver model driver
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH] drm/mipi-dsi: Add a mipi_dual_dsi_dcs_write_seq() macro

[Jani Nikula]

On Fri, 10 Mar 2023, Jianhua Lu <lujianhua000@gmail.com> wrote:
> On Fri, Mar 10, 2023 at 01:54:18PM +0200, Jani Nikula wrote:
>> On Fri, 10 Mar 2023, Jianhua Lu <lujianhua000@gmail.com> wrote:
>> > The panels with two dsi connected (sync dual dsi mode) need to transmit
>> > dcs command to the two dsi host simultaneously, let's add
>> > mipi_dual_dsi_dcs_write_seq() macro for this kind of panels.
>> 
>> If we were to add a helper for this case, it should be a proper function
>> and not a macro like this.
>> 
>> We'd also need to see a user for this upstream.
>> 
>> >
>> > Signed-off-by: Jianhua Lu <lujianhua000@gmail.com>
>> > ---
>> >  include/drm/drm_mipi_dsi.h | 15 +++++++++++++++
>> >  1 file changed, 15 insertions(+)
>> >
>> > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
>> > index c9df0407980c..d0f0f75d4d83 100644
>> > --- a/include/drm/drm_mipi_dsi.h
>> > +++ b/include/drm/drm_mipi_dsi.h
>> > @@ -336,6 +336,21 @@ int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi,
>> >  		}                                                          \
>> >  	} while (0)
>> >  
>> > +/**
>> > + * mipi_dsi_dcs_write_seq - transmit a DCS command with payload
>> > + * @dsi: array of 2 DSI peripheral devices
>> 
>> This makes the assumption the devices are stored in an array. What if
>> drivers want to store them differently, for whatever reason? Maybe they
>> have an array of some container structs that have the devices? Maybe
>> they just have two struct mipi_dsi_device pointers?
> This array just store two struct mipi_dsi_device pointers
>> 
>> > + * @cmd: Command
>> > + * @seq: buffer containing data to be transmitted
>> > + */
>> > +#define mipi_dual_dsi_dcs_write_seq(dsi, cmd, seq...)                   \
>> > +	do {                                                             \
>> > +		if (ARRAY_SIZE(dsi) > 2)                                 \
>> > +			return -EINVAL;                                  \
>> > +		int i;                                                   \
>> 
>> I believe this should lead to a warning for mixing code and
>> declarations.
>> 
>> > +		for (i = 0; i < ARRAY_SIZE(dsi); i++)                    \
>> > +			mipi_dsi_dcs_write_seq(dsi[i], cmd, seq);        \
>> 
>> This ignores errors.
> mipi_dsi_dcs_write_seq is also a macro, contains error checks in the body block.

Ugh, I think it's pretty scary to hide control flow like return
statements in macros like this.

IMO the the main user of mipi_dsi_dcs_write_seq()
i.e. panel_nv3051d_init_sequence() in
drivers/gpu/drm/panel/panel-newvision-nv3051d.c should be written to do
the writes from an array in a loop instead.

BR,
Jani.


>> 
>> > +	} while (0)
>> > +
>> 
>> Without an example user, I'm not yet convinced about the usefulness of
>> the helper, but I'd imagine something like this would be a more generic
>> approach, not enforcing the array, and handling errors properly:
>> 
>> ssize_t mipi_dsi_dual_dcs_write(struct mipi_dsi_device *dsi0,
>>                                 struct mipi_dsi_device *dsi1,
>>                                 u8 cmd, const void *data, size_t len)
>> {
>> 	ssize_t err = 0;
>> 
>> 	if (dsi0)
>>         	err = mipi_dsi_dcs_write(dsi0, cmd, data, len);
>> 
>> 	if (dsi1 && !err)
>>         	err = mipi_dsi_dcs_write(dsi1, cmd, data, len);
>> 
>> 	return err;
>> }
> Thanks for your explanation and this looks more reasonable.
>> 
>> But even that begs the question where does it end? There are a lot of
>> mipi_dsi_dcs_*() functions as well as mipi_dsi_generic_write(). Dual
>> wrappers for all of them? :o
> It's definitly useless to wrap all of them. Please ignore this patch.
>> 
>> 
>> BR,
>> Jani.
>> 
>> 
>> >  /**
>> >   * struct mipi_dsi_driver - DSI driver
>> >   * @driver: device driver model driver
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH] drm/mipi-dsi: Add a mipi_dual_dsi_dcs_write_seq() macro

[Linus Walleij]

(CC Javier)

On Fri, Mar 10, 2023 at 2:52 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:

> >> > +          for (i = 0; i < ARRAY_SIZE(dsi); i++)                    \
> >> > +                  mipi_dsi_dcs_write_seq(dsi[i], cmd, seq);        \
> >>
> >> This ignores errors.
> > mipi_dsi_dcs_write_seq is also a macro, contains error checks in the body block.
>
> Ugh, I think it's pretty scary to hide control flow like return
> statements in macros like this.

The macros are written like this because:

#define mipi_dsi_generic_write_seq(dsi, seq...)
(...)
                static const u8 d[] = { seq };

Array of bytes

                ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d));

So we can use use ARRAY_SIZE() in the macro and pass in any
arbitrary sequence, e.g.

mipi_dsi_generic_write_seq(dsi, 0xfb, 0x01);

Any function-esque definitions will (as in your example) require a
length to be passed in so it would become:

mipi_dsi_generic_write_seq(dsi, 0xfb, 0x01, 2);

And if you grep mipi_dsi_generic_write_seq | wc -l you find all the
245 opportunities to get that last len wrong and cause an out-of-bounds
bug.

I think this macro is the lesser evil for this reason, also it saves code
that you otherwise have to do manually, and one should never put
a person to do a machine's job.

Any suggestions to rewrite the macro using varargs welcome.
I think it is not very easy to do without the preprocessor.

Yours,
Linus Walleij