| Message ID | 20230329-rfc-msm-dsc-helper-v10-4-4cb21168c227@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce MSM-specific DSC helpers | expand |
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@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 >
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@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 >>
On 2023-05-15 13:29:21, Jessica Zhang wrote: <snip> > > 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. The inverse helper is available ad DSC_BPP in drm_dsc_helper.c. Perhaps we can move the two variants to the header and define them uniformly? This isn't MSM-specific it seems (i.e. the format supports fractional bpp but no RC parameters appear to be defined for such a format yet). <snip> > >> + * @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 Note that I'm not just asking to explain it to me, but to explain it in the documentation. The function is named bytes_per_line() but then Returns: says it returns the pclk per interface, then the note says that it actually doesn't unless extra calculations are applied. We can explain the same much more concisely by rewriting Returns and dropping Note: Returns: Integer value representing bytes per line. DSI and DP need to perform further processing/calculations to turn this into pclk_per_intf, such as dividing by different values depending on if widebus is enabled. And so we have written the same, just more concisely. Feel free to reword it slightly, such as dropping the word "processing". <snip> > > 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. The name is good, I'm just not too keen on it hiding the multiplication with msm_dsc_get_slice_per_intf() which is already also computed and available in DSI, and I assume DP too? - Marijn
On 16/05/2023 01:01, Marijn Suijten wrote: > On 2023-05-15 13:29:21, Jessica Zhang wrote: > <snip> >>> 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. > > The inverse helper is available ad DSC_BPP in drm_dsc_helper.c. Perhaps > we can move the two variants to the header and define them uniformly? > This isn't MSM-specific it seems (i.e. the format supports fractional > bpp but no RC parameters appear to be defined for such a format yet). I think DSC_BPP was removed (around v2 or v3 if I read changelog correctly). As for the fraction-point BPP, I think AMD supports .5 bpp granularity, see drivers/gpu/drm/amd/display/dc/dml/dsc/qp_tables.h
On 5/15/2023 3:01 PM, Marijn Suijten wrote: > On 2023-05-15 13:29:21, Jessica Zhang wrote: > <snip> >>> 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. > > The inverse helper is available ad DSC_BPP in drm_dsc_helper.c. Perhaps > we can move the two variants to the header and define them uniformly? > This isn't MSM-specific it seems (i.e. the format supports fractional > bpp but no RC parameters appear to be defined for such a format yet). > > <snip> >>>> + * @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 > > Note that I'm not just asking to explain it to me, but to explain it in > the documentation. The function is named bytes_per_line() but then > Returns: says it returns the pclk per interface, then the note says that > it actually doesn't unless extra calculations are applied. > > We can explain the same much more concisely by rewriting Returns and > dropping Note: > > Returns: Integer value representing bytes per line. DSI and DP need > to perform further processing/calculations to turn this into > pclk_per_intf, such as dividing by different values depending on > if widebus is enabled. > > And so we have written the same, just more concisely. Feel free to > reword it slightly, such as dropping the word "processing". Sure, sounds good. > > <snip> >>> 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. > > The name is good, I'm just not too keen on it hiding the multiplication > with msm_dsc_get_slice_per_intf() which is already also computed and > available in DSI, and I assume DP too? Got it, I see why you want to make that change now. DP only uses get_slice_per_intf() to get eol_byte_num similarly to DSI, so I can just do that then. Thanks, Jessica Zhang > > - Marijn
On 5/15/2023 3:07 PM, Dmitry Baryshkov wrote: > On 16/05/2023 01:01, Marijn Suijten wrote: >> On 2023-05-15 13:29:21, Jessica Zhang wrote: >> <snip> >>>> 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. >> >> The inverse helper is available ad DSC_BPP in drm_dsc_helper.c. Perhaps >> we can move the two variants to the header and define them uniformly? >> This isn't MSM-specific it seems (i.e. the format supports fractional >> bpp but no RC parameters appear to be defined for such a format yet). > > I think DSC_BPP was removed (around v2 or v3 if I read changelog > correctly). Hi Dmitry, That's correct, we did have a DSC_BPP macro with the same functionality as msm_dsc_get_bpp_int, but it was renamed in v2 to msm_dsc_get_bpp_int (there's a typo in the changelog... I will correct that in the next revision). However, I do see a DSC_BPP macro in drm_dsc_helper.c that has a different functionality. Thanks, Jessica Zhang > > As for the fraction-point BPP, I think AMD supports .5 bpp granularity, > see drivers/gpu/drm/amd/display/dc/dml/dsc/qp_tables.h > > -- > With best wishes > Dmitry >
On 2023-05-16 01:07:05, Dmitry Baryshkov wrote: > > On 16/05/2023 01:01, Marijn Suijten wrote: > > On 2023-05-15 13:29:21, Jessica Zhang wrote: > > <snip> > >>> 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. > > > > The inverse helper is available ad DSC_BPP in drm_dsc_helper.c. Perhaps > > we can move the two variants to the header and define them uniformly? > > This isn't MSM-specific it seems (i.e. the format supports fractional > > bpp but no RC parameters appear to be defined for such a format yet). > > I think DSC_BPP was removed (around v2 or v3 if I read changelog correctly). Seems like it was removed at some point indeed, and now the helper file picked up an identically named DSC_BPP macro but with the inverse implementation :) - at least it's a *.c file. Perhaps we can make it more consistent by defining both ways with concise macros in a header. > As for the fraction-point BPP, I think AMD supports .5 bpp granularity, > see drivers/gpu/drm/amd/display/dc/dml/dsc/qp_tables.h That won't use the helper then. > With best wishes > Dmitry - Marijn
On 2023-05-15 17:59:14, Jessica Zhang wrote: <snip> > > The name is good, I'm just not too keen on it hiding the multiplication > > with msm_dsc_get_slice_per_intf() which is already also computed and > > available in DSI, and I assume DP too? > > Got it, I see why you want to make that change now. DP only uses > get_slice_per_intf() to get eol_byte_num similarly to DSI, so I can just > do that then. Thanks, the function is indeed only called once to calculate bytes_per_intf() for eol_byte_num, and the value for slice_per_intf is already available in-line. - Marijn
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. + */ + +/** + * 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) +{ + 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 + * Returns: Integer representing the slice per interface + */ +static inline int msm_dsc_get_slice_per_intf(struct drm_dsc_config *dsc, int intf_width) +{ + return DIV_ROUND_UP(intf_width, dsc->slice_width); +} + +/** + * msm_dsc_get_bytes_per_line() - Calculate bytes per line + * @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. + */ +static inline int msm_dsc_get_bytes_per_line(struct drm_dsc_config *dsc) +{ + return dsc->slice_count * dsc->slice_chunk_size; +} + +/** + * 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 + */ +static inline u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int intf_width) +{ + return dsc->slice_chunk_size * msm_dsc_get_slice_per_intf(dsc, intf_width); +} + +#endif /* MSM_DSC_HELPER_H_ */
Introduce MSM-specific DSC helper methods, as some calculations are common between DP and DSC. Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> --- drivers/gpu/drm/msm/msm_dsc_helper.h | 65 ++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+)