Re: [Freedreno] [PATCH v7 4/8] drm/msm: Add MSM-specific DSC helper methods
On 2023-05-11 13:05:15, Abhinav Kumar wrote: > > Don't think the DP series alone should point that out, as it heavily > > depends on the relation between slice size and bpp parameters, and > > whether those end up with a fractional part or not (are you able to test > > and confirm all those combinations?). Regardless it seems more natural > > to use slice_chunk_size which is used in various other ways and sent in > > the PPS: it wouldn't make sense to adhere to a different slice byte > > count elsewhere. > > > > They are not totally different. > > I guess what Jessica is trying to say here is we will have to do more > validation with slice/bpp combinations for DP to conclude whether there > is a difference in math. We can go with two approaches: > > 1) Either go with slice_chunk_size for now and re-validate with the > monitors we have and drop this particular helper > 2) Keep this particular helper for now and upon more validation if we > see it's same across more combinations, drop this later. I would argue that if we need a different rounding strategy on the number of bytes within a slice, shouldn't the DRM helper calculating slice_chunk_size be updated as well? This sounds/feels like a thing that is specified in the DSC docs, as all the parameters involved are part of the DSC spec (but I am not currently in a position to review that within the coming few days). > I just have a slight preference for (2), but looks like your preference > is for (1) but this is just more validation work for us which is fine I > guess this time since we will revalidate DP again anyway. Thanks, that's appreciated, but do make sure to test the cases where the rounding method actually makes a difference in the resulting value. > >> I also want to note that this math has stayed the same throughout all 7 > >> revisions. In the interest of making review more efficient, I think it > >> would be helpful to point out important details like this early on in > >> the process. That way we can address major concerns early on and keep > >> the number of revisions per series low. > > > > I've already expressed to Abhinav and you that the revisions for all > > these series were coming in way too hot, leaving no time for review and > > discussions before the next revision hits the list. If you want to keep > > the number of revisions low, v8 shouln't have already been sent. > > > > You had the concern for DSC 1.2 DPU series from kuogee. So to respect > that we held that back the entire end of last week and early this week > (~6 days) and posted today. > > Now, you have the same concern with this series from jessica. Sorry, as > much as i would like to get your feedback on every series, I cannot hold > back every QC display developer to wait for your reviews on patch rev 1 > before posting patch rev 2. They all work mostly independently. So if > one reviewer say dmitry has reviewed one revision and we agreed on the > comments, in the absence of another comment from another reviewer, the > developer is going to post the next revision for more reviews as its > more efficient to manage their time as they are in the same context. > This is nothing unusual from normal upstream development. I am not saying that it is unusual to send many revisions (though in "quick succession", which is only loosely defined). Just that it is unusual to "demand" reviews (which are seemingly perceived as requesting a lot, even though I don't think I'm asking anything outrageous here) to stop when the revisions reach a certain number, or to have happened early on in the series. I understand it's annoying from a developer perspective, but sometimes it is what it is. This whole process would be better if there's a more explicit list of "these specific people have to test, review and sign off on every patch" before this series is ready to be merged, but I understand that people do not want to commit to that from both sides. > If by the time you start reviewing its going to be revision 7 or 8, then > start it as a fresh review which it is for you, folks are obviously > going to question why you didnt review revisions 1-7 so far. So I cannot > take sides on this. Fine to question it, and my answer is: besides having a hard time finding enough free time to look at this, I also want to test it by piecing together all the various series, and besides regression-testing on sdm845 even managed to use these series in a very positive way to finally (albeit with some additional fixes) get working DSC and panels on my SM8150 and SM8250 boards. Meaning I have even more hardware and configurations to test, valudate, **and provide feedback** on, taking away from the review time initially. > I wish I could help more to accommodate your schedules but its hard to > control who pushes when with distributed development and tell each of > them to hold back. > > > I desire to review *and test* all these patches (which I believe is in > >
Re: [Freedreno] [PATCH v7 4/8] drm/msm: Add MSM-specific DSC helper methods
On 5/10/2023 11:15 PM, Marijn Suijten wrote:
On 2023-05-10 14:03:14, Jessica Zhang wrote:
On 5/9/2023 11:33 PM, Marijn Suijten wrote:
On 2023-05-09 15:06:50, Jessica Zhang wrote:
Introduce MSM-specific DSC helper methods, as some calculations are
common between DP and DSC.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/msm/Makefile | 1 +
drivers/gpu/drm/msm/msm_dsc_helper.c | 26 ++
drivers/gpu/drm/msm/msm_dsc_helper.h | 69
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 ..29feb3e3b5a4
--- /dev/null
+++ b/drivers/gpu/drm/msm/msm_dsc_helper.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved
+ */
+
+#include
+#include
+
+#include "msm_dsc_helper.h"
+
+s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc)
+{
+ return drm_fixp_from_fraction(dsc->slice_width *
msm_dsc_get_bpp_int(dsc), 8);
How about using dsc->slice_chunk_size?
Hi Marijn,
Thanks for pointing this out. However, I would prefer to keep this fixed
point version of the slice_chunk_size math as the downstream DP math
also uses fixed point [1].
That looks like a totally different computation. For this specific
value it appears drm_fixp_from_fraction does rounding whereas
DIV_ROUND_UP always rounds up when used for slice_chunk_size, but these
values are generally multiples of 8. slice_chunk_size will also take
fractional bpp into account (even if unsupported by DPU otherwise).
If we are able to confirm that integer math also works for DP, we will
make the change to use slice_chunk_size within the DP DSC series.
Don't think the DP series alone should point that out, as it heavily
depends on the relation between slice size and bpp parameters, and
whether those end up with a fractional part or not (are you able to test
and confirm all those combinations?). Regardless it seems more natural
to use slice_chunk_size which is used in various other ways and sent in
the PPS: it wouldn't make sense to adhere to a different slice byte
count elsewhere.
They are not totally different.
I guess what Jessica is trying to say here is we will have to do more
validation with slice/bpp combinations for DP to conclude whether there
is a difference in math. We can go with two approaches:
1) Either go with slice_chunk_size for now and re-validate with the
monitors we have and drop this particular helper
2) Keep this particular helper for now and upon more validation if we
see it's same across more combinations, drop this later.
I just have a slight preference for (2), but looks like your preference
is for (1) but this is just more validation work for us which is fine I
guess this time since we will revalidate DP again anyway.
I also want to note that this math has stayed the same throughout all 7
revisions. In the interest of making review more efficient, I think it
would be helpful to point out important details like this early on in
the process. That way we can address major concerns early on and keep
the number of revisions per series low.
I've already expressed to Abhinav and you that the revisions for all
these series were coming in way too hot, leaving no time for review and
discussions before the next revision hits the list. If you want to keep
the number of revisions low, v8 shouln't have already been sent.
You had the concern for DSC 1.2 DPU series from kuogee. So to respect
that we held that back the entire end of last week and early this week
(~6 days) and posted today.
Now, you have the same concern with this series from jessica. Sorry, as
much as i would like to get your feedback on every series, I cannot hold
back every QC display developer to wait for your reviews on patch rev 1
before posting patch rev 2. They all work mostly independently. So if
one reviewer say dmitry has reviewed one revision and we agreed on the
comments, in the absence of another comment from another reviewer, the
developer is going to post the next revision for more reviews as its
more efficient to manage their time as they are in the same context.
This is nothing unusual from normal upstream development.
If by the time you start reviewing its going to be revision 7 or 8, then
start it as a fresh review which it is for you, folks are obviously
going to question why you didnt review revisions 1-7 so far. So I cannot
take sides
Re: [Freedreno] [PATCH v7 4/8] drm/msm: Add MSM-specific DSC helper methods
On 5/10/2023 11:28 PM, Dmitry Baryshkov wrote:
On 11/05/2023 00:03, Jessica Zhang wrote:
On 5/9/2023 11:33 PM, Marijn Suijten wrote:
On 2023-05-09 15:06:50, Jessica Zhang wrote:
Introduce MSM-specific DSC helper methods, as some calculations are
common between DP and DSC.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/msm/Makefile | 1 +
drivers/gpu/drm/msm/msm_dsc_helper.c | 26 ++
drivers/gpu/drm/msm/msm_dsc_helper.h | 69
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 ..29feb3e3b5a4
--- /dev/null
+++ b/drivers/gpu/drm/msm/msm_dsc_helper.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights
reserved
+ */
+
+#include
+#include
+
+#include "msm_dsc_helper.h"
+
+s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc)
+{
+ return drm_fixp_from_fraction(dsc->slice_width *
msm_dsc_get_bpp_int(dsc), 8);
How about using dsc->slice_chunk_size?
Hi Marijn,
Thanks for pointing this out. However, I would prefer to keep this
fixed point version of the slice_chunk_size math as the downstream DP
math also uses fixed point [1].
This is pretty weak argument. Especially since this particular piece of
code does lots of wrong or inefficient things.
I guess Jessica's concern was we will have to revalidate some bpc/bpp
and slice combinations again for DP. So its a question of keeping this
helper for now and dropping it if unnecessary later or dropping it now
itself. If the consensus is going to be to drop this, we will just
re-test the monitors we have and do it.
If we are able to confirm that integer math also works for DP, we will
make the change to use slice_chunk_size within the DP DSC series.
This is why we usually do not accept API-only series. It is next to
imposible to judge if the API is good enough without the actual users.
I also want to note that this math has stayed the same throughout all
7 revisions. In the interest of making review more efficient, I think
it would be helpful to point out important details like this early on
in the process. That way we can address major concerns early on and
keep the number of revisions per series low.
This is not always possible. We grasp the details of the patchset as we
review and dive into the patchset under the review and other close
enough patches/commits. So it is infrequent but still valid when at some
point a reviewer (or the author) would come up with the comments
demanding significant changes to the patch.
I dont think Jessica is really complaining about multiple reviewers. Its
a question of when those reviews are coming. Not reviewing revisions 1-7
and starting the reviews at revision 8 (that essentially becomes
revision 1 now) , is going to make this go on forever.
So its a bit frustrating from the developers point of view to just
ignore revisions 1-7 and start at rev 8.
[1]
https://github.com/ianmacd/gts6lwifi/blob/master/drivers/gpu/drm/msm/dp/dp_panel.c#L335
+}
+
+u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int
intf_width)
+{
+ u32 bytes_per_soft_slice;
+ s64 bytes_per_soft_slice_fp;
+ int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width);
+
+ bytes_per_soft_slice_fp = msm_dsc_get_bytes_per_soft_slice(dsc);
+ bytes_per_soft_slice = drm_fixp2int_ceil(bytes_per_soft_slice_fp);
+
+ return bytes_per_soft_slice * slice_per_intf;
+}
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 ..38f3651d0b79
--- /dev/null
+++ b/drivers/gpu/drm/msm/msm_dsc_helper.h
@@ -0,0 +1,69 @@
+/* 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
+#include
+#include
+
+/*
+ * 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
For all function docs, don't forget the trailing parenthesis after the
function name: msm_dsc_get_bpp_int()
https://www.kernel.org/doc/html/next/doc-guide/kernel-doc.html#function-documentation
Acked.
+ * @dsc: Pointer to drm dsc config struct
+ * Returns: BPP integer value
+ */
+static inline int msm_dsc_get_bpp_int(struct
Re: [Freedreno] [PATCH v7 4/8] drm/msm: Add MSM-specific DSC helper methods
On 11/05/2023 00:03, Jessica Zhang wrote:
On 5/9/2023 11:33 PM, Marijn Suijten wrote:
On 2023-05-09 15:06:50, Jessica Zhang wrote:
Introduce MSM-specific DSC helper methods, as some calculations are
common between DP and DSC.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/msm/Makefile | 1 +
drivers/gpu/drm/msm/msm_dsc_helper.c | 26 ++
drivers/gpu/drm/msm/msm_dsc_helper.h | 69
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 ..29feb3e3b5a4
--- /dev/null
+++ b/drivers/gpu/drm/msm/msm_dsc_helper.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights
reserved
+ */
+
+#include
+#include
+
+#include "msm_dsc_helper.h"
+
+s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc)
+{
+ return drm_fixp_from_fraction(dsc->slice_width *
msm_dsc_get_bpp_int(dsc), 8);
How about using dsc->slice_chunk_size?
Hi Marijn,
Thanks for pointing this out. However, I would prefer to keep this fixed
point version of the slice_chunk_size math as the downstream DP math
also uses fixed point [1].
This is pretty weak argument. Especially since this particular piece of
code does lots of wrong or inefficient things.
If we are able to confirm that integer math also works for DP, we will
make the change to use slice_chunk_size within the DP DSC series.
This is why we usually do not accept API-only series. It is next to
imposible to judge if the API is good enough without the actual users.
I also want to note that this math has stayed the same throughout all 7
revisions. In the interest of making review more efficient, I think it
would be helpful to point out important details like this early on in
the process. That way we can address major concerns early on and keep
the number of revisions per series low.
This is not always possible. We grasp the details of the patchset as we
review and dive into the patchset under the review and other close
enough patches/commits. So it is infrequent but still valid when at some
point a reviewer (or the author) would come up with the comments
demanding significant changes to the patch.
[1]
https://github.com/ianmacd/gts6lwifi/blob/master/drivers/gpu/drm/msm/dp/dp_panel.c#L335
+}
+
+u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int
intf_width)
+{
+ u32 bytes_per_soft_slice;
+ s64 bytes_per_soft_slice_fp;
+ int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width);
+
+ bytes_per_soft_slice_fp = msm_dsc_get_bytes_per_soft_slice(dsc);
+ bytes_per_soft_slice = drm_fixp2int_ceil(bytes_per_soft_slice_fp);
+
+ return bytes_per_soft_slice * slice_per_intf;
+}
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 ..38f3651d0b79
--- /dev/null
+++ b/drivers/gpu/drm/msm/msm_dsc_helper.h
@@ -0,0 +1,69 @@
+/* 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
+#include
+#include
+
+/*
+ * 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
For all function docs, don't forget the trailing parenthesis after the
function name: msm_dsc_get_bpp_int()
https://www.kernel.org/doc/html/next/doc-guide/kernel-doc.html#function-documentation
Acked.
+ * @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);
Looks good.
+}
+
+/**
+ * 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
Re: [Freedreno] [PATCH v7 4/8] drm/msm: Add MSM-specific DSC helper methods
On 2023-05-10 14:03:14, Jessica Zhang wrote:
>
>
> On 5/9/2023 11:33 PM, Marijn Suijten wrote:
> > On 2023-05-09 15:06:50, Jessica Zhang wrote:
> >> Introduce MSM-specific DSC helper methods, as some calculations are
> >> common between DP and DSC.
> >>
> >> Reviewed-by: Dmitry Baryshkov
> >> Signed-off-by: Jessica Zhang
> >> ---
> >> drivers/gpu/drm/msm/Makefile | 1 +
> >> drivers/gpu/drm/msm/msm_dsc_helper.c | 26 ++
> >> drivers/gpu/drm/msm/msm_dsc_helper.h | 69
> >>
> >> 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 ..29feb3e3b5a4
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.c
> >> @@ -0,0 +1,26 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved
> >> + */
> >> +
> >> +#include
> >> +#include
> >> +
> >> +#include "msm_dsc_helper.h"
> >> +
> >> +s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc)
> >> +{
> >> + return drm_fixp_from_fraction(dsc->slice_width *
> >> msm_dsc_get_bpp_int(dsc), 8);
> >
> > How about using dsc->slice_chunk_size?
>
> Hi Marijn,
>
> Thanks for pointing this out. However, I would prefer to keep this fixed
> point version of the slice_chunk_size math as the downstream DP math
> also uses fixed point [1].
That looks like a totally different computation. For this specific
value it appears drm_fixp_from_fraction does rounding whereas
DIV_ROUND_UP always rounds up when used for slice_chunk_size, but these
values are generally multiples of 8. slice_chunk_size will also take
fractional bpp into account (even if unsupported by DPU otherwise).
> If we are able to confirm that integer math also works for DP, we will
> make the change to use slice_chunk_size within the DP DSC series.
Don't think the DP series alone should point that out, as it heavily
depends on the relation between slice size and bpp parameters, and
whether those end up with a fractional part or not (are you able to test
and confirm all those combinations?). Regardless it seems more natural
to use slice_chunk_size which is used in various other ways and sent in
the PPS: it wouldn't make sense to adhere to a different slice byte
count elsewhere.
> I also want to note that this math has stayed the same throughout all 7
> revisions. In the interest of making review more efficient, I think it
> would be helpful to point out important details like this early on in
> the process. That way we can address major concerns early on and keep
> the number of revisions per series low.
I've already expressed to Abhinav and you that the revisions for all
these series were coming in way too hot, leaving no time for review and
discussions before the next revision hits the list. If you want to keep
the number of revisions low, v8 shouln't have already been sent.
I desire to review *and test* all these patches (which I believe is in
everyone's best interest) and have not initially been able to do so as
all these efforts come out of my free time, which is sometimes limited
of has been used to finalize the INTF TE series (which is a dependency
of these series).
It should be pretty obvious to see that this patch has not had a reply
from me on any of the previous revisions, and there are more patches
that I have not been able to comment on yet.
The alternative to that is of course not seeing enough value in my
review and testing (or ""late"" comments...) and merging it regardless
at some point, but that's not up to me to decide.
- Marijn
>
> [1]
> https://github.com/ianmacd/gts6lwifi/blob/master/drivers/gpu/drm/msm/dp/dp_panel.c#L335
>
> >
> >> +}
> >> +
> >> +u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int intf_width)
> >> +{
> >> + u32 bytes_per_soft_slice;
> >> + s64 bytes_per_soft_slice_fp;
> >> + int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width);
> >> +
> >> + bytes_per_soft_slice_fp = msm_dsc_get_bytes_per_soft_slice(dsc);
> >> + bytes_per_soft_slice = drm_fixp2int_ceil(bytes_per_soft_slice_fp);
> >> +
> >> + return bytes_per_soft_slice * slice_per_intf;
> >> +}
> >> 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 ..38f3651d0b79
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.h
> >> @@ -0,0 +1,69 @@
> >> +/* SPDX-License-Identifier:
Re: [Freedreno] [PATCH v7 4/8] drm/msm: Add MSM-specific DSC helper methods
On 5/9/2023 11:33 PM, Marijn Suijten wrote:
On 2023-05-09 15:06:50, Jessica Zhang wrote:
Introduce MSM-specific DSC helper methods, as some calculations are
common between DP and DSC.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/msm/Makefile | 1 +
drivers/gpu/drm/msm/msm_dsc_helper.c | 26 ++
drivers/gpu/drm/msm/msm_dsc_helper.h | 69
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 ..29feb3e3b5a4
--- /dev/null
+++ b/drivers/gpu/drm/msm/msm_dsc_helper.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved
+ */
+
+#include
+#include
+
+#include "msm_dsc_helper.h"
+
+s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc)
+{
+ return drm_fixp_from_fraction(dsc->slice_width *
msm_dsc_get_bpp_int(dsc), 8);
How about using dsc->slice_chunk_size?
Hi Marijn,
Thanks for pointing this out. However, I would prefer to keep this fixed
point version of the slice_chunk_size math as the downstream DP math
also uses fixed point [1].
If we are able to confirm that integer math also works for DP, we will
make the change to use slice_chunk_size within the DP DSC series.
I also want to note that this math has stayed the same throughout all 7
revisions. In the interest of making review more efficient, I think it
would be helpful to point out important details like this early on in
the process. That way we can address major concerns early on and keep
the number of revisions per series low.
[1]
https://github.com/ianmacd/gts6lwifi/blob/master/drivers/gpu/drm/msm/dp/dp_panel.c#L335
+}
+
+u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int intf_width)
+{
+ u32 bytes_per_soft_slice;
+ s64 bytes_per_soft_slice_fp;
+ int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width);
+
+ bytes_per_soft_slice_fp = msm_dsc_get_bytes_per_soft_slice(dsc);
+ bytes_per_soft_slice = drm_fixp2int_ceil(bytes_per_soft_slice_fp);
+
+ return bytes_per_soft_slice * slice_per_intf;
+}
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 ..38f3651d0b79
--- /dev/null
+++ b/drivers/gpu/drm/msm/msm_dsc_helper.h
@@ -0,0 +1,69 @@
+/* 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
+#include
+#include
+
+/*
+ * 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
For all function docs, don't forget the trailing parenthesis after the
function name: msm_dsc_get_bpp_int()
https://www.kernel.org/doc/html/next/doc-guide/kernel-doc.html#function-documentation
Acked.
+ * @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);
Looks good.
+}
+
+/**
+ * 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 DIV_ROUND_UP(dsc->slice_width * dsc->slice_count *
msm_dsc_get_bpp_int(dsc), 8);
dsc->slice_chunk_size * dsc->slice_count?
Acked.
+}
+
+/**
+ * msm_dsc_get_bytes_per_soft_slice - get size of each soft slice for dsc
Explain to the reader what a "soft" slice is?
A soft slice is a slice defined in software as opposed to "hard slices"
that are defined by hardware.
Since the slice-related
Re: [Freedreno] [PATCH v7 4/8] drm/msm: Add MSM-specific DSC helper methods
On 2023-05-09 15:06:50, Jessica Zhang wrote:
> Introduce MSM-specific DSC helper methods, as some calculations are
> common between DP and DSC.
>
> Reviewed-by: Dmitry Baryshkov
> Signed-off-by: Jessica Zhang
> ---
> drivers/gpu/drm/msm/Makefile | 1 +
> drivers/gpu/drm/msm/msm_dsc_helper.c | 26 ++
> drivers/gpu/drm/msm/msm_dsc_helper.h | 69
>
> 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 ..29feb3e3b5a4
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.c
> @@ -0,0 +1,26 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved
> + */
> +
> +#include
> +#include
> +
> +#include "msm_dsc_helper.h"
> +
> +s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc)
> +{
> + return drm_fixp_from_fraction(dsc->slice_width *
> msm_dsc_get_bpp_int(dsc), 8);
How about using dsc->slice_chunk_size?
> +}
> +
> +u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int intf_width)
> +{
> + u32 bytes_per_soft_slice;
> + s64 bytes_per_soft_slice_fp;
> + int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width);
> +
> + bytes_per_soft_slice_fp = msm_dsc_get_bytes_per_soft_slice(dsc);
> + bytes_per_soft_slice = drm_fixp2int_ceil(bytes_per_soft_slice_fp);
> +
> + return bytes_per_soft_slice * slice_per_intf;
> +}
> 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 ..38f3651d0b79
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/msm_dsc_helper.h
> @@ -0,0 +1,69 @@
> +/* 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
> +#include
> +#include
> +
> +/*
> + * 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
For all function docs, don't forget the trailing parenthesis after the
function name: msm_dsc_get_bpp_int()
https://www.kernel.org/doc/html/next/doc-guide/kernel-doc.html#function-documentation
> + * @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);
Looks good.
> +}
> +
> +/**
> + * 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 DIV_ROUND_UP(dsc->slice_width * dsc->slice_count *
> msm_dsc_get_bpp_int(dsc), 8);
dsc->slice_chunk_size * dsc->slice_count?
> +}
> +
> +/**
> + * msm_dsc_get_bytes_per_soft_slice - get size of each soft slice for dsc
Explain to the reader what a "soft" slice is?
- Marijn
> + * @dsc: Pointer to drm dsc config struct
> + * Returns: s31.32 fixed point value representing bytes per soft slice
> + */
> +s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc);
> +
> +/**
> + * 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
> + */
> +u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int intf_width);
> +
> +#endif /* MSM_DSC_HELPER_H_ */
>
> --
> 2.40.1
>
[Freedreno] [PATCH v7 4/8] drm/msm: Add MSM-specific DSC helper methods
Introduce MSM-specific DSC helper methods, as some calculations are
common between DP and DSC.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/msm/Makefile | 1 +
drivers/gpu/drm/msm/msm_dsc_helper.c | 26 ++
drivers/gpu/drm/msm/msm_dsc_helper.h | 69
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 ..29feb3e3b5a4
--- /dev/null
+++ b/drivers/gpu/drm/msm/msm_dsc_helper.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved
+ */
+
+#include
+#include
+
+#include "msm_dsc_helper.h"
+
+s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc)
+{
+ return drm_fixp_from_fraction(dsc->slice_width *
msm_dsc_get_bpp_int(dsc), 8);
+}
+
+u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int intf_width)
+{
+ u32 bytes_per_soft_slice;
+ s64 bytes_per_soft_slice_fp;
+ int slice_per_intf = msm_dsc_get_slice_per_intf(dsc, intf_width);
+
+ bytes_per_soft_slice_fp = msm_dsc_get_bytes_per_soft_slice(dsc);
+ bytes_per_soft_slice = drm_fixp2int_ceil(bytes_per_soft_slice_fp);
+
+ return bytes_per_soft_slice * slice_per_intf;
+}
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 ..38f3651d0b79
--- /dev/null
+++ b/drivers/gpu/drm/msm/msm_dsc_helper.h
@@ -0,0 +1,69 @@
+/* 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
+#include
+#include
+
+/*
+ * 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 DIV_ROUND_UP(dsc->slice_width * dsc->slice_count *
msm_dsc_get_bpp_int(dsc), 8);
+}
+
+/**
+ * msm_dsc_get_bytes_per_soft_slice - get size of each soft slice for dsc
+ * @dsc: Pointer to drm dsc config struct
+ * Returns: s31.32 fixed point value representing bytes per soft slice
+ */
+s64 msm_dsc_get_bytes_per_soft_slice(struct drm_dsc_config *dsc);
+
+/**
+ * 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
+ */
+u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int intf_width);
+
+#endif /* MSM_DSC_HELPER_H_ */
--
2.40.1
