1. Patchew
  2. linux
  3. [PATCH v3 0/4] drm/panel: add support for LG SW43408 panel
  4. View patch

[PATCH v3 3/4] drm/mipi-dsi: add mipi_dsi_compression_mode_ext()

Dmitry Baryshkov posted 4 patches 1 year, 10 months ago
There is a newer version of this series
[PATCH v3 3/4] drm/mipi-dsi: add mipi_dsi_compression_mode_ext()
Posted by Dmitry Baryshkov 1 year, 10 months ago 
Add the extended version of mipi_dsi_compression_mode(). It provides
a way to specify the algorithm and PPS selector.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/drm_mipi_dsi.c | 33 +++++++++++++++++++++++++++------
 include/drm/drm_mipi_dsi.h     |  9 +++++++++
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 9874ff6d4718..0ecbc811eb7a 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -645,19 +645,24 @@ int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
 EXPORT_SYMBOL(mipi_dsi_set_maximum_return_packet_size);
 
 /**
- * mipi_dsi_compression_mode() - enable/disable DSC on the peripheral
+ * mipi_dsi_compression_mode_ext() - enable/disable DSC on the peripheral
  * @dsi: DSI peripheral device
  * @enable: Whether to enable or disable the DSC
+ * @algo: Selected algorithm
+ * @pps_selector: The PPS selector
  *
- * Enable or disable Display Stream Compression on the peripheral using the
- * default Picture Parameter Set and VESA DSC 1.1 algorithm.
+ * Enable or disable Display Stream Compression on the peripheral.
  *
  * Return: 0 on success or a negative error code on failure.
  */
-int mipi_dsi_compression_mode(struct mipi_dsi_device *dsi, bool enable)
+int mipi_dsi_compression_mode_ext(struct mipi_dsi_device *dsi, bool enable,
+				  enum mipi_dsi_compression_algo algo,
+				  unsigned int pps_selector)
 {
-	/* Note: Needs updating for non-default PPS or algorithm */
-	u8 tx[2] = { enable << 0, 0 };
+	u8 data = (enable << 0) |
+		(algo << 1) |
+		(pps_selector << 4);
+	u8 tx[2] = { data, 0 };
 	struct mipi_dsi_msg msg = {
 		.channel = dsi->channel,
 		.type = MIPI_DSI_COMPRESSION_MODE,
@@ -668,6 +673,22 @@ int mipi_dsi_compression_mode(struct mipi_dsi_device *dsi, bool enable)
 
 	return (ret < 0) ? ret : 0;
 }
+EXPORT_SYMBOL(mipi_dsi_compression_mode_ext);
+
+/**
+ * mipi_dsi_compression_mode() - enable/disable DSC on the peripheral
+ * @dsi: DSI peripheral device
+ * @enable: Whether to enable or disable the DSC
+ *
+ * Enable or disable Display Stream Compression on the peripheral using the
+ * default Picture Parameter Set and VESA DSC 1.1 algorithm.
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int mipi_dsi_compression_mode(struct mipi_dsi_device *dsi, bool enable)
+{
+	return mipi_dsi_compression_mode_ext(dsi, enable, 0, MIPI_DSI_COMPRESSION_DSC);
+}
 EXPORT_SYMBOL(mipi_dsi_compression_mode);
 
 /**
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 3011d33eccbd..78cb7b688b1d 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -226,6 +226,12 @@ static inline int mipi_dsi_pixel_format_to_bpp(enum mipi_dsi_pixel_format fmt)
 	return -EINVAL;
 }
 
+enum mipi_dsi_compression_algo {
+	MIPI_DSI_COMPRESSION_DSC = 0,
+	MIPI_DSI_COMPRESSION_VENDOR = 3,
+	/* other two values are reserved, DSI 1.3 */
+};
+
 struct mipi_dsi_device *
 mipi_dsi_device_register_full(struct mipi_dsi_host *host,
 			      const struct mipi_dsi_device_info *info);
