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

[Jessica Zhang]

On 5/14/2023 2:25 PM, Marijn Suijten wrote:
> 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?

Hi Marijn,

Acked.

> 
>> +
>> +/**
>> + * 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).

Acked.

I would prefer to keep this helper so that we have a way to easily get 
BPP information from the DRM DSC config in the future, but if you'd 
prefer we add this helper then, I'm also ok with that.

> 
>> +{
>> +	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*

Acked.

> 
>> + * Returns: Integer representing the slice per interface
> 
> the *number of slices* per interface.
> 
> Also, the returned value applies specifically to *the given interface*
> (width).

Acked.

> 
>> + */
>> +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.

Acked.

> 
>> +{
>> +	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)

Acked.

> 
>> + * @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?

The latter -- this method is used for calculating the pclk for DSI and 
DP. While it does get the raw bytes_per_line, there are some extra 
calculations needed before it can be set as the pclk_rate. These "extra 
calculations" are different between DP and DSI.

For more context, please refer to the earlier revisions of this patch 
and the usage of the helper in dsi_host.c

> 
>> + */
>> +static inline int msm_dsc_get_bytes_per_line(struct drm_dsc_config *dsc)
> 
> const, return u32.

Acked.

> 
>> +{
>> +	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?

Acked.

> 
>> +}
>> +
>> +/**
>> + * 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.

Acked.

> 
>> + */
>> +static inline u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int intf_width)
> 
> And one more const.  

Acked.

> 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.

I would prefer to keep this as a helper as this math is common between 
DP and DSI, and I believe the name of the helper accurately reflects 
what is being calculated.

If there's any confusion with the name of the method, I am open to 
suggestions.

Thanks,

Jessica Zhang

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