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@xxxxxxxxxx>
> > ---
> > 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