@@ -242,6 +248,9 @@ int mipi_dsi_turn_on_peripheral(struct mipi_dsi_device *dsi);
 int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
 					    u16 value);
 int mipi_dsi_compression_mode(struct mipi_dsi_device *dsi, bool enable);
+int mipi_dsi_compression_mode_ext(struct mipi_dsi_device *dsi, bool enable,
+				  unsigned int pps_selector,
+				  enum mipi_dsi_compression_algo algo);
 int mipi_dsi_picture_parameter_set(struct mipi_dsi_device *dsi,
 				   const struct drm_dsc_picture_parameter_set *pps);
 

-- 
2.39.2
Re: [PATCH v3 3/4] drm/mipi-dsi: add mipi_dsi_compression_mode_ext()
Posted by Marijn Suijten 1 year, 10 months ago 
On 2024-04-02 02:51:14, Dmitry Baryshkov wrote:
> Add the extended version of mipi_dsi_compression_mode(). It provides
> a way to specify the algorithm and PPS selector.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/drm_mipi_dsi.c | 33 +++++++++++++++++++++++++++------
>  include/drm/drm_mipi_dsi.h     |  9 +++++++++
>  2 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 9874ff6d4718..0ecbc811eb7a 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -645,19 +645,24 @@ int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
>  EXPORT_SYMBOL(mipi_dsi_set_maximum_return_packet_size);
>  
>  /**
> - * mipi_dsi_compression_mode() - enable/disable DSC on the peripheral
> + * mipi_dsi_compression_mode_ext() - enable/disable DSC on the peripheral
>   * @dsi: DSI peripheral device
>   * @enable: Whether to enable or disable the DSC
> + * @algo: Selected algorithm
> + * @pps_selector: The PPS selector

Not a big fan of paraphrasing the parameter name, it adds no value.  How about
describing what this parameter means and what it does?:

	PPS table index to use.  Corresponds to a table pre-programmed on the peripheral
	or a table programmed with &drm_dsc_picture_parameter_set.pps_identifier.

(That should be a valid kernel-doc cross-reference to the field)

>   *
> - * Enable or disable Display Stream Compression on the peripheral using the
> - * default Picture Parameter Set and VESA DSC 1.1 algorithm.
> + * Enable or disable Display Stream Compression on the peripheral.
>   *
>   * Return: 0 on success or a negative error code on failure.
>   */
> -int mipi_dsi_compression_mode(struct mipi_dsi_device *dsi, bool enable)
> +int mipi_dsi_compression_mode_ext(struct mipi_dsi_device *dsi, bool enable,
> +				  enum mipi_dsi_compression_algo algo,
> +				  unsigned int pps_selector)
>  {
> -	/* Note: Needs updating for non-default PPS or algorithm */
> -	u8 tx[2] = { enable << 0, 0 };
> +	u8 data = (enable << 0) |
> +		(algo << 1) |
> +		(pps_selector << 4);

Do we need some size validation (if > 3 return -EINVAL)?  FIELD_PREP() might be
too heavy though.

> +	u8 tx[2] = { data, 0 };
>  	struct mipi_dsi_msg msg = {
>  		.channel = dsi->channel,
>  		.type = MIPI_DSI_COMPRESSION_MODE,
> @@ -668,6 +673,22 @@ int mipi_dsi_compression_mode(struct mipi_dsi_device *dsi, bool enable)
>  
>  	return (ret < 0) ? ret : 0;
>  }
> +EXPORT_SYMBOL(mipi_dsi_compression_mode_ext);
> +
> +/**
> + * mipi_dsi_compression_mode() - enable/disable DSC on the peripheral
> + * @dsi: DSI peripheral device
> + * @enable: Whether to enable or disable the DSC
> + *
> + * Enable or disable Display Stream Compression on the peripheral using the
> + * default Picture Parameter Set and VESA DSC 1.1 algorithm.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int mipi_dsi_compression_mode(struct mipi_dsi_device *dsi, bool enable)
> +{
> +	return mipi_dsi_compression_mode_ext(dsi, enable, 0, MIPI_DSI_COMPRESSION_DSC);

I hope the compiler complains here that it should be MIPI_DSI_COMPRESSION_DSC,0

(Enum algo first, int pps_selector last)

> +}
>  EXPORT_SYMBOL(mipi_dsi_compression_mode);
>  
>  /**
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index 3011d33eccbd..78cb7b688b1d 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -226,6 +226,12 @@ static inline int mipi_dsi_pixel_format_to_bpp(enum mipi_dsi_pixel_format fmt)
>  	return -EINVAL;
>  }
>  
> +enum mipi_dsi_compression_algo {
> +	MIPI_DSI_COMPRESSION_DSC = 0,

Add 1.1?  Or does it also allow 1.2 (when the version is also set via PPS)?

> +	MIPI_DSI_COMPRESSION_VENDOR = 3,
> +	/* other two values are reserved, DSI 1.3 */
> +};
> +
>  struct mipi_dsi_device *
>  mipi_dsi_device_register_full(struct mipi_dsi_host *host,
>  			      const struct mipi_dsi_device_info *info);
> @@ -242,6 +248,9 @@ int mipi_dsi_turn_on_peripheral(struct mipi_dsi_device *dsi);
>  int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
>  					    u16 value);
>  int mipi_dsi_compression_mode(struct mipi_dsi_device *dsi, bool enable);
> +int mipi_dsi_compression_mode_ext(struct mipi_dsi_device *dsi, bool enable,
> +				  unsigned int pps_selector,
> +				  enum mipi_dsi_compression_algo algo);

