| Message ID | 20230329-rfc-msm-dsc-helper-v2-2-3c13ced536b2@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce MSM-specific DSC helpers | expand |
On 31/03/2023 21:49, Jessica Zhang wrote: > Introduce MSM-specific DSC helper methods, as some calculations are > common between DP and DSC. > > Changes in v2: > - Moved files up to msm/ directory > - Dropped get_comp_ratio() helper > - Used drm_int2fixp() to convert to integers to fp > - Style changes to improve readability > - Dropped unused bpp variable in msm_dsc_get_dce_bytes_per_line() > - Changed msm_dsc_get_slice_per_intf() to a static inline method > - Dropped last division step of msm_dsc_get_pclk_per_line() and changed > method name accordingly > - Changed DSC_BPP macro to drm_dsc_get_bpp_int() helper method > - Fixed some math issues caused by passing in incorrect types to > drm_fixed methods in get_bytes_per_soft_slice() > > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > --- > drivers/gpu/drm/msm/Makefile | 1 + > drivers/gpu/drm/msm/msm_dsc_helper.c | 53 ++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/msm/msm_dsc_helper.h | 42 ++++++++++++++++++++++++++++ > 3 files changed, 96 insertions(+) > > diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile > index 7274c41228ed..b814fc80e2d5 100644 > --- a/drivers/gpu/drm/msm/Makefile > +++ b/drivers/gpu/drm/msm/Makefile > @@ -94,6 +94,7 @@ msm-y += \ > msm_atomic_tracepoints.o \ > msm_debugfs.o \ > msm_drv.o \ > + msm_dsc_helper.o \ > msm_fb.o \ > msm_fence.o \ > msm_gem.o \ > diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.c b/drivers/gpu/drm/msm/msm_dsc_helper.c > new file mode 100644 > index 000000000000..60b73e17e6eb > --- /dev/null > +++ b/drivers/gpu/drm/msm/msm_dsc_helper.c > @@ -0,0 +1,53 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved > + */ > + > +#include <linux/kernel.h> > +#include <linux/errno.h> > +#include <drm/drm_fixed.h> > + > +#include "msm_drv.h" > +#include "msm_dsc_helper.h" > + > +static s64 get_bytes_per_soft_slice(struct drm_dsc_config *dsc, int intf_width, u32 src_bpp) intf_width is unused > +{ > + int bpp = msm_dsc_get_bpp_int(dsc); > + s64 numerator_fp, denominator_fp; > + s64 comp_ratio_fp = drm_fixp_from_fraction(src_bpp, bpp); > + > + numerator_fp = drm_int2fixp(dsc->slice_width * 3); You have lost dsc->bits_per_component here. > + denominator_fp = drm_fixp_from_fraction(comp_ratio_fp * 8, drm_int2fixp(bpp)); denominator_fp = drm_fixp_from_fraction(src_bpp * 8, bpp); > + > + return drm_fixp_div(numerator_fp, denominator_fp); > +} > + > +u32 msm_dsc_get_eol_byte_num(struct drm_dsc_config *dsc, int intf_width, u32 src_bpp) > +{ > + u32 bytes_per_soft_slice, extra_eol_bytes, bytes_per_intf; > + s64 bytes_per_soft_slice_fp; > + int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width); > + > + bytes_per_soft_slice_fp = get_bytes_per_soft_slice(dsc, intf_width, src_bpp); > + bytes_per_soft_slice = drm_fixp2int_ceil(bytes_per_soft_slice_fp); > + > + bytes_per_intf = bytes_per_soft_slice * slice_per_intf; > + extra_eol_bytes = bytes_per_intf % 3; > + if (extra_eol_bytes != 0) > + extra_eol_bytes = 3 - extra_eol_bytes; I become confused here when I checked eol_bytes in the display techpack. I see that for DP the dp_panel_dsc_pclk_param_calc() calculates dsc->eol_bytes_num in this way, the size to pad dsc_byte_count * slice_per_intf to 3 bytes. However, for DSI this is a simple as total_bytes_per_intf % 3 , so it is not a padding, but a length of the last chunk. Could you please clarify? If the techpack code is correct, I'd prefer if we return last chunk size here and calculate the padding length in the DP driver. > + > + return extra_eol_bytes; > +} > + > +int msm_dsc_get_uncompressed_pclk_per_line(struct drm_dsc_config *dsc, int intf_width, u32 src_bpp) Basing on Abhinav's description ("pclk_per_line can be only per interface") would it better be named as msm_dsc_get_uncompressed_pclk_per_intf() ? or msm_dsc_get_uncompressed_pclk_for_intf() ? BTW: if get_bytes_per_soft_slice() doesn't use intf_width, we can probably drop it here too. > +{ > + s64 data_width; > + > + if (!dsc->slice_width || (intf_width < dsc->slice_width)) > + return -EINVAL; Error code is not validated at dsi_timing_setup. I'd suggest moving error checks there and dropping the error handling here. If dsc->slice_width is not set, we should stop much earlier than drm_bridge's pre_enable() callback. > + > + data_width = drm_fixp_mul(dsc->slice_count, > + get_bytes_per_soft_slice(dsc, intf_width, src_bpp)); > + > + return drm_fixp2int_ceil(data_width); > +} > 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..743cd324b7d9 > --- /dev/null > +++ b/drivers/gpu/drm/msm/msm_dsc_helper.h > @@ -0,0 +1,42 @@ > +/* 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 <drm/display/drm_dsc_helper.h> > +#include <drm/drm_modes.h> > + > +/* > + * Helper methods for MSM specific DSC calculations that are common between timing engine, > + * DSI, and DP. > + */ > + > +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; > +} > + > +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); > +} > + > +static inline u32 msm_dsc_get_dce_bytes_per_line(struct drm_dsc_config *dsc, int intf_width) > +{ > + return DIV_ROUND_UP(msm_dsc_get_bpp_int(dsc) * intf_width, 8); > +} > + > +u32 msm_dsc_get_eol_byte_num(struct drm_dsc_config *dsc, int intf_width, u32 src_bpp); > +u32 msm_dsc_get_dce_bytes_per_line(struct drm_dsc_config *dsc, int intf_width); > + > +/* Calculate uncompressed pclk per line. This value will then be passed along to > + * DSI and DP to calculate pclk_per_line. This is because DSI and DP divide the > + * uncompressed pclk_per_line by different values depending on if widebus is enabled. > + */ > +int msm_dsc_get_uncompressed_pclk_per_line(struct drm_dsc_config *dsc, > + int intf_width, u32 src_bpp); > +#endif /* MSM_DSC_HELPER_H_ */ >
On 4/2/2023 4:21 AM, Dmitry Baryshkov wrote: > On 31/03/2023 21:49, Jessica Zhang wrote: >> Introduce MSM-specific DSC helper methods, as some calculations are >> common between DP and DSC. >> >> Changes in v2: >> - Moved files up to msm/ directory >> - Dropped get_comp_ratio() helper >> - Used drm_int2fixp() to convert to integers to fp >> - Style changes to improve readability >> - Dropped unused bpp variable in msm_dsc_get_dce_bytes_per_line() >> - Changed msm_dsc_get_slice_per_intf() to a static inline method >> - Dropped last division step of msm_dsc_get_pclk_per_line() and changed >> method name accordingly >> - Changed DSC_BPP macro to drm_dsc_get_bpp_int() helper method >> - Fixed some math issues caused by passing in incorrect types to >> drm_fixed methods in get_bytes_per_soft_slice() >> >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >> --- >> drivers/gpu/drm/msm/Makefile | 1 + >> drivers/gpu/drm/msm/msm_dsc_helper.c | 53 >> ++++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/msm/msm_dsc_helper.h | 42 ++++++++++++++++++++++++++++ >> 3 files changed, 96 insertions(+) >> >> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile >> index 7274c41228ed..b814fc80e2d5 100644 >> --- a/drivers/gpu/drm/msm/Makefile >> +++ b/drivers/gpu/drm/msm/Makefile >> @@ -94,6 +94,7 @@ msm-y += \ >> msm_atomic_tracepoints.o \ >> msm_debugfs.o \ >> msm_drv.o \ >> + msm_dsc_helper.o \ >> msm_fb.o \ >> msm_fence.o \ >> msm_gem.o \ >> diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.c >> b/drivers/gpu/drm/msm/msm_dsc_helper.c >> new file mode 100644 >> index 000000000000..60b73e17e6eb >> --- /dev/null >> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.c >> @@ -0,0 +1,53 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights >> reserved >> + */ >> + >> +#include <linux/kernel.h> >> +#include <linux/errno.h> >> +#include <drm/drm_fixed.h> >> + >> +#include "msm_drv.h" >> +#include "msm_dsc_helper.h" >> + >> +static s64 get_bytes_per_soft_slice(struct drm_dsc_config *dsc, int >> intf_width, u32 src_bpp) > > intf_width is unused Hi Dmitry, Acked. > >> +{ >> + int bpp = msm_dsc_get_bpp_int(dsc); >> + s64 numerator_fp, denominator_fp; >> + s64 comp_ratio_fp = drm_fixp_from_fraction(src_bpp, bpp); >> + >> + numerator_fp = drm_int2fixp(dsc->slice_width * 3); > > You have lost dsc->bits_per_component here. This was moved to the denominator calculation, but I'll move it back to this line to avoid confusion. > >> + denominator_fp = drm_fixp_from_fraction(comp_ratio_fp * 8, >> drm_int2fixp(bpp)); > > denominator_fp = drm_fixp_from_fraction(src_bpp * 8, bpp); Acked. > >> + >> + return drm_fixp_div(numerator_fp, denominator_fp); >> +} >> + >> +u32 msm_dsc_get_eol_byte_num(struct drm_dsc_config *dsc, int >> intf_width, u32 src_bpp) >> +{ >> + u32 bytes_per_soft_slice, extra_eol_bytes, bytes_per_intf; >> + s64 bytes_per_soft_slice_fp; >> + int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width); >> + >> + bytes_per_soft_slice_fp = get_bytes_per_soft_slice(dsc, >> intf_width, src_bpp); >> + bytes_per_soft_slice = drm_fixp2int_ceil(bytes_per_soft_slice_fp); >> + >> + bytes_per_intf = bytes_per_soft_slice * slice_per_intf; >> + extra_eol_bytes = bytes_per_intf % 3; >> + if (extra_eol_bytes != 0) >> + extra_eol_bytes = 3 - extra_eol_bytes; > > I become confused here when I checked eol_bytes in the display techpack. > > I see that for DP the dp_panel_dsc_pclk_param_calc() calculates > dsc->eol_bytes_num in this way, the size to pad dsc_byte_count * > slice_per_intf to 3 bytes. > > However, for DSI this is a simple as total_bytes_per_intf % 3 , so it is > not a padding, but a length of the last chunk. > > Could you please clarify? If the techpack code is correct, I'd prefer if > we return last chunk size here and calculate the padding length in the > DP driver. I've double checked the calculations between DP and DSI, and I think you're right. Will move the `if (extra_eol_bytes != 0)` block out to DP code. > >> + >> + return extra_eol_bytes; >> +} >> + >> +int msm_dsc_get_uncompressed_pclk_per_line(struct drm_dsc_config >> *dsc, int intf_width, u32 src_bpp) > > Basing on Abhinav's description ("pclk_per_line can be only per > interface") would it better be named as > msm_dsc_get_uncompressed_pclk_per_intf() ? or > msm_dsc_get_uncompressed_pclk_for_intf() ? > > BTW: if get_bytes_per_soft_slice() doesn't use intf_width, we can > probably drop it here too. > >> +{ >> + s64 data_width; >> + >> + if (!dsc->slice_width || (intf_width < dsc->slice_width)) >> + return -EINVAL; > > Error code is not validated at dsi_timing_setup. I'd suggest moving > error checks there and dropping the error handling here. If > dsc->slice_width is not set, we should stop much earlier than > drm_bridge's pre_enable() callback. Acked. Thanks, Jessica Zhang > >> + >> + data_width = drm_fixp_mul(dsc->slice_count, >> + get_bytes_per_soft_slice(dsc, intf_width, src_bpp)); >> + >> + return drm_fixp2int_ceil(data_width); >> +} >> 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..743cd324b7d9 >> --- /dev/null >> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.h >> @@ -0,0 +1,42 @@ >> +/* 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 <drm/display/drm_dsc_helper.h> >> +#include <drm/drm_modes.h> >> + >> +/* >> + * Helper methods for MSM specific DSC calculations that are common >> between timing engine, >> + * DSI, and DP. >> + */ >> + >> +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; >> +} >> + >> +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); >> +} >> + >> +static inline u32 msm_dsc_get_dce_bytes_per_line(struct >> drm_dsc_config *dsc, int intf_width) >> +{ >> + return DIV_ROUND_UP(msm_dsc_get_bpp_int(dsc) * intf_width, 8); >> +} >> + >> +u32 msm_dsc_get_eol_byte_num(struct drm_dsc_config *dsc, int >> intf_width, u32 src_bpp); >> +u32 msm_dsc_get_dce_bytes_per_line(struct drm_dsc_config *dsc, int >> intf_width); >> + >> +/* Calculate uncompressed pclk per line. This value will then be >> passed along to >> + * DSI and DP to calculate pclk_per_line. This is because DSI and DP >> divide the >> + * uncompressed pclk_per_line by different values depending on if >> widebus is enabled. >> + */ >> +int msm_dsc_get_uncompressed_pclk_per_line(struct drm_dsc_config *dsc, >> + int intf_width, u32 src_bpp); >> +#endif /* MSM_DSC_HELPER_H_ */ >> > > -- > With best wishes > Dmitry >
On 04/04/2023 00:38, Jessica Zhang wrote: > > > On 4/2/2023 4:21 AM, Dmitry Baryshkov wrote: >> On 31/03/2023 21:49, Jessica Zhang wrote: >>> Introduce MSM-specific DSC helper methods, as some calculations are >>> common between DP and DSC. >>> >>> Changes in v2: >>> - Moved files up to msm/ directory >>> - Dropped get_comp_ratio() helper >>> - Used drm_int2fixp() to convert to integers to fp >>> - Style changes to improve readability >>> - Dropped unused bpp variable in msm_dsc_get_dce_bytes_per_line() >>> - Changed msm_dsc_get_slice_per_intf() to a static inline method >>> - Dropped last division step of msm_dsc_get_pclk_per_line() and changed >>> method name accordingly >>> - Changed DSC_BPP macro to drm_dsc_get_bpp_int() helper method >>> - Fixed some math issues caused by passing in incorrect types to >>> drm_fixed methods in get_bytes_per_soft_slice() >>> >>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>> --- >>> drivers/gpu/drm/msm/Makefile | 1 + >>> drivers/gpu/drm/msm/msm_dsc_helper.c | 53 >>> ++++++++++++++++++++++++++++++++++++ >>> drivers/gpu/drm/msm/msm_dsc_helper.h | 42 ++++++++++++++++++++++++++++ >>> 3 files changed, 96 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile >>> index 7274c41228ed..b814fc80e2d5 100644 >>> --- a/drivers/gpu/drm/msm/Makefile >>> +++ b/drivers/gpu/drm/msm/Makefile >>> @@ -94,6 +94,7 @@ msm-y += \ >>> msm_atomic_tracepoints.o \ >>> msm_debugfs.o \ >>> msm_drv.o \ >>> + msm_dsc_helper.o \ >>> msm_fb.o \ >>> msm_fence.o \ >>> msm_gem.o \ >>> diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.c >>> b/drivers/gpu/drm/msm/msm_dsc_helper.c >>> new file mode 100644 >>> index 000000000000..60b73e17e6eb >>> --- /dev/null >>> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.c >>> @@ -0,0 +1,53 @@ >>> +// SPDX-License-Identifier: GPL-2.0-only >>> +/* >>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights >>> reserved >>> + */ >>> + >>> +#include <linux/kernel.h> >>> +#include <linux/errno.h> >>> +#include <drm/drm_fixed.h> >>> + >>> +#include "msm_drv.h" >>> +#include "msm_dsc_helper.h" >>> + >>> +static s64 get_bytes_per_soft_slice(struct drm_dsc_config *dsc, int >>> intf_width, u32 src_bpp) >> >> intf_width is unused > > Hi Dmitry, > > Acked. > >> >>> +{ >>> + int bpp = msm_dsc_get_bpp_int(dsc); >>> + s64 numerator_fp, denominator_fp; >>> + s64 comp_ratio_fp = drm_fixp_from_fraction(src_bpp, bpp); >>> + >>> + numerator_fp = drm_int2fixp(dsc->slice_width * 3); >> >> You have lost dsc->bits_per_component here. > > This was moved to the denominator calculation, but I'll move it back to > this line to avoid confusion. Maybe you occasionally mixed bpp and bpc, because there is no bits_per_component usage in denominator. Could you please recheck the calculations. > >> >>> + denominator_fp = drm_fixp_from_fraction(comp_ratio_fp * 8, >>> drm_int2fixp(bpp)); >> >> denominator_fp = drm_fixp_from_fraction(src_bpp * 8, bpp); > > Acked. > >> >>> + >>> + return drm_fixp_div(numerator_fp, denominator_fp); >>> +} >>> + >>> +u32 msm_dsc_get_eol_byte_num(struct drm_dsc_config *dsc, int >>> intf_width, u32 src_bpp) >>> +{ >>> + u32 bytes_per_soft_slice, extra_eol_bytes, bytes_per_intf; >>> + s64 bytes_per_soft_slice_fp; >>> + int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width); >>> + >>> + bytes_per_soft_slice_fp = get_bytes_per_soft_slice(dsc, >>> intf_width, src_bpp); >>> + bytes_per_soft_slice = drm_fixp2int_ceil(bytes_per_soft_slice_fp); >>> + >>> + bytes_per_intf = bytes_per_soft_slice * slice_per_intf; >>> + extra_eol_bytes = bytes_per_intf % 3; >>> + if (extra_eol_bytes != 0) >>> + extra_eol_bytes = 3 - extra_eol_bytes; >> >> I become confused here when I checked eol_bytes in the display techpack. >> >> I see that for DP the dp_panel_dsc_pclk_param_calc() calculates >> dsc->eol_bytes_num in this way, the size to pad dsc_byte_count * >> slice_per_intf to 3 bytes. >> >> However, for DSI this is a simple as total_bytes_per_intf % 3 , so it >> is not a padding, but a length of the last chunk. >> >> Could you please clarify? If the techpack code is correct, I'd prefer >> if we return last chunk size here and calculate the padding length in >> the DP driver. > > I've double checked the calculations between DP and DSI, and I think > you're right. Will move the `if (extra_eol_bytes != 0)` block out to DP > code. Ack. Could you please check with HW team that our understanding is correct? > >> >>> + >>> + return extra_eol_bytes; >>> +} >>> + >>> +int msm_dsc_get_uncompressed_pclk_per_line(struct drm_dsc_config >>> *dsc, int intf_width, u32 src_bpp) >> >> Basing on Abhinav's description ("pclk_per_line can be only per >> interface") would it better be named as >> msm_dsc_get_uncompressed_pclk_per_intf() ? or >> msm_dsc_get_uncompressed_pclk_for_intf() ? >> >> BTW: if get_bytes_per_soft_slice() doesn't use intf_width, we can >> probably drop it here too. >> >>> +{ >>> + s64 data_width; >>> + >>> + if (!dsc->slice_width || (intf_width < dsc->slice_width)) >>> + return -EINVAL; >> >> Error code is not validated at dsi_timing_setup. I'd suggest moving >> error checks there and dropping the error handling here. If >> dsc->slice_width is not set, we should stop much earlier than >> drm_bridge's pre_enable() callback. > > Acked. > > Thanks, > > Jessica Zhang > >> >>> + >>> + data_width = drm_fixp_mul(dsc->slice_count, >>> + get_bytes_per_soft_slice(dsc, intf_width, src_bpp)); >>> + >>> + return drm_fixp2int_ceil(data_width); >>> +} >>> 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..743cd324b7d9 >>> --- /dev/null >>> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.h >>> @@ -0,0 +1,42 @@ >>> +/* 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 <drm/display/drm_dsc_helper.h> >>> +#include <drm/drm_modes.h> >>> + >>> +/* >>> + * Helper methods for MSM specific DSC calculations that are common >>> between timing engine, >>> + * DSI, and DP. >>> + */ >>> + >>> +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; >>> +} >>> + >>> +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); >>> +} >>> + >>> +static inline u32 msm_dsc_get_dce_bytes_per_line(struct >>> drm_dsc_config *dsc, int intf_width) >>> +{ >>> + return DIV_ROUND_UP(msm_dsc_get_bpp_int(dsc) * intf_width, 8); >>> +} >>> + >>> +u32 msm_dsc_get_eol_byte_num(struct drm_dsc_config *dsc, int >>> intf_width, u32 src_bpp); >>> +u32 msm_dsc_get_dce_bytes_per_line(struct drm_dsc_config *dsc, int >>> intf_width); >>> + >>> +/* Calculate uncompressed pclk per line. This value will then be >>> passed along to >>> + * DSI and DP to calculate pclk_per_line. This is because DSI and DP >>> divide the >>> + * uncompressed pclk_per_line by different values depending on if >>> widebus is enabled. >>> + */ >>> +int msm_dsc_get_uncompressed_pclk_per_line(struct drm_dsc_config *dsc, >>> + int intf_width, u32 src_bpp); >>> +#endif /* MSM_DSC_HELPER_H_ */ >>> >> >> -- >> With best wishes >> Dmitry >>
On 4/3/2023 5:33 PM, Dmitry Baryshkov wrote: > On 04/04/2023 00:38, Jessica Zhang wrote: >> >> >> On 4/2/2023 4:21 AM, Dmitry Baryshkov wrote: >>> On 31/03/2023 21:49, Jessica Zhang wrote: >>>> Introduce MSM-specific DSC helper methods, as some calculations are >>>> common between DP and DSC. >>>> >>>> Changes in v2: >>>> - Moved files up to msm/ directory >>>> - Dropped get_comp_ratio() helper >>>> - Used drm_int2fixp() to convert to integers to fp >>>> - Style changes to improve readability >>>> - Dropped unused bpp variable in msm_dsc_get_dce_bytes_per_line() >>>> - Changed msm_dsc_get_slice_per_intf() to a static inline method >>>> - Dropped last division step of msm_dsc_get_pclk_per_line() and changed >>>> method name accordingly >>>> - Changed DSC_BPP macro to drm_dsc_get_bpp_int() helper method >>>> - Fixed some math issues caused by passing in incorrect types to >>>> drm_fixed methods in get_bytes_per_soft_slice() >>>> >>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>>> --- >>>> drivers/gpu/drm/msm/Makefile | 1 + >>>> drivers/gpu/drm/msm/msm_dsc_helper.c | 53 >>>> ++++++++++++++++++++++++++++++++++++ >>>> drivers/gpu/drm/msm/msm_dsc_helper.h | 42 >>>> ++++++++++++++++++++++++++++ >>>> 3 files changed, 96 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/msm/Makefile >>>> b/drivers/gpu/drm/msm/Makefile >>>> index 7274c41228ed..b814fc80e2d5 100644 >>>> --- a/drivers/gpu/drm/msm/Makefile >>>> +++ b/drivers/gpu/drm/msm/Makefile >>>> @@ -94,6 +94,7 @@ msm-y += \ >>>> msm_atomic_tracepoints.o \ >>>> msm_debugfs.o \ >>>> msm_drv.o \ >>>> + msm_dsc_helper.o \ >>>> msm_fb.o \ >>>> msm_fence.o \ >>>> msm_gem.o \ >>>> diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.c >>>> b/drivers/gpu/drm/msm/msm_dsc_helper.c >>>> new file mode 100644 >>>> index 000000000000..60b73e17e6eb >>>> --- /dev/null >>>> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.c >>>> @@ -0,0 +1,53 @@ >>>> +// SPDX-License-Identifier: GPL-2.0-only >>>> +/* >>>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights >>>> reserved >>>> + */ >>>> + >>>> +#include <linux/kernel.h> >>>> +#include <linux/errno.h> >>>> +#include <drm/drm_fixed.h> >>>> + >>>> +#include "msm_drv.h" >>>> +#include "msm_dsc_helper.h" >>>> + >>>> +static s64 get_bytes_per_soft_slice(struct drm_dsc_config *dsc, int >>>> intf_width, u32 src_bpp) >>> >>> intf_width is unused >> >> Hi Dmitry, >> >> Acked. >> >>> >>>> +{ >>>> + int bpp = msm_dsc_get_bpp_int(dsc); >>>> + s64 numerator_fp, denominator_fp; >>>> + s64 comp_ratio_fp = drm_fixp_from_fraction(src_bpp, bpp); >>>> + >>>> + numerator_fp = drm_int2fixp(dsc->slice_width * 3); >>> >>> You have lost dsc->bits_per_component here. >> >> This was moved to the denominator calculation, but I'll move it back >> to this line to avoid confusion. > > Maybe you occasionally mixed bpp and bpc, because there is no > bits_per_component usage in denominator. Could you please recheck the > calculations. > >> >>> >>>> + denominator_fp = drm_fixp_from_fraction(comp_ratio_fp * 8, >>>> drm_int2fixp(bpp)); >>> >>> denominator_fp = drm_fixp_from_fraction(src_bpp * 8, bpp); >> >> Acked. >> >>> >>>> + >>>> + return drm_fixp_div(numerator_fp, denominator_fp); >>>> +} >>>> + >>>> +u32 msm_dsc_get_eol_byte_num(struct drm_dsc_config *dsc, int >>>> intf_width, u32 src_bpp) >>>> +{ >>>> + u32 bytes_per_soft_slice, extra_eol_bytes, bytes_per_intf; >>>> + s64 bytes_per_soft_slice_fp; >>>> + int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width); >>>> + >>>> + bytes_per_soft_slice_fp = get_bytes_per_soft_slice(dsc, >>>> intf_width, src_bpp); >>>> + bytes_per_soft_slice = drm_fixp2int_ceil(bytes_per_soft_slice_fp); >>>> + >>>> + bytes_per_intf = bytes_per_soft_slice * slice_per_intf; >>>> + extra_eol_bytes = bytes_per_intf % 3; >>>> + if (extra_eol_bytes != 0) >>>> + extra_eol_bytes = 3 - extra_eol_bytes; >>> >>> I become confused here when I checked eol_bytes in the display techpack. >>> >>> I see that for DP the dp_panel_dsc_pclk_param_calc() calculates >>> dsc->eol_bytes_num in this way, the size to pad dsc_byte_count * >>> slice_per_intf to 3 bytes. >>> >>> However, for DSI this is a simple as total_bytes_per_intf % 3 , so it >>> is not a padding, but a length of the last chunk. >>> >>> Could you please clarify? If the techpack code is correct, I'd prefer >>> if we return last chunk size here and calculate the padding length in >>> the DP driver. >> >> I've double checked the calculations between DP and DSI, and I think >> you're right. Will move the `if (extra_eol_bytes != 0)` block out to >> DP code. > > Ack. Could you please check with HW team that our understanding is correct? Hey Dmitry, I've checked with the HW team and looks like the math for eol_byte_nums differs between DP and DSI. For DSI, eol_byte_num = bytes_per_intf % 3 But for DP, eol_byte_num = (bytes_per_intf % 3 == 0) ? 0 : 3 - bytes_per_intf % 3 *only* for non-widebus. For DP && widebus enabled, eol_byte_num = (bytes_per_intf % 6 == 0) ? 0 : 6 - bytes_per_intf % 6 In that case, we should move even the bytes_per_intf % 3 out and change this method to msm_dsc_get_bytes_per_intf() instead. Thanks, Jessica Zhang > >> >>> >>>> + >>>> + return extra_eol_bytes; >>>> +} >>>> + >>>> +int msm_dsc_get_uncompressed_pclk_per_line(struct drm_dsc_config >>>> *dsc, int intf_width, u32 src_bpp) >>> >>> Basing on Abhinav's description ("pclk_per_line can be only per >>> interface") would it better be named as >>> msm_dsc_get_uncompressed_pclk_per_intf() ? or >>> msm_dsc_get_uncompressed_pclk_for_intf() ? >>> >>> BTW: if get_bytes_per_soft_slice() doesn't use intf_width, we can >>> probably drop it here too. >>> >>>> +{ >>>> + s64 data_width; >>>> + >>>> + if (!dsc->slice_width || (intf_width < dsc->slice_width)) >>>> + return -EINVAL; >>> >>> Error code is not validated at dsi_timing_setup. I'd suggest moving >>> error checks there and dropping the error handling here. If >>> dsc->slice_width is not set, we should stop much earlier than >>> drm_bridge's pre_enable() callback. >> >> Acked. >> >> Thanks, >> >> Jessica Zhang >> >>> >>>> + >>>> + data_width = drm_fixp_mul(dsc->slice_count, >>>> + get_bytes_per_soft_slice(dsc, intf_width, src_bpp)); >>>> + >>>> + return drm_fixp2int_ceil(data_width); >>>> +} >>>> 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..743cd324b7d9 >>>> --- /dev/null >>>> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.h >>>> @@ -0,0 +1,42 @@ >>>> +/* 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 <drm/display/drm_dsc_helper.h> >>>> +#include <drm/drm_modes.h> >>>> + >>>> +/* >>>> + * Helper methods for MSM specific DSC calculations that are common >>>> between timing engine, >>>> + * DSI, and DP. >>>> + */ >>>> + >>>> +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; >>>> +} >>>> + >>>> +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); >>>> +} >>>> + >>>> +static inline u32 msm_dsc_get_dce_bytes_per_line(struct >>>> drm_dsc_config *dsc, int intf_width) >>>> +{ >>>> + return DIV_ROUND_UP(msm_dsc_get_bpp_int(dsc) * intf_width, 8); >>>> +} >>>> + >>>> +u32 msm_dsc_get_eol_byte_num(struct drm_dsc_config *dsc, int >>>> intf_width, u32 src_bpp); >>>> +u32 msm_dsc_get_dce_bytes_per_line(struct drm_dsc_config *dsc, int >>>> intf_width); >>>> + >>>> +/* Calculate uncompressed pclk per line. This value will then be >>>> passed along to >>>> + * DSI and DP to calculate pclk_per_line. This is because DSI and >>>> DP divide the >>>> + * uncompressed pclk_per_line by different values depending on if >>>> widebus is enabled. >>>> + */ >>>> +int msm_dsc_get_uncompressed_pclk_per_line(struct drm_dsc_config *dsc, >>>> + int intf_width, u32 src_bpp); >>>> +#endif /* MSM_DSC_HELPER_H_ */ >>>> >>> >>> -- >>> With best wishes >>> Dmitry >>> > > -- > With best wishes > Dmitry >
On 04/04/2023 19:29, Jessica Zhang wrote: > > > On 4/3/2023 5:33 PM, Dmitry Baryshkov wrote: >> On 04/04/2023 00:38, Jessica Zhang wrote: >>> >>> >>> On 4/2/2023 4:21 AM, Dmitry Baryshkov wrote: >>>> On 31/03/2023 21:49, Jessica Zhang wrote: >>>>> Introduce MSM-specific DSC helper methods, as some calculations are >>>>> common between DP and DSC. >>>>> >>>>> Changes in v2: >>>>> - Moved files up to msm/ directory >>>>> - Dropped get_comp_ratio() helper >>>>> - Used drm_int2fixp() to convert to integers to fp >>>>> - Style changes to improve readability >>>>> - Dropped unused bpp variable in msm_dsc_get_dce_bytes_per_line() >>>>> - Changed msm_dsc_get_slice_per_intf() to a static inline method >>>>> - Dropped last division step of msm_dsc_get_pclk_per_line() and >>>>> changed >>>>> method name accordingly >>>>> - Changed DSC_BPP macro to drm_dsc_get_bpp_int() helper method >>>>> - Fixed some math issues caused by passing in incorrect types to >>>>> drm_fixed methods in get_bytes_per_soft_slice() >>>>> >>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>>>> --- >>>>> drivers/gpu/drm/msm/Makefile | 1 + >>>>> drivers/gpu/drm/msm/msm_dsc_helper.c | 53 >>>>> ++++++++++++++++++++++++++++++++++++ >>>>> drivers/gpu/drm/msm/msm_dsc_helper.h | 42 >>>>> ++++++++++++++++++++++++++++ >>>>> 3 files changed, 96 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/msm/Makefile >>>>> b/drivers/gpu/drm/msm/Makefile >>>>> index 7274c41228ed..b814fc80e2d5 100644 >>>>> --- a/drivers/gpu/drm/msm/Makefile >>>>> +++ b/drivers/gpu/drm/msm/Makefile >>>>> @@ -94,6 +94,7 @@ msm-y += \ >>>>> msm_atomic_tracepoints.o \ >>>>> msm_debugfs.o \ >>>>> msm_drv.o \ >>>>> + msm_dsc_helper.o \ >>>>> msm_fb.o \ >>>>> msm_fence.o \ >>>>> msm_gem.o \ >>>>> diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.c >>>>> b/drivers/gpu/drm/msm/msm_dsc_helper.c >>>>> new file mode 100644 >>>>> index 000000000000..60b73e17e6eb >>>>> --- /dev/null >>>>> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.c >>>>> @@ -0,0 +1,53 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0-only >>>>> +/* >>>>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights >>>>> reserved >>>>> + */ >>>>> + >>>>> +#include <linux/kernel.h> >>>>> +#include <linux/errno.h> >>>>> +#include <drm/drm_fixed.h> >>>>> + >>>>> +#include "msm_drv.h" >>>>> +#include "msm_dsc_helper.h" >>>>> + >>>>> +static s64 get_bytes_per_soft_slice(struct drm_dsc_config *dsc, >>>>> int intf_width, u32 src_bpp) >>>> >>>> intf_width is unused >>> >>> Hi Dmitry, >>> >>> Acked. >>> >>>> >>>>> +{ >>>>> + int bpp = msm_dsc_get_bpp_int(dsc); >>>>> + s64 numerator_fp, denominator_fp; >>>>> + s64 comp_ratio_fp = drm_fixp_from_fraction(src_bpp, bpp); >>>>> + >>>>> + numerator_fp = drm_int2fixp(dsc->slice_width * 3); >>>> >>>> You have lost dsc->bits_per_component here. >>> >>> This was moved to the denominator calculation, but I'll move it back >>> to this line to avoid confusion. >> >> Maybe you occasionally mixed bpp and bpc, because there is no >> bits_per_component usage in denominator. Could you please recheck the >> calculations. >> >>> >>>> >>>>> + denominator_fp = drm_fixp_from_fraction(comp_ratio_fp * 8, >>>>> drm_int2fixp(bpp)); >>>> >>>> denominator_fp = drm_fixp_from_fraction(src_bpp * 8, bpp); >>> >>> Acked. >>> >>>> >>>>> + >>>>> + return drm_fixp_div(numerator_fp, denominator_fp); >>>>> +} >>>>> + >>>>> +u32 msm_dsc_get_eol_byte_num(struct drm_dsc_config *dsc, int >>>>> intf_width, u32 src_bpp) >>>>> +{ >>>>> + u32 bytes_per_soft_slice, extra_eol_bytes, bytes_per_intf; >>>>> + s64 bytes_per_soft_slice_fp; >>>>> + int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width); >>>>> + >>>>> + bytes_per_soft_slice_fp = get_bytes_per_soft_slice(dsc, >>>>> intf_width, src_bpp); >>>>> + bytes_per_soft_slice = >>>>> drm_fixp2int_ceil(bytes_per_soft_slice_fp); >>>>> + >>>>> + bytes_per_intf = bytes_per_soft_slice * slice_per_intf; >>>>> + extra_eol_bytes = bytes_per_intf % 3; >>>>> + if (extra_eol_bytes != 0) >>>>> + extra_eol_bytes = 3 - extra_eol_bytes; >>>> >>>> I become confused here when I checked eol_bytes in the display >>>> techpack. >>>> >>>> I see that for DP the dp_panel_dsc_pclk_param_calc() calculates >>>> dsc->eol_bytes_num in this way, the size to pad dsc_byte_count * >>>> slice_per_intf to 3 bytes. >>>> >>>> However, for DSI this is a simple as total_bytes_per_intf % 3 , so >>>> it is not a padding, but a length of the last chunk. >>>> >>>> Could you please clarify? If the techpack code is correct, I'd >>>> prefer if we return last chunk size here and calculate the padding >>>> length in the DP driver. >>> >>> I've double checked the calculations between DP and DSI, and I think >>> you're right. Will move the `if (extra_eol_bytes != 0)` block out to >>> DP code. >> >> Ack. Could you please check with HW team that our understanding is >> correct? > > Hey Dmitry, > > I've checked with the HW team and looks like the math for eol_byte_nums > differs between DP and DSI. > > For DSI, eol_byte_num = bytes_per_intf % 3 > > But for DP, eol_byte_num = (bytes_per_intf % 3 == 0) ? 0 : 3 - > bytes_per_intf % 3 *only* for non-widebus. > > For DP && widebus enabled, eol_byte_num = (bytes_per_intf % 6 == 0) ? 0 > : 6 - bytes_per_intf % 6 > > In that case, we should move even the bytes_per_intf % 3 out and change > this method to msm_dsc_get_bytes_per_intf() instead. Thanks for the note. I'm looking forward to seeing the v3 then. > > Thanks, > > Jessica Zhang
On 4/3/2023 5:33 PM, Dmitry Baryshkov wrote: > On 04/04/2023 00:38, Jessica Zhang wrote: >> >> >> On 4/2/2023 4:21 AM, Dmitry Baryshkov wrote: >>> On 31/03/2023 21:49, Jessica Zhang wrote: >>>> Introduce MSM-specific DSC helper methods, as some calculations are >>>> common between DP and DSC. >>>> >>>> Changes in v2: >>>> - Moved files up to msm/ directory >>>> - Dropped get_comp_ratio() helper >>>> - Used drm_int2fixp() to convert to integers to fp >>>> - Style changes to improve readability >>>> - Dropped unused bpp variable in msm_dsc_get_dce_bytes_per_line() >>>> - Changed msm_dsc_get_slice_per_intf() to a static inline method >>>> - Dropped last division step of msm_dsc_get_pclk_per_line() and changed >>>> method name accordingly >>>> - Changed DSC_BPP macro to drm_dsc_get_bpp_int() helper method >>>> - Fixed some math issues caused by passing in incorrect types to >>>> drm_fixed methods in get_bytes_per_soft_slice() >>>> >>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>>> --- >>>> drivers/gpu/drm/msm/Makefile | 1 + >>>> drivers/gpu/drm/msm/msm_dsc_helper.c | 53 >>>> ++++++++++++++++++++++++++++++++++++ >>>> drivers/gpu/drm/msm/msm_dsc_helper.h | 42 >>>> ++++++++++++++++++++++++++++ >>>> 3 files changed, 96 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/msm/Makefile >>>> b/drivers/gpu/drm/msm/Makefile >>>> index 7274c41228ed..b814fc80e2d5 100644 >>>> --- a/drivers/gpu/drm/msm/Makefile >>>> +++ b/drivers/gpu/drm/msm/Makefile >>>> @@ -94,6 +94,7 @@ msm-y += \ >>>> msm_atomic_tracepoints.o \ >>>> msm_debugfs.o \ >>>> msm_drv.o \ >>>> + msm_dsc_helper.o \ >>>> msm_fb.o \ >>>> msm_fence.o \ >>>> msm_gem.o \ >>>> diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.c >>>> b/drivers/gpu/drm/msm/msm_dsc_helper.c >>>> new file mode 100644 >>>> index 000000000000..60b73e17e6eb >>>> --- /dev/null >>>> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.c >>>> @@ -0,0 +1,53 @@ >>>> +// SPDX-License-Identifier: GPL-2.0-only >>>> +/* >>>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights >>>> reserved >>>> + */ >>>> + >>>> +#include <linux/kernel.h> >>>> +#include <linux/errno.h> >>>> +#include <drm/drm_fixed.h> >>>> + >>>> +#include "msm_drv.h" >>>> +#include "msm_dsc_helper.h" >>>> + >>>> +static s64 get_bytes_per_soft_slice(struct drm_dsc_config *dsc, int >>>> intf_width, u32 src_bpp) >>> >>> intf_width is unused >> >> Hi Dmitry, >> >> Acked. >> >>> >>>> +{ >>>> + int bpp = msm_dsc_get_bpp_int(dsc); >>>> + s64 numerator_fp, denominator_fp; >>>> + s64 comp_ratio_fp = drm_fixp_from_fraction(src_bpp, bpp); >>>> + >>>> + numerator_fp = drm_int2fixp(dsc->slice_width * 3); >>> >>> You have lost dsc->bits_per_component here. >> >> This was moved to the denominator calculation, but I'll move it back >> to this line to avoid confusion. > > Maybe you occasionally mixed bpp and bpc, because there is no > bits_per_component usage in denominator. Could you please recheck the > calculations. Hey Dmitry, Sorry, forgot to respond to this comment in my earlier reply. This was a mistake in the v2 code -- should have used dsc->bits_per_component instead of bpp and did not catch it during validation as bpp == bpc for the panel I'm testing on. Will use bits_per_compnent going forward. Thanks, Jessica Zhang > >> >>> >>>> + denominator_fp = drm_fixp_from_fraction(comp_ratio_fp * 8, >>>> drm_int2fixp(bpp)); >>> >>> denominator_fp = drm_fixp_from_fraction(src_bpp * 8, bpp); >> >> Acked. >> >>> >>>> + >>>> + return drm_fixp_div(numerator_fp, denominator_fp); >>>> +} >>>> + >>>> +u32 msm_dsc_get_eol_byte_num(struct drm_dsc_config *dsc, int >>>> intf_width, u32 src_bpp) >>>> +{ >>>> + u32 bytes_per_soft_slice, extra_eol_bytes, bytes_per_intf; >>>> + s64 bytes_per_soft_slice_fp; >>>> + int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width); >>>> + >>>> + bytes_per_soft_slice_fp = get_bytes_per_soft_slice(dsc, >>>> intf_width, src_bpp); >>>> + bytes_per_soft_slice = drm_fixp2int_ceil(bytes_per_soft_slice_fp); >>>> + >>>> + bytes_per_intf = bytes_per_soft_slice * slice_per_intf; >>>> + extra_eol_bytes = bytes_per_intf % 3; >>>> + if (extra_eol_bytes != 0) >>>> + extra_eol_bytes = 3 - extra_eol_bytes; >>> >>> I become confused here when I checked eol_bytes in the display techpack. >>> >>> I see that for DP the dp_panel_dsc_pclk_param_calc() calculates >>> dsc->eol_bytes_num in this way, the size to pad dsc_byte_count * >>> slice_per_intf to 3 bytes. >>> >>> However, for DSI this is a simple as total_bytes_per_intf % 3 , so it >>> is not a padding, but a length of the last chunk. >>> >>> Could you please clarify? If the techpack code is correct, I'd prefer >>> if we return last chunk size here and calculate the padding length in >>> the DP driver. >> >> I've double checked the calculations between DP and DSI, and I think >> you're right. Will move the `if (extra_eol_bytes != 0)` block out to >> DP code. > > Ack. Could you please check with HW team that our understanding is correct? > >> >>> >>>> + >>>> + return extra_eol_bytes; >>>> +} >>>> + >>>> +int msm_dsc_get_uncompressed_pclk_per_line(struct drm_dsc_config >>>> *dsc, int intf_width, u32 src_bpp) >>> >>> Basing on Abhinav's description ("pclk_per_line can be only per >>> interface") would it better be named as >>> msm_dsc_get_uncompressed_pclk_per_intf() ? or >>> msm_dsc_get_uncompressed_pclk_for_intf() ? >>> >>> BTW: if get_bytes_per_soft_slice() doesn't use intf_width, we can >>> probably drop it here too. >>> >>>> +{ >>>> + s64 data_width; >>>> + >>>> + if (!dsc->slice_width || (intf_width < dsc->slice_width)) >>>> + return -EINVAL; >>> >>> Error code is not validated at dsi_timing_setup. I'd suggest moving >>> error checks there and dropping the error handling here. If >>> dsc->slice_width is not set, we should stop much earlier than >>> drm_bridge's pre_enable() callback. >> >> Acked. >> >> Thanks, >> >> Jessica Zhang >> >>> >>>> + >>>> + data_width = drm_fixp_mul(dsc->slice_count, >>>> + get_bytes_per_soft_slice(dsc, intf_width, src_bpp)); >>>> + >>>> + return drm_fixp2int_ceil(data_width); >>>> +} >>>> 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..743cd324b7d9 >>>> --- /dev/null >>>> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.h >>>> @@ -0,0 +1,42 @@ >>>> +/* 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 <drm/display/drm_dsc_helper.h> >>>> +#include <drm/drm_modes.h> >>>> + >>>> +/* >>>> + * Helper methods for MSM specific DSC calculations that are common >>>> between timing engine, >>>> + * DSI, and DP. >>>> + */ >>>> + >>>> +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; >>>> +} >>>> + >>>> +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); >>>> +} >>>> + >>>> +static inline u32 msm_dsc_get_dce_bytes_per_line(struct >>>> drm_dsc_config *dsc, int intf_width) >>>> +{ >>>> + return DIV_ROUND_UP(msm_dsc_get_bpp_int(dsc) * intf_width, 8); >>>> +} >>>> + >>>> +u32 msm_dsc_get_eol_byte_num(struct drm_dsc_config *dsc, int >>>> intf_width, u32 src_bpp); >>>> +u32 msm_dsc_get_dce_bytes_per_line(struct drm_dsc_config *dsc, int >>>> intf_width); >>>> + >>>> +/* Calculate uncompressed pclk per line. This value will then be >>>> passed along to >>>> + * DSI and DP to calculate pclk_per_line. This is because DSI and >>>> DP divide the >>>> + * uncompressed pclk_per_line by different values depending on if >>>> widebus is enabled. >>>> + */ >>>> +int msm_dsc_get_uncompressed_pclk_per_line(struct drm_dsc_config *dsc, >>>> + int intf_width, u32 src_bpp); >>>> +#endif /* MSM_DSC_HELPER_H_ */ >>>> >>> >>> -- >>> With best wishes >>> Dmitry >>> > > -- > With best wishes > Dmitry >
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile index 7274c41228ed..b814fc80e2d5 100644 --- a/drivers/gpu/drm/msm/Makefile +++ b/drivers/gpu/drm/msm/Makefile @@ -94,6 +94,7 @@ msm-y += \ msm_atomic_tracepoints.o \ msm_debugfs.o \ msm_drv.o \ + msm_dsc_helper.o \ msm_fb.o \ msm_fence.o \ msm_gem.o \ diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.c b/drivers/gpu/drm/msm/msm_dsc_helper.c new file mode 100644 index 000000000000..60b73e17e6eb --- /dev/null +++ b/drivers/gpu/drm/msm/msm_dsc_helper.c @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved + */ + +#include <linux/kernel.h> +#include <linux/errno.h> +#include <drm/drm_fixed.h> + +#include "msm_drv.h" +#include "msm_dsc_helper.h" + +static s64 get_bytes_per_soft_slice(struct drm_dsc_config *dsc, int intf_width, u32 src_bpp) +{ + int bpp = msm_dsc_get_bpp_int(dsc); + s64 numerator_fp, denominator_fp; + s64 comp_ratio_fp = drm_fixp_from_fraction(src_bpp, bpp); + + numerator_fp = drm_int2fixp(dsc->slice_width * 3); + denominator_fp = drm_fixp_from_fraction(comp_ratio_fp * 8, drm_int2fixp(bpp)); + + return drm_fixp_div(numerator_fp, denominator_fp); +} + +u32 msm_dsc_get_eol_byte_num(struct drm_dsc_config *dsc, int intf_width, u32 src_bpp) +{ + u32 bytes_per_soft_slice, extra_eol_bytes, bytes_per_intf; + s64 bytes_per_soft_slice_fp; + int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width); + + bytes_per_soft_slice_fp = get_bytes_per_soft_slice(dsc, intf_width, src_bpp); + bytes_per_soft_slice = drm_fixp2int_ceil(bytes_per_soft_slice_fp); + + bytes_per_intf = bytes_per_soft_slice * slice_per_intf; + extra_eol_bytes = bytes_per_intf % 3; + if (extra_eol_bytes != 0) + extra_eol_bytes = 3 - extra_eol_bytes; + + return extra_eol_bytes; +} + +int msm_dsc_get_uncompressed_pclk_per_line(struct drm_dsc_config *dsc, int intf_width, u32 src_bpp) +{ + s64 data_width; + + if (!dsc->slice_width || (intf_width < dsc->slice_width)) + return -EINVAL; + + data_width = drm_fixp_mul(dsc->slice_count, + get_bytes_per_soft_slice(dsc, intf_width, src_bpp)); + + return drm_fixp2int_ceil(data_width); +} 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..743cd324b7d9 --- /dev/null +++ b/drivers/gpu/drm/msm/msm_dsc_helper.h @@ -0,0 +1,42 @@ +/* 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 <drm/display/drm_dsc_helper.h> +#include <drm/drm_modes.h> + +/* + * Helper methods for MSM specific DSC calculations that are common between timing engine, + * DSI, and DP. + */ + +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; +} + +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); +} + +static inline u32 msm_dsc_get_dce_bytes_per_line(struct drm_dsc_config *dsc, int intf_width) +{ + return DIV_ROUND_UP(msm_dsc_get_bpp_int(dsc) * intf_width, 8); +} + +u32 msm_dsc_get_eol_byte_num(struct drm_dsc_config *dsc, int intf_width, u32 src_bpp); +u32 msm_dsc_get_dce_bytes_per_line(struct drm_dsc_config *dsc, int intf_width); + +/* Calculate uncompressed pclk per line. This value will then be passed along to + * DSI and DP to calculate pclk_per_line. This is because DSI and DP divide the + * uncompressed pclk_per_line by different values depending on if widebus is enabled. + */ +int msm_dsc_get_uncompressed_pclk_per_line(struct drm_dsc_config *dsc, + int intf_width, u32 src_bpp); +#endif /* MSM_DSC_HELPER_H_ */
Introduce MSM-specific DSC helper methods, as some calculations are common between DP and DSC. Changes in v2: - Moved files up to msm/ directory - Dropped get_comp_ratio() helper - Used drm_int2fixp() to convert to integers to fp - Style changes to improve readability - Dropped unused bpp variable in msm_dsc_get_dce_bytes_per_line() - Changed msm_dsc_get_slice_per_intf() to a static inline method - Dropped last division step of msm_dsc_get_pclk_per_line() and changed method name accordingly - Changed DSC_BPP macro to drm_dsc_get_bpp_int() helper method - Fixed some math issues caused by passing in incorrect types to drm_fixed methods in get_bytes_per_soft_slice() Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> --- drivers/gpu/drm/msm/Makefile | 1 + drivers/gpu/drm/msm/msm_dsc_helper.c | 53 ++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/msm/msm_dsc_helper.h | 42 ++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+)