| Message ID | 20240402-lg-sw43408-panel-v3-3-144f17a11a56@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/panel: add support for LG SW43408 panel | expand |
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 >
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 > >
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);
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(-)