Oh, this declaration is inverse from the definition...

- Marijn

>  int mipi_dsi_picture_parameter_set(struct mipi_dsi_device *dsi,
>  				   const struct drm_dsc_picture_parameter_set *pps);
>  
> 
> -- 
> 2.39.2
>
Re: [PATCH v3 3/4] drm/mipi-dsi: add mipi_dsi_compression_mode_ext()
Posted by Dmitry Baryshkov 1 year, 10 months ago 
On Tue, Apr 02, 2024 at 11:09:29PM +0200, Marijn Suijten wrote:
> On 2024-04-02 02:51:14, Dmitry Baryshkov wrote:
> > Add the extended version of mipi_dsi_compression_mode(). It provides
> > a way to specify the algorithm and PPS selector.
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/gpu/drm/drm_mipi_dsi.c | 33 +++++++++++++++++++++++++++------
> >  include/drm/drm_mipi_dsi.h     |  9 +++++++++
> >  2 files changed, 36 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> > index 9874ff6d4718..0ecbc811eb7a 100644
> > --- a/drivers/gpu/drm/drm_mipi_dsi.c
> > +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> > @@ -645,19 +645,24 @@ int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
> >  EXPORT_SYMBOL(mipi_dsi_set_maximum_return_packet_size);
> >  
> >  /**
> > - * mipi_dsi_compression_mode() - enable/disable DSC on the peripheral
> > + * mipi_dsi_compression_mode_ext() - enable/disable DSC on the peripheral
> >   * @dsi: DSI peripheral device
> >   * @enable: Whether to enable or disable the DSC
> > + * @algo: Selected algorithm
> > + * @pps_selector: The PPS selector
> 
> Not a big fan of paraphrasing the parameter name, it adds no value.  How about
> describing what this parameter means and what it does?:
> 
> 	PPS table index to use.  Corresponds to a table pre-programmed on the peripheral
> 	or a table programmed with &drm_dsc_picture_parameter_set.pps_identifier.
> 
> (That should be a valid kernel-doc cross-reference to the field)

I don't think such meaning is defined in the standard. In the end, the
pps_identifier is 8-bit wide, while this field has just 2 bits and 4
possible values. The standards are pretty vague about this field. 

> 
> >   *
> > - * Enable or disable Display Stream Compression on the peripheral using the
> > - * default Picture Parameter Set and VESA DSC 1.1 algorithm.
> > + * Enable or disable Display Stream Compression on the peripheral.
> >   *
> >   * Return: 0 on success or a negative error code on failure.
> >   */
> > -int mipi_dsi_compression_mode(struct mipi_dsi_device *dsi, bool enable)
> > +int mipi_dsi_compression_mode_ext(struct mipi_dsi_device *dsi, bool enable,
> > +				  enum mipi_dsi_compression_algo algo,
> > +				  unsigned int pps_selector)
> >  {
> > -	/* Note: Needs updating for non-default PPS or algorithm */
> > -	u8 tx[2] = { enable << 0, 0 };
> > +	u8 data = (enable << 0) |
> > +		(algo << 1) |
> > +		(pps_selector << 4);
> 
> Do we need some size validation (if > 3 return -EINVAL)?  FIELD_PREP() might be
> too heavy though.

Ack, let's add it.

> 
> > +	u8 tx[2] = { data, 0 };
> >  	struct mipi_dsi_msg msg = {
> >  		.channel = dsi->channel,
> >  		.type = MIPI_DSI_COMPRESSION_MODE,
> > @@ -668,6 +673,22 @@ int mipi_dsi_compression_mode(struct mipi_dsi_device *dsi, bool enable)
> >  
> >  	return (ret < 0) ? ret : 0;
> >  }
> > +EXPORT_SYMBOL(mipi_dsi_compression_mode_ext);
> > +
> > +/**
> > + * mipi_dsi_compression_mode() - enable/disable DSC on the peripheral
> > + * @dsi: DSI peripheral device
> > + * @enable: Whether to enable or disable the DSC
> > + *
> > + * Enable or disable Display Stream Compression on the peripheral using the
> > + * default Picture Parameter Set and VESA DSC 1.1 algorithm.
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int mipi_dsi_compression_mode(struct mipi_dsi_device *dsi, bool enable)
> > +{
> > +	return mipi_dsi_compression_mode_ext(dsi, enable, 0, MIPI_DSI_COMPRESSION_DSC);
> 
> I hope the compiler complains here that it should be MIPI_DSI_COMPRESSION_DSC,0

Nope, it didn't. Both are integers.

> 
> (Enum algo first, int pps_selector last)
> 
> > +}
> >  EXPORT_SYMBOL(mipi_dsi_compression_mode);
> >  
> >  /**
> > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> > index 3011d33eccbd..78cb7b688b1d 100644
> > --- a/include/drm/drm_mipi_dsi.h
> > +++ b/include/drm/drm_mipi_dsi.h
> > @@ -226,6 +226,12 @@ static inline int mipi_dsi_pixel_format_to_bpp(enum mipi_dsi_pixel_format fmt)
> >  	return -EINVAL;
> >  }
> >  
> > +enum mipi_dsi_compression_algo {
> > +	MIPI_DSI_COMPRESSION_DSC = 0,
> 
> Add 1.1?  Or does it also allow 1.2 (when the version is also set via PPS)?

I have only DSI 1.3 at hand, which only talks about 1.1. I think 1.2 is
allowed by inheritance. That's why I skipped the version here.

> 
> > +	MIPI_DSI_COMPRESSION_VENDOR = 3,
> > +	/* other two values are reserved, DSI 1.3 */
> > +};
> > +
> >  struct mipi_dsi_device *
> >  mipi_dsi_device_register_full(struct mipi_dsi_host *host,
> >  			      const struct mipi_dsi_device_info *info);
> > @@ -242,6 +248,9 @@ int mipi_dsi_turn_on_peripheral(struct mipi_dsi_device *dsi);
> >  int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
> >  					    u16 value);
> >  int mipi_dsi_compression_mode(struct mipi_dsi_device *dsi, bool enable);
> > +int mipi_dsi_compression_mode_ext(struct mipi_dsi_device *dsi, bool enable,
> > +				  unsigned int pps_selector,
> > +				  enum mipi_dsi_compression_algo algo);
> 
> Oh, this declaration is inverse from the definition...

I'll check it.

> 
> - Marijn
> 
> >  int mipi_dsi_picture_parameter_set(struct mipi_dsi_device *dsi,
> >  				   const struct drm_dsc_picture_parameter_set *pps);
> >  
> > 
> > -- 
> > 2.39.2
> > 

-- 
With best wishes
Dmitry

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.