[PATCH v10 4/8] drm/msm: Add MSM-specific DSC helper methods

[Marijn Suijten]

On 2023-05-12 14:32:14, Jessica Zhang wrote:
> 
> Introduce MSM-specific DSC helper methods, as some calculations are
> common between DP and DSC.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan at quicinc.com>
> ---
>  drivers/gpu/drm/msm/msm_dsc_helper.h | 65 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.h b/drivers/gpu/drm/msm/msm_dsc_helper.h
> new file mode 100644
> index 000000000000..0d2a097b428d
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.h
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved
> + */
> +
> +#ifndef MSM_DSC_HELPER_H_
> +#define MSM_DSC_HELPER_H_
> +
> +#include <linux/bug.h>
> +#include <linux/math.h>
> +#include <drm/display/drm_dsc_helper.h>
> +
> +/*
> + * Helper methods for MSM specific DSC calculations that are common between timing engine,
> + * DSI, and DP.
> + */

Isn't this more common to have directly below the copyright statement,
above the includes?

> +
> +/**
> + * msm_dsc_get_bpp_int() - get bits per pixel integer value
> + * @dsc: Pointer to drm dsc config struct
> + * Returns: BPP integer value
> + */
> +static inline int msm_dsc_get_bpp_int(struct drm_dsc_config *dsc)

Const, as requested elsewhere.  But this function is not used anywhere
in any of the series (because we replaced the usages with more sensible
member accesses like slice_chunk_size).

> +{
> +	WARN_ON_ONCE(dsc->bits_per_pixel & 0xf);
> +	return dsc->bits_per_pixel >> 4;
> +}
> +
> +/**
> + * msm_dsc_get_slice_per_intf() - get number of slices per interface
> + * @dsc: Pointer to drm dsc config struct
> + * @intf_width: interface width

Width of the interface (to query), *in pixels*

> + * Returns: Integer representing the slice per interface

the *number of slices* per interface.

Also, the returned value applies specifically to *the given interface*
(width).

> + */
> +static inline int msm_dsc_get_slice_per_intf(struct drm_dsc_config *dsc, int intf_width)

Const pointer.

Also: sliceS_per_intf?  It's pluiral in the docs too.

Should the argument and return value be u32, to match the uses?  Same
for everything below.

> +{
> +	return DIV_ROUND_UP(intf_width, dsc->slice_width);
> +}
> +
> +/**
> + * msm_dsc_get_bytes_per_line() - Calculate bytes per line

Calculate -> (lowecase) get
(to match all the other helpers in this file)

> + * @dsc: Pointer to drm dsc config struct
> + * Returns: Integer value representing pclk per interface
> + *
> + * Note: This value will then be passed along to DSI and DP for some more
> + * calculations. This is because DSI and DP divide the pclk_per_intf value
> + * by different values depending on if widebus is enabled.

Can you elaborate what this "note" is trying to tell users of this
function?  That they should not use bytes_per_line raw?  That it doesn't
actually represent bytes_per_line if the extra calculations mentioned
here are not applied?

> + */
> +static inline int msm_dsc_get_bytes_per_line(struct drm_dsc_config *dsc)

const, return u32.

> +{
> +	return dsc->slice_count * dsc->slice_chunk_size;

This is a u8 times a u16.  Could it overflow a u16 and should we hence
cast one of the expressions to u32 first?

> +}
> +
> +/**
> + * msm_dsc_get_bytes_per_intf() - get total bytes per interface
> + * @dsc: Pointer to drm dsc config struct
> + * @intf_width: interface width
> + * Returns: u32 value representing bytes per interface

Nit: no need to repeat the type, I think?  Just "number of bytes per
interface" is more concise.

> + */
> +static inline u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int intf_width)

And one more const.  Not sure that this helper is useful though: it is
only used where msm_dsc_get_slice_per_intf() was already called, so it
makes more sense to the reader to just multiply slice_per_intf by
slice_chunk_size than to defer to an opaque helper.

- Marijn

> +{
> +	return dsc->slice_chunk_size * msm_dsc_get_slice_per_intf(dsc, intf_width);
> +}
> +
> +#endif /* MSM_DSC_HELPER_H_ */
> 
> -- 
> 2.40.1
>