Re: [Freedreno] [PATCH v1 1/2] drm/msm/dp: enable HDP plugin/unplugged interrupts to hpd_enable/disable
On Fri, 12 May 2023 at 03:16, Kuogee Hsieh wrote:
>
>
> On 5/11/2023 8:57 AM, Dmitry Baryshkov wrote:
> > On 11/05/2023 18:53, Bjorn Andersson wrote:
> >> On Thu, May 11, 2023 at 07:24:46AM +0300, Dmitry Baryshkov wrote:
> >>> On Wed, 10 May 2023 at 23:31, Kuogee Hsieh
> >>> wrote:
>
> The internal_hpd flag was introduced to handle external DP HPD
> derived from GPIO
> pinmuxed into DP controller. HPD plug/unplug interrupts cannot be
> enabled until
> internal_hpd flag is set to true.
> At both bootup and resume time, the DP driver will enable external DP
> plugin interrupts and handle plugin interrupt accordingly.
> Unfortunately
> dp_bridge_hpd_enable() bridge ops function was called to set
> internal_hpd
> flag to true later than where DP driver expected during bootup time.
>
> This causes external DP plugin event to not get detected and
> display stays blank.
> Move enabling HDP plugin/unplugged interrupts to
> dp_bridge_hpd_enable()/disable() to
> set internal_hpd to true along with enabling HPD plugin/unplugged
> interrupts
> simultaneously to avoid timing issue during bootup and resume.
>
> Fixes: cd198caddea7 ("drm/msm/dp: Rely on hpd_enable/disable
> callbacks")
> Signed-off-by: Kuogee Hsieh
> >>>
> >>> Thanks for debugging this!
> >>>
> >>> However after looking at the driver I think there is more than this.
> >>>
> >>> We have several other places gated on internal_hpd flag, where we do
> >>> not have a strict ordering of events.
> >>> I see that dp_hpd_plug_handle() and dp_hpd_unplug_handle() also toggle
> >>> DP_DP_IRQ_HPD_INT_MASK and DP_DP_HPD_REPLUG_INT_MASK depending on
> >>> internal_hpd. Can we toggle all 4 interrupts from the
> >>> hpd_enable/hpd_disable functions? If we can do it, then I think we can
> >>> drop the internal_hpd flag completely.
> >>>
> >>
> >> Yes, that's what I believe the DRM framework intend us to do.
> >>
> >> The problem, and reason why I didn't do tat in my series, was that in
> >> order to update the INT_MASKs you need to clock the IP-block and that's
> >> done elsewhere.
> >>
> >> So, for the internal_hpd case, it seems appropriate to pm_runtime_get()
> >> in hpd_enable() and unmask the HPD interrupts, and mask the interrupts
> >> and pm_runtime_put() in hpd_disable().
> >>
> >>
> >> But for edp and external HPD-signal we also need to make sure power is
> >> on while something is connected...
> >
> > I think this is already handled by the existing code, see calls to the
> > dp_display_host_init().
> >
> >>
> >>> I went on and checked other places where it is used:
> >>> - dp_hpd_unplug_handle(), guarding DP_DP_HPD_PLUG_INT_MASK toggling. I
> >>> think we can drop these two calls completely. The function is under
> >>> the event_mutex protection, so other events can not interfere.
> >>> - dp_bridge_hpd_notify(). What is the point of this check? If some
> >>> other party informs us of the HPD event, we'd better handle it instead
> >>> of dropping it. Correct? In other words, I'd prefer seeing the
> >>> hpd_event_thread removal. Instead of that I think that on
> >>> HPD/plug/unplug/etc. IRQ the driver should call into the drm stack,
> >>> then the hpd_notify call should process those events.
> >>>
> 1) DP with GPIO: No downstream drm_bridge are connected, is_edp = false
> and internal HPD-logic is in used (internal_hpd = true). Power needs to
> be on at all times etc.
>
> 2) DP without GPIO: Downstream drm_bridge connected, is_edp = false and
> internal HPD-logic should not be used/enabled (internal_hpd = false).
> Power doesn't need to be on unless hpd_notify is invoked to tell us that
> there's something connected...
>
> - dp_bridge_hpd_notify(). What is the point of this check? <== i have
> below two questions,
>
> 1) can you explain when/what this dp_bridge_hpd_notify() will be called?
The call chain is drm_bridge_hpd_notify() ->
drm_bridge_connector_hpd_notify() -> .hpd_notify() for all drm_bridge
in chain
One should add a call to drm_bridge_hpd_notify() when the hotplug
event has been detected.
Also please note the patch https://patchwork.freedesktop.org/patch/484432/
>
> 2) is dp_bridge_hpd_notify() only will be called at above case #2? and
> it will not be used by case #1?
Once the driver calls drm_bridge_hpd_notify() in the hpd path, the
hpd_notify callbacks will be called in case#1 too.
BTW: I don't see drm_bridge_hpd_notify() or
drm_kms_{,connector_}_hotplug_event() HPD notifications in the DP
driver at all. This should be fixed.
>
>
>
> >>
> >> I agree, that seems to be what's expected of us from the DRM framework.
> >>
> >> Regards,
> >> Bjorn
> >>
> >>>
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 27 ++-
> 1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index
Re: [Freedreno] [PATCH v1 1/2] drm/msm/dp: enable HDP plugin/unplugged interrupts to hpd_enable/disable
On 5/11/2023 8:57 AM, Dmitry Baryshkov wrote:
On 11/05/2023 18:53, Bjorn Andersson wrote:
On Thu, May 11, 2023 at 07:24:46AM +0300, Dmitry Baryshkov wrote:
On Wed, 10 May 2023 at 23:31, Kuogee Hsieh
wrote:
The internal_hpd flag was introduced to handle external DP HPD
derived from GPIO
pinmuxed into DP controller. HPD plug/unplug interrupts cannot be
enabled until
internal_hpd flag is set to true.
At both bootup and resume time, the DP driver will enable external DP
plugin interrupts and handle plugin interrupt accordingly.
Unfortunately
dp_bridge_hpd_enable() bridge ops function was called to set
internal_hpd
flag to true later than where DP driver expected during bootup time.
This causes external DP plugin event to not get detected and
display stays blank.
Move enabling HDP plugin/unplugged interrupts to
dp_bridge_hpd_enable()/disable() to
set internal_hpd to true along with enabling HPD plugin/unplugged
interrupts
simultaneously to avoid timing issue during bootup and resume.
Fixes: cd198caddea7 ("drm/msm/dp: Rely on hpd_enable/disable
callbacks")
Signed-off-by: Kuogee Hsieh
Thanks for debugging this!
However after looking at the driver I think there is more than this.
We have several other places gated on internal_hpd flag, where we do
not have a strict ordering of events.
I see that dp_hpd_plug_handle() and dp_hpd_unplug_handle() also toggle
DP_DP_IRQ_HPD_INT_MASK and DP_DP_HPD_REPLUG_INT_MASK depending on
internal_hpd. Can we toggle all 4 interrupts from the
hpd_enable/hpd_disable functions? If we can do it, then I think we can
drop the internal_hpd flag completely.
Yes, that's what I believe the DRM framework intend us to do.
The problem, and reason why I didn't do tat in my series, was that in
order to update the INT_MASKs you need to clock the IP-block and that's
done elsewhere.
So, for the internal_hpd case, it seems appropriate to pm_runtime_get()
in hpd_enable() and unmask the HPD interrupts, and mask the interrupts
and pm_runtime_put() in hpd_disable().
But for edp and external HPD-signal we also need to make sure power is
on while something is connected...
I think this is already handled by the existing code, see calls to the
dp_display_host_init().
I went on and checked other places where it is used:
- dp_hpd_unplug_handle(), guarding DP_DP_HPD_PLUG_INT_MASK toggling. I
think we can drop these two calls completely. The function is under
the event_mutex protection, so other events can not interfere.
- dp_bridge_hpd_notify(). What is the point of this check? If some
other party informs us of the HPD event, we'd better handle it instead
of dropping it. Correct? In other words, I'd prefer seeing the
hpd_event_thread removal. Instead of that I think that on
HPD/plug/unplug/etc. IRQ the driver should call into the drm stack,
then the hpd_notify call should process those events.
1) DP with GPIO: No downstream drm_bridge are connected, is_edp = false
and internal HPD-logic is in used (internal_hpd = true). Power needs to
be on at all times etc.
2) DP without GPIO: Downstream drm_bridge connected, is_edp = false and
internal HPD-logic should not be used/enabled (internal_hpd = false).
Power doesn't need to be on unless hpd_notify is invoked to tell us that
there's something connected...
- dp_bridge_hpd_notify(). What is the point of this check? <== i have
below two questions,
1) can you explain when/what this dp_bridge_hpd_notify() will be called?
2) is dp_bridge_hpd_notify() only will be called at above case #2? and
it will not be used by case #1?
I agree, that seems to be what's expected of us from the DRM framework.
Regards,
Bjorn
---
drivers/gpu/drm/msm/dp/dp_display.c | 27 ++-
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 3e13acdf..71aa944 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1088,13 +1088,6 @@ static void dp_display_config_hpd(struct
dp_display_private *dp)
dp_display_host_init(dp);
dp_catalog_ctrl_hpd_config(dp->catalog);
- /* Enable plug and unplug interrupts only if requested */
- if (dp->dp_display.internal_hpd)
- dp_catalog_hpd_config_intr(dp->catalog,
- DP_DP_HPD_PLUG_INT_MASK |
- DP_DP_HPD_UNPLUG_INT_MASK,
- true);
-
/* Enable interrupt first time
* we are leaving dp clocks on during disconnect
* and never disable interrupt
@@ -1396,12 +1389,6 @@ static int dp_pm_resume(struct device *dev)
dp_catalog_ctrl_hpd_config(dp->catalog);
- if (dp->dp_display.internal_hpd)
- dp_catalog_hpd_config_intr(dp->catalog,
- DP_DP_HPD_PLUG_INT_MASK |
- DP_DP_HPD_UNPLUG_INT_MASK,
-
Re: [Freedreno] [PATCH v7 2/8] drm/display/dsc: add helper to set semi-const parameters
On 2023-05-11 09:22:47, Dmitry Baryshkov wrote: > On 11/05/2023 09:18, Marijn Suijten wrote: > > On 2023-05-11 07:26:28, Dmitry Baryshkov wrote: > >> On 11/05/2023 01:35, Jessica Zhang wrote: > >>> > >>> > >>> On 5/9/2023 11:29 PM, Marijn Suijten wrote: > On 2023-05-09 15:06:48, Jessica Zhang wrote: > > From: Dmitry Baryshkov > > > > Add a helper setting config values which are typically constant across > > operating modes (table E-4 of the standard) and mux_word_size (which is > > a const according to 3.5.2). > > > > Signed-off-by: Dmitry Baryshkov > > Signed-off-by: Jessica Zhang > > Same question about ordering. > >>> > >>> Hi Marijn, > >>> > >>> This patch was authored by Dmitry and originally part of his DRM DSC > >>> helpers series [1], but was removed from that series for mergeability > >>> reasons. > >>> > >>> Looking over the kernel documentation, the last Signed-off-by should be > >>> from the patch submitter [2], so I think my s-o-b tag should be at the > >>> bottom. > > > > That's true, but I also think the S-o-B at the top should match the > > From: author. > > I'd say, this is usual, but not the required order of tags. > > > > >>> As for the order in the previous patch, I can add a duplicate s-o-b > >>> before Dmitry's so that it reflects the history of the patch. > >> > >> I think this is an overkill. Instead you can drop my SOB from the patch > >> 1. We do not need this level of detail. > >> > >> For this patch the ordering of tags is correct. > > > > So indeed, that either means duplicating the S-o-B or dropping it > > entirely as we do not care that it was part of that series earlier. > > Dmitry will likely sign this off once again when picking the patches. > > Yes. > > I'd suggest the following tags: > > Patch 1 (Add flatness...): > From: Jessica > SOB: Jessica > > Patches 2 (add helper to set) and 3 (use DRM DSC helpers): > From: Dmitry > SOB: Dmitry > SOB: Jessica Both sound exactly right, as we do not care about that fact that the first patch was temporarily picked up by you in another series, and then dropped again. - Marijn
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 v1 1/2] drm/msm/dp: enable HDP plugin/unplugged interrupts to hpd_enable/disable
Quoting Bjorn Andersson (2023-05-11 08:44:16) > On Wed, May 10, 2023 at 05:39:07PM -0700, Abhinav Kumar wrote: > > > > > > On 5/10/2023 4:55 PM, Stephen Boyd wrote: > > > Quoting Kuogee Hsieh (2023-05-10 13:31:04) > > > > The internal_hpd flag was introduced to handle external DP HPD derived > > > > from GPIO > > > > pinmuxed into DP controller. > > > > > > Was it? It looks more like it was done to differentiate between eDP and > > > DP, because internal_hpd is set only if DRM_BRIDGE_OP_HPD is set on the > > > bridge and we only set the bridge op if the connector type is DP. The > > > assumption looks like if you have DP connector_type, you have the gpio > > > pinmuxed for "dp_hot" mode, which isn't exactly true. We don't treat > > > that gpio as an irq either, because it isn't. Instead the gpio is muxed > > > to the mdss inside the SoC and then that generates an mdss interrupt > > > that's combined with non-HPD things like "video ready". > > > > > > If that all follows, then I don't quite understand why we're setting > > > internal_hpd to false at all at runtime. It should be set to true at > > > some point, but ideally that point is during probe. > > > > > > > Kuogee had the same thought originally but were not entirely sure of this > > part of the commit message in Bjorn's original commit which introduced these > > changes. > > > > "This difference is not appropriately represented by the "is_edp" > > boolean, but is properly represented by the frameworks invocation of the > > hpd_enable() and hpd_disable() callbacks. Switch the current condition > > to rely on these callbacks instead" > > > > Does this along with below documentation mean we should generate the hpd > > interrupts only after hpd_enable callback happens? > > > > " * Call _bridge_funcs.hpd_enable if implemented and register the given > > @cb > > * and @data as hot plug notification callback. From now on the @cb will be > > * called with @data when an output status change is detected by the bridge, > > * until hot plug notification gets disabled with drm_bridge_hpd_disable(). > > " > > > > Bjorn, can you please clarify this? > > > > We currently have 3 cases: > > 1) DP with GPIO: No downstream drm_bridge are connected, is_edp = false > and internal HPD-logic is in used (internal_hpd = true). Power needs to > be on at all times etc. > > 2) DP without GPIO: Downstream drm_bridge connected, is_edp = false and > internal HPD-logic should not be used/enabled (internal_hpd = false). > Power doesn't need to be on unless hpd_notify is invoked to tell us that > there's something connected... > > 3) eDP with or without HPD signal and/or HPD gpio. Downstream > drm_bridge/panel is connected, is_edp = true and internal HPD logic is > short-circuited regardless of the panel providing HPD signal or not. Oh weird. I thought that the "is_edp" controller on sc7280 didn't have HPD hardware in the PHY (phy@aec2a00), hence all the logic to avoid using the HPD interrupts and bits there. What is "is_edp" about then? > > > In #1 dp_bridge_hpd_enable() will be invoked to indicate that the DP > controller is expected to perform HPD handling. In #2 > dp_bridge_hpd_enable() will _not_ be invoked, instead some downstream > drm_bridge/panel will get the hpd_enable() callback and will be > responsible to updating the HPD state of the chain, which will cause > hpd_notify to be invoked. > > > Note that #3 is based entirely on the controller, it has currently no > relation to what is attached. It seems reasonable that this is just > another case of #2 (perhaps just always reporting > connector_status_connected?). > Looking at drm_bridge_connector_detect() the default is to consider DRM_MODE_CONNECTOR_eDP as connector_status_connected. I wonder if panel_bridge_bridge_funcs can gain a 'detect' function and also set DRM_BRIDGE_OP_DETECT if the connector_type is DRM_MODE_CONNECTOR_eDP.
[Freedreno] [PATCH v7 5/8] drm/msm/dpu: add support for DSC encoder v1.2 engine
Add support for DSC 1.2 by providing the necessary hooks to program
the DPU DSC 1.2 encoder.
Changes in v3:
-- fixed kernel test rebot report that "__iomem *off" is declared but not
used at dpu_hw_dsc_config_1_2()
-- unrolling thresh loops
Changes in v4:
-- delete DPU_DSC_HW_REV_1_1
-- delete off and used real register name directly
Changes in v7:
-- replace offset with sblk->enc.base
-- replace ss with slice
Reported-by: kernel test robot
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/Makefile | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 32 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h | 14 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c | 383 +
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 7 +-
5 files changed, 433 insertions(+), 4 deletions(-)
create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index b814fc8..b9af5e4 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -65,6 +65,7 @@ msm-$(CONFIG_DRM_MSM_DPU) += \
disp/dpu1/dpu_hw_catalog.o \
disp/dpu1/dpu_hw_ctl.o \
disp/dpu1/dpu_hw_dsc.o \
+ disp/dpu1/dpu_hw_dsc_1_2.o \
disp/dpu1/dpu_hw_interrupts.o \
disp/dpu1/dpu_hw_intf.o \
disp/dpu1/dpu_hw_lm.o \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index dc0a4da..4eda2cc 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -1,6 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0-only */
/*
- * Copyright (c) 2022. Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2022-2023, Qualcomm Innovation Center, Inc. All rights
reserved.
* Copyright (c) 2015-2018, 2020 The Linux Foundation. All rights reserved.
*/
@@ -244,12 +244,18 @@ enum {
};
/**
- * DSC features
+ * DSC sub-blocks/features
* @DPU_DSC_OUTPUT_CTRL Configure which PINGPONG block gets
*the pixel output from this DSC.
+ * @DPU_DSC_HW_REV_1_2DSC block supports dsc 1.1 and 1.2
+ * @DPU_DSC_NATIVE_422_EN Supports native422 and native420 encoding
+ * @DPU_DSC_MAX
*/
enum {
DPU_DSC_OUTPUT_CTRL = 0x1,
+ DPU_DSC_HW_REV_1_2,
+ DPU_DSC_NATIVE_422_EN,
+ DPU_DSC_MAX
};
/**
@@ -306,6 +312,14 @@ struct dpu_pp_blk {
};
/**
+ * struct dpu_dsc_blk - DSC Encoder sub-blk information
+ * @info: HW register and features supported by this sub-blk
+ */
+struct dpu_dsc_blk {
+ DPU_HW_SUBBLK_INFO;
+};
+
+/**
* enum dpu_qos_lut_usage - define QoS LUT use cases
*/
enum dpu_qos_lut_usage {
@@ -452,6 +466,17 @@ struct dpu_pingpong_sub_blks {
};
/**
+ * struct dpu_dsc_sub_blks - DSC sub-blks
+ * @enc: DSC encoder sub block
+ * @ctl: DSC controller sub block
+ *
+ */
+struct dpu_dsc_sub_blks {
+ struct dpu_dsc_blk enc;
+ struct dpu_dsc_blk ctl;
+};
+
+/**
* dpu_clk_ctrl_type - Defines top level clock control signals
*/
enum dpu_clk_ctrl_type {
@@ -605,10 +630,13 @@ struct dpu_merge_3d_cfg {
* struct dpu_dsc_cfg - information of DSC blocks
* @id enum identifying this block
* @base register offset of this block
+ * @len: length of hardware block
* @features bit mask identifying sub-blocks/features
+ * @sblk sub-blocks information
*/
struct dpu_dsc_cfg {
DPU_HW_BLK_INFO;
+ const struct dpu_dsc_sub_blks *sblk;
};
/**
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
index 138080a..bdff74d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
@@ -1,5 +1,8 @@
/* SPDX-License-Identifier: GPL-2.0-only */
-/* Copyright (c) 2020-2022, Linaro Limited */
+/*
+ * Copyright (c) 2020-2022, Linaro Limited
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved
+ */
#ifndef _DPU_HW_DSC_H
#define _DPU_HW_DSC_H
@@ -69,6 +72,15 @@ struct dpu_hw_dsc *dpu_hw_dsc_init(const struct dpu_dsc_cfg
*cfg,
void __iomem *addr);
/**
+ * dpu_hw_dsc_init_1_2 - initializes the v1.2 DSC hw driver block
+ * @cfg: DSC catalog entry for which driver object is required
+ * @addr: Mapped register io address of MDP
+ * Returns: Error code or allocated dpu_hw_dsc context
+ */
+struct dpu_hw_dsc *dpu_hw_dsc_init_1_2(const struct dpu_dsc_cfg *cfg,
+ void __iomem *addr);
+
+/**
* dpu_hw_dsc_destroy - destroys dsc driver context
* @dsc: Pointer to dsc driver context returned by dpu_hw_dsc_init
*/
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
new file mode 100644
index ..dd77d03
--- /dev/null
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
@@ -0,0 +1,383 @@
+//
[Freedreno] [PATCH v7 6/8] drm/msm/dpu: separate DSC flush update out of interface
Current DSC flush update is piggyback inside dpu_hw_ctl_intf_cfg_v1().
This patch separates DSC flush away from dpu_hw_ctl_intf_cfg_v1() by
adding dpu_hw_ctl_update_pending_flush_dsc_v1() to handle both per
DSC engine and DSC flush bits at same time to make it consistent with
the location of flush programming of other dpu sub blocks.
Signed-off-by: Kuogee Hsieh
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 --
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 22 --
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 10 ++
3 files changed, 38 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index ffa6f04..94b805b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1834,12 +1834,18 @@ dpu_encoder_dsc_initial_line_calc(struct drm_dsc_config
*dsc,
return DIV_ROUND_UP(total_pixels, dsc->slice_width);
}
-static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc,
+static void dpu_encoder_dsc_pipe_cfg(struct dpu_encoder_virt *dpu_enc,
+struct dpu_hw_dsc *hw_dsc,
struct dpu_hw_pingpong *hw_pp,
struct drm_dsc_config *dsc,
u32 common_mode,
u32 initial_lines)
{
+ struct dpu_encoder_phys *cur_master = dpu_enc->cur_master;
+ struct dpu_hw_ctl *ctl;
+
+ ctl = cur_master->hw_ctl;
+
if (hw_dsc->ops.dsc_config)
hw_dsc->ops.dsc_config(hw_dsc, dsc, common_mode, initial_lines);
@@ -1854,6 +1860,9 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc
*hw_dsc,
if (hw_pp->ops.enable_dsc)
hw_pp->ops.enable_dsc(hw_pp);
+
+ if (ctl->ops.update_pending_flush_dsc)
+ ctl->ops.update_pending_flush_dsc(ctl, hw_dsc->idx);
}
static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc,
@@ -1898,7 +1907,8 @@ static void dpu_encoder_prep_dsc(struct dpu_encoder_virt
*dpu_enc,
initial_lines = dpu_encoder_dsc_initial_line_calc(dsc, enc_ip_w);
for (i = 0; i < MAX_CHANNELS_PER_ENC; i++)
- dpu_encoder_dsc_pipe_cfg(hw_dsc[i], hw_pp[i], dsc,
dsc_common_mode, initial_lines);
+ dpu_encoder_dsc_pipe_cfg(dpu_enc, hw_dsc[i], hw_pp[i], dsc,
+ dsc_common_mode, initial_lines);
}
void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
index 4f7cfa9..832a6a7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
@@ -139,6 +139,11 @@ static inline void dpu_hw_ctl_trigger_flush_v1(struct
dpu_hw_ctl *ctx)
CTL_DSPP_n_FLUSH(dspp - DSPP_0),
ctx->pending_dspp_flush_mask[dspp - DSPP_0]);
}
+
+ if (ctx->pending_flush_mask & BIT(DSC_IDX))
+ DPU_REG_WRITE(>hw, CTL_DSC_FLUSH,
+ ctx->pending_dsc_flush_mask);
+
DPU_REG_WRITE(>hw, CTL_FLUSH, ctx->pending_flush_mask);
}
@@ -285,6 +290,13 @@ static void
dpu_hw_ctl_update_pending_flush_merge_3d_v1(struct dpu_hw_ctl *ctx,
ctx->pending_flush_mask |= BIT(MERGE_3D_IDX);
}
+static void dpu_hw_ctl_update_pending_flush_dsc_v1(struct dpu_hw_ctl *ctx,
+ enum dpu_dsc dsc_num)
+{
+ ctx->pending_dsc_flush_mask |= BIT(dsc_num - DSC_0);
+ ctx->pending_flush_mask |= BIT(DSC_IDX);
+}
+
static void dpu_hw_ctl_update_pending_flush_dspp(struct dpu_hw_ctl *ctx,
enum dpu_dspp dspp, u32 dspp_sub_blk)
{
@@ -502,9 +514,6 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx,
if ((test_bit(DPU_CTL_VM_CFG, >caps->features)))
mode_sel = CTL_DEFAULT_GROUP_ID << 28;
- if (cfg->dsc)
- DPU_REG_WRITE(>hw, CTL_DSC_FLUSH, cfg->dsc);
-
if (cfg->intf_mode_sel == DPU_CTL_MODE_SEL_CMD)
mode_sel |= BIT(17);
@@ -524,10 +533,8 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx,
if (cfg->merge_3d)
DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE,
BIT(cfg->merge_3d - MERGE_3D_0));
- if (cfg->dsc) {
- DPU_REG_WRITE(>hw, CTL_FLUSH, DSC_IDX);
+ if (cfg->dsc)
DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
- }
}
static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
@@ -630,6 +637,9 @@ static void _setup_ctl_ops(struct dpu_hw_ctl_ops *ops,
ops->update_pending_flush_merge_3d =
dpu_hw_ctl_update_pending_flush_merge_3d_v1;
ops->update_pending_flush_wb =
dpu_hw_ctl_update_pending_flush_wb_v1;
+
[Freedreno] [PATCH v7 8/8] drm/msm/dpu: tear down DSC data path when DSC disabled
Unset DSC_ACTIVE bit at dpu_hw_ctl_reset_intf_cfg_v1(),
dpu_encoder_unprep_dsc() and dpu_encoder_dsc_pipe_clr() functions
to tear down DSC data path if DSC data path was setup previous.
Signed-off-by: Kuogee Hsieh
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 44 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +
2 files changed, 51 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 94b805b..6500589 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1214,6 +1214,45 @@ static void dpu_encoder_virt_atomic_enable(struct
drm_encoder *drm_enc,
mutex_unlock(_enc->enc_lock);
}
+static void dpu_encoder_dsc_pipe_clr(struct dpu_encoder_virt *dpu_enc,
+ struct dpu_hw_dsc *hw_dsc,
+ struct dpu_hw_pingpong *hw_pp)
+{
+ struct dpu_encoder_phys *cur_master = dpu_enc->cur_master;
+ struct dpu_hw_ctl *ctl;
+
+ ctl = cur_master->hw_ctl;
+
+ if (hw_dsc->ops.dsc_disable)
+ hw_dsc->ops.dsc_disable(hw_dsc);
+
+ if (hw_pp->ops.disable_dsc)
+ hw_pp->ops.disable_dsc(hw_pp);
+
+ if (hw_dsc->ops.dsc_bind_pingpong_blk)
+ hw_dsc->ops.dsc_bind_pingpong_blk(hw_dsc, PINGPONG_NONE);
+
+ if (ctl->ops.update_pending_flush_dsc)
+ ctl->ops.update_pending_flush_dsc(ctl, hw_dsc->idx);
+}
+
+static void dpu_encoder_unprep_dsc(struct dpu_encoder_virt *dpu_enc)
+{
+ /* coding only for 2LM, 2enc, 1 dsc config */
+ struct dpu_hw_dsc *hw_dsc[MAX_CHANNELS_PER_ENC];
+ struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC];
+ int i;
+
+ for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
+ hw_pp[i] = dpu_enc->hw_pp[i];
+ hw_dsc[i] = dpu_enc->hw_dsc[i];
+
+ if (hw_pp[i] && hw_dsc[i])
+ dpu_encoder_dsc_pipe_clr(dpu_enc, hw_dsc[i], hw_pp[i]);
+ }
+
+}
+
static void dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc,
struct drm_atomic_state *state)
{
@@ -2090,6 +2129,9 @@ void dpu_encoder_helper_phys_cleanup(struct
dpu_encoder_phys *phys_enc)
phys_enc->hw_pp->merge_3d->idx);
}
+ if (dpu_enc->dsc)
+ dpu_encoder_unprep_dsc(dpu_enc);
+
intf_cfg.stream_sel = 0; /* Don't care value for video mode */
intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
@@ -2101,6 +2143,8 @@ void dpu_encoder_helper_phys_cleanup(struct
dpu_encoder_phys *phys_enc)
if (phys_enc->hw_pp->merge_3d)
intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
+ intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
+
if (ctl->ops.reset_intf_cfg)
ctl->ops.reset_intf_cfg(ctl, _cfg);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
index 832a6a7..b34dac5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
@@ -577,6 +577,7 @@ static void dpu_hw_ctl_reset_intf_cfg_v1(struct dpu_hw_ctl
*ctx,
u32 intf_active = 0;
u32 wb_active = 0;
u32 merge3d_active = 0;
+ u32 dsc_active;
/*
* This API resets each portion of the CTL path namely,
@@ -606,6 +607,12 @@ static void dpu_hw_ctl_reset_intf_cfg_v1(struct dpu_hw_ctl
*ctx,
wb_active &= ~BIT(cfg->wb - WB_0);
DPU_REG_WRITE(c, CTL_WB_ACTIVE, wb_active);
}
+
+ if (cfg->dsc) {
+ dsc_active = DPU_REG_READ(c, CTL_DSC_ACTIVE);
+ dsc_active &= ~cfg->dsc;
+ DPU_REG_WRITE(c, CTL_DSC_ACTIVE, dsc_active);
+ }
}
static void dpu_hw_ctl_set_fetch_pipe_active(struct dpu_hw_ctl *ctx,
--
2.7.4
[Freedreno] [PATCH v7 7/8] drm/msm/dpu: add DSC 1.2 hw blocks for relevant chipsets
From: Abhinav Kumar
Add DSC 1.2 hardware blocks to the catalog with necessary sub-block and
feature flag information. Each display compression engine (DCE) contains
dual hard slice DSC encoders so both share same base address but with
its own different sub block address.
changes in v4:
-- delete DPU_DSC_HW_REV_1_1
-- re arrange sc8280xp_dsc[]
Signed-off-by: Abhinav Kumar
Signed-off-by: Kuogee Hsieh
Reviewed-by: Dmitry Baryshkov
---
.../gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h | 14
.../gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h | 7 ++
.../drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 16 ++
.../gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h | 14
.../gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 14
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 25 +-
6 files changed, 89 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
index 500cfd0..c4c93c8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
@@ -153,6 +153,18 @@ static const struct dpu_merge_3d_cfg sm8350_merge_3d[] = {
MERGE_3D_BLK("merge_3d_2", MERGE_3D_2, 0x5),
};
+/*
+ * NOTE: Each display compression engine (DCE) contains dual hard
+ * slice DSC encoders so both share same base address but with
+ * its own different sub block address.
+ */
+static const struct dpu_dsc_cfg sm8350_dsc[] = {
+ DSC_BLK_1_2("dce_0", DSC_0, 0x8, 0x100, 0, dsc_sblk_0),
+ DSC_BLK_1_2("dce_0", DSC_1, 0x8, 0x100, 0, dsc_sblk_1),
+ DSC_BLK_1_2("dce_1", DSC_2, 0x81000, 0x100, BIT(DPU_DSC_NATIVE_422_EN),
dsc_sblk_0),
+ DSC_BLK_1_2("dce_1", DSC_3, 0x81000, 0x100, BIT(DPU_DSC_NATIVE_422_EN),
dsc_sblk_1),
+};
+
static const struct dpu_intf_cfg sm8350_intf[] = {
INTF_BLK("intf_0", INTF_0, 0x34000, 0x280, INTF_DP,
MSM_DP_CONTROLLER_0, 24, INTF_SC7280_MASK,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 24),
@@ -215,6 +227,8 @@ const struct dpu_mdss_cfg dpu_sm8350_cfg = {
.dspp = sm8350_dspp,
.pingpong_count = ARRAY_SIZE(sm8350_pp),
.pingpong = sm8350_pp,
+ .dsc = sm8350_dsc,
+ .dsc_count = ARRAY_SIZE(sm8350_dsc),
.merge_3d_count = ARRAY_SIZE(sm8350_merge_3d),
.merge_3d = sm8350_merge_3d,
.intf_count = ARRAY_SIZE(sm8350_intf),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
index 5646713..42c66fe 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
@@ -93,6 +93,11 @@ static const struct dpu_pingpong_cfg sc7280_pp[] = {
PP_BLK_DITHER("pingpong_3", PINGPONG_3, 0x6c000, 0, sc7280_pp_sblk, -1,
-1),
};
+/* NOTE: sc7280 only has one dsc hard slice encoder */
+static const struct dpu_dsc_cfg sc7280_dsc[] = {
+ DSC_BLK_1_2("dce_0", DSC_0, 0x8, 0x100, BIT(DPU_DSC_NATIVE_422_EN),
dsc_sblk_0),
+};
+
static const struct dpu_intf_cfg sc7280_intf[] = {
INTF_BLK("intf_0", INTF_0, 0x34000, 0x280, INTF_DP,
MSM_DP_CONTROLLER_0, 24, INTF_SC7280_MASK,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 24),
@@ -149,6 +154,8 @@ const struct dpu_mdss_cfg dpu_sc7280_cfg = {
.mixer = sc7280_lm,
.pingpong_count = ARRAY_SIZE(sc7280_pp),
.pingpong = sc7280_pp,
+ .dsc_count = ARRAY_SIZE(sc7280_dsc),
+ .dsc = sc7280_dsc,
.intf_count = ARRAY_SIZE(sc7280_intf),
.intf = sc7280_intf,
.vbif_count = ARRAY_SIZE(sdm845_vbif),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
index 808aacd..508479f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
@@ -141,6 +141,20 @@ static const struct dpu_merge_3d_cfg sc8280xp_merge_3d[] =
{
MERGE_3D_BLK("merge_3d_2", MERGE_3D_2, 0x5),
};
+/*
+ * NOTE: Each display compression engine (DCE) contains dual hard
+ * slice DSC encoders so both share same base address but with
+ * its own different sub block address.
+ */
+static const struct dpu_dsc_cfg sc8280xp_dsc[] = {
+ DSC_BLK_1_2("dce_0", DSC_0, 0x8, 0x100, 0, dsc_sblk_0),
+ DSC_BLK_1_2("dce_0", DSC_1, 0x8, 0x100, 0, dsc_sblk_1),
+ DSC_BLK_1_2("dce_1", DSC_2, 0x81000, 0x100, BIT(DPU_DSC_NATIVE_422_EN),
dsc_sblk_0),
+ DSC_BLK_1_2("dce_1", DSC_3, 0x81000, 0x100, BIT(DPU_DSC_NATIVE_422_EN),
dsc_sblk_1),
+ DSC_BLK_1_2("dce_2", DSC_4, 0x82000, 0x100, 0, dsc_sblk_0),
+ DSC_BLK_1_2("dce_2", DSC_5, 0x82000, 0x100, 0, dsc_sblk_1),
+};
+
/* TODO: INTF 3, 8 and 7 are used for MST, marked as INTF_NONE for now */
static const struct dpu_intf_cfg
[Freedreno] [PATCH v7 2/8] drm/msm/dpu: add DPU_PINGPONG_DSC feature bit for DPU < 7.0.0
DPU < 7.0.0 requires the PINGPONG block to be involved during
DSC setting up. Since DPU >= 7.0.0, enabling and starting the DSC
encoder engine was moved to INTF with the help of the flush mechanism.
Add a DPU_PINGPONG_DSC feature bit to restrict the availability of
dpu_hw_pp_setup_dsc() and dpu_hw_pp_dsc_{enable,disable}() on the
PINGPONG block to DPU < 7.0.0 hardware, as the registers are not
available [in the PINGPONG block] on DPU 7.0.0 and higher anymore.
Add DPU_PINGPONG_DSC to PINGPONG_SDM845_MASK, PINGPONG_SDM845_TE2_MASK
and PINGPONG_SM8150_MASK which is used for all DPU < 7.0 chipsets.
changes in v6:
-- split patches and rearrange to keep catalog related files at this patch
changes in v7:
-- rewording commit text as suggested at review comments
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 6 +++---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 4 +++-
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 82b58c6..78e4bf6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -76,13 +76,13 @@
(BIT(DPU_DIM_LAYER) | BIT(DPU_MIXER_COMBINED_ALPHA))
#define PINGPONG_SDM845_MASK \
- (BIT(DPU_PINGPONG_DITHER) | BIT(DPU_PINGPONG_TE))
+ (BIT(DPU_PINGPONG_DITHER) | BIT(DPU_PINGPONG_TE) |
BIT(DPU_PINGPONG_DSC))
#define PINGPONG_SDM845_TE2_MASK \
- (PINGPONG_SDM845_MASK | BIT(DPU_PINGPONG_TE2))
+ (PINGPONG_SDM845_MASK | BIT(DPU_PINGPONG_TE2) | BIT(DPU_PINGPONG_DSC))
#define PINGPONG_SM8150_MASK \
- (BIT(DPU_PINGPONG_DITHER))
+ (BIT(DPU_PINGPONG_DITHER) | BIT(DPU_PINGPONG_DSC))
#define CTL_SC7280_MASK \
(BIT(DPU_CTL_ACTIVE_CFG) | \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index 6ee48f0..dc0a4da 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -144,7 +144,8 @@ enum {
* @DPU_PINGPONG_TE2Additional tear check block for split pipes
* @DPU_PINGPONG_SPLIT PP block supports split fifo
* @DPU_PINGPONG_SLAVE PP block is a suitable slave for split fifo
- * @DPU_PINGPONG_DITHER,Dither blocks
+ * @DPU_PINGPONG_DITHER Dither blocks
+ * @DPU_PINGPONG_DSCPP ops functions required for DSC
* @DPU_PINGPONG_MAX
*/
enum {
@@ -153,6 +154,7 @@ enum {
DPU_PINGPONG_SPLIT,
DPU_PINGPONG_SLAVE,
DPU_PINGPONG_DITHER,
+ DPU_PINGPONG_DSC,
DPU_PINGPONG_MAX
};
--
2.7.4
[Freedreno] [PATCH v7 4/8] drm/msm/dpu: Introduce PINGPONG_NONE to disconnect DSC from PINGPONG
Disabling the crossbar mux between DSC and PINGPONG currently
requires a bogus enum dpu_pingpong value to be passed when calling
dsc_bind_pingpong_blk() with enable=false, even though the register
value written is independent of the current PINGPONG block. Replace
that `bool enable` parameter with a new PINGPONG_NONE dpu_pingpong
flag that triggers the write of the "special" 0xF "crossbar
disabled" value to the register instead.
Changes in v4:
-- more details to commit text
Changes in v5:
-- rewording commit text suggested by Marijn
-- add DRM_DEBUG_KMS for DSC unbinding case
Signed-off-by: Kuogee Hsieh
Reviewed-by: Dmitry Baryshkov
Reviewed-by: Marijn Suijten
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 15 ---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h | 1 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 3 ++-
4 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index cf1de5d..ffa6f04 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1850,7 +1850,7 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc
*hw_dsc,
hw_pp->ops.setup_dsc(hw_pp);
if (hw_dsc->ops.dsc_bind_pingpong_blk)
- hw_dsc->ops.dsc_bind_pingpong_blk(hw_dsc, true, hw_pp->idx);
+ hw_dsc->ops.dsc_bind_pingpong_blk(hw_dsc, hw_pp->idx);
if (hw_pp->ops.enable_dsc)
hw_pp->ops.enable_dsc(hw_pp);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
index 4a6bbcc..47cb9f3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
@@ -157,7 +157,6 @@ static void dpu_hw_dsc_config_thresh(struct dpu_hw_dsc
*hw_dsc,
static void dpu_hw_dsc_bind_pingpong_blk(
struct dpu_hw_dsc *hw_dsc,
- bool enable,
const enum dpu_pingpong pp)
{
struct dpu_hw_blk_reg_map *c = _dsc->hw;
@@ -166,14 +165,16 @@ static void dpu_hw_dsc_bind_pingpong_blk(
dsc_ctl_offset = DSC_CTL(hw_dsc->idx);
- if (enable)
+ if (pp)
mux_cfg = (pp - PINGPONG_0) & 0x7;
- DRM_DEBUG_KMS("%s dsc:%d %s pp:%d\n",
- enable ? "Binding" : "Unbinding",
- hw_dsc->idx - DSC_0,
- enable ? "to" : "from",
- pp - PINGPONG_0);
+ if (pp)
+ DRM_DEBUG_KMS("Binding dsc:%d to pp:%d\n",
+ hw_dsc->idx - DSC_0,
+ pp - PINGPONG_0);
+ else
+ DRM_DEBUG_KMS("Unbinding dsc:%d from any pp\n",
+ hw_dsc->idx - DSC_0);
DPU_REG_WRITE(c, dsc_ctl_offset, mux_cfg);
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
index 287ec5f..138080a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
@@ -44,7 +44,6 @@ struct dpu_hw_dsc_ops {
struct drm_dsc_config *dsc);
void (*dsc_bind_pingpong_blk)(struct dpu_hw_dsc *hw_dsc,
- bool enable,
enum dpu_pingpong pp);
};
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
index 1913a19..02a0f48 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
@@ -191,7 +191,8 @@ enum dpu_dsc {
};
enum dpu_pingpong {
- PINGPONG_0 = 1,
+ PINGPONG_NONE,
+ PINGPONG_0,
PINGPONG_1,
PINGPONG_2,
PINGPONG_3,
--
2.7.4
[Freedreno] [PATCH v7 3/8] drm/msm/dpu: test DPU_PINGPONG_DSC bit before assign DSC ops to PINGPONG
DPU < 7.0.0 has DPU_PINGPONG_DSC feature bit set to indicate it requires
both dpu_hw_pp_setup_dsc() and dpu_hw_pp_dsc_{enable,disable}() to be
executed to complete DSC configuration if DSC hardware block is present.
Hence test DPU_PINGPONG_DSC feature bit and assign DSC related functions
to the ops of PINGPONG block accordingly if DPU_PINGPONG_DSC bit is set.
changes in v6:
-- split patches and keep the function file handles DPU_PINGPONG_DSC bit at
this patch
Signed-off-by: Kuogee Hsieh
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
index 79e4576..e7f47a4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
@@ -295,6 +295,12 @@ static void _setup_pingpong_ops(struct dpu_hw_pingpong *c,
c->ops.enable_dsc = dpu_hw_pp_dsc_enable;
c->ops.disable_dsc = dpu_hw_pp_dsc_disable;
+ if (test_bit(DPU_PINGPONG_DSC, )) {
+ c->ops.setup_dsc = dpu_hw_pp_setup_dsc;
+ c->ops.enable_dsc = dpu_hw_pp_dsc_enable;
+ c->ops.disable_dsc = dpu_hw_pp_dsc_disable;
+ }
+
if (test_bit(DPU_PINGPONG_DITHER, ))
c->ops.setup_dither = dpu_hw_pp_setup_dither;
};
--
2.7.4
[Freedreno] [PATCH v7 1/8] drm/msm/dpu: add dsc blocks for remaining chipsets in catalog
From: Abhinav Kumar
There are some platforms has DSC blocks but it is not declared at catalog.
For completeness, this patch adds DSC blocks for platforms which missed
them.
Signed-off-by: Abhinav Kumar
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 7 +++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h | 11 +++
2 files changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
index c0dd477..521cfd5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
@@ -126,6 +126,11 @@ static const struct dpu_pingpong_cfg msm8998_pp[] = {
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 15)),
};
+static const struct dpu_dsc_cfg msm8998_dsc[] = {
+ DSC_BLK("dsc_0", DSC_0, 0x8, 0),
+ DSC_BLK("dsc_1", DSC_1, 0x80400, 0),
+};
+
static const struct dpu_dspp_cfg msm8998_dspp[] = {
DSPP_BLK("dspp_0", DSPP_0, 0x54000, DSPP_MSM8998_MASK,
_dspp_sblk),
@@ -199,6 +204,8 @@ const struct dpu_mdss_cfg dpu_msm8998_cfg = {
.dspp = msm8998_dspp,
.pingpong_count = ARRAY_SIZE(msm8998_pp),
.pingpong = msm8998_pp,
+ .dsc_count = ARRAY_SIZE(msm8998_dsc),
+ .dsc = msm8998_dsc,
.intf_count = ARRAY_SIZE(msm8998_intf),
.intf = msm8998_intf,
.vbif_count = ARRAY_SIZE(msm8998_vbif),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
index e8057a1..fec1665 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
@@ -142,6 +142,15 @@ static const struct dpu_merge_3d_cfg sc8180x_merge_3d[] = {
MERGE_3D_BLK("merge_3d_2", MERGE_3D_2, 0x83200),
};
+static const struct dpu_dsc_cfg sc8180x_dsc[] = {
+ DSC_BLK("dsc_0", DSC_0, 0x8, BIT(DPU_DSC_OUTPUT_CTRL)),
+ DSC_BLK("dsc_1", DSC_1, 0x80400, BIT(DPU_DSC_OUTPUT_CTRL)),
+ DSC_BLK("dsc_2", DSC_2, 0x80800, BIT(DPU_DSC_OUTPUT_CTRL)),
+ DSC_BLK("dsc_3", DSC_3, 0x80c00, BIT(DPU_DSC_OUTPUT_CTRL)),
+ DSC_BLK("dsc_4", DSC_4, 0x81000, BIT(DPU_DSC_OUTPUT_CTRL)),
+ DSC_BLK("dsc_5", DSC_5, 0x81400, BIT(DPU_DSC_OUTPUT_CTRL)),
+};
+
static const struct dpu_intf_cfg sc8180x_intf[] = {
INTF_BLK("intf_0", INTF_0, 0x6a000, 0x280, INTF_DP,
MSM_DP_CONTROLLER_0, 24, INTF_SC7180_MASK,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 24),
@@ -206,6 +215,8 @@ const struct dpu_mdss_cfg dpu_sc8180x_cfg = {
.mixer = sc8180x_lm,
.pingpong_count = ARRAY_SIZE(sc8180x_pp),
.pingpong = sc8180x_pp,
+ .dsc_count = ARRAY_SIZE(sc8180x_dsc),
+ .dsc = sc8180x_dsc,
.merge_3d_count = ARRAY_SIZE(sc8180x_merge_3d),
.merge_3d = sc8180x_merge_3d,
.intf_count = ARRAY_SIZE(sc8180x_intf),
--
2.7.4
[Freedreno] [PATCH v7 0/8] add DSC 1.2 dpu supports
This series adds the DPU side changes to support DSC 1.2 encoder. This was validated with both DSI DSC 1.2 panel and DP DSC 1.2 monitor. The DSI and DP parts will be pushed later on top of this change. This seriel is rebase on [1], [2] and catalog fixes from rev-4 of [3]. [1]: https://patchwork.freedesktop.org/series/116851/ [2]: https://patchwork.freedesktop.org/series/116615/ [3]: https://patchwork.freedesktop.org/series/112332/ Abhinav Kumar (2): drm/msm/dpu: add dsc blocks for remaining chipsets in catalog drm/msm/dpu: add DSC 1.2 hw blocks for relevant chipsets Kuogee Hsieh (6): drm/msm/dpu: add DPU_PINGPONG_DSC feature bit for DPU < 7.0.0 drm/msm/dpu: test DPU_PINGPONG_DSC bit before assign DSC ops to PINGPONG drm/msm/dpu: Introduce PINGPONG_NONE to disconnect DSC from PINGPONG drm/msm/dpu: add support for DSC encoder v1.2 engine drm/msm/dpu: separate DSC flush update out of interface drm/msm/dpu: tear down DSC data path when DSC disabled drivers/gpu/drm/msm/Makefile | 1 + .../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h| 7 + .../drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h| 11 + .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h | 14 + .../gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h | 7 + .../drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 16 + .../gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h | 14 + .../gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 14 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 60 +++- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 31 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 36 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 29 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 10 + drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 15 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h | 15 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c | 383 + drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h| 3 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c| 6 + drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 7 +- 19 files changed, 652 insertions(+), 27 deletions(-) create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c -- 2.7.4
Re: [Freedreno] [PATCH v6 5/8] drm/msm/dpu: add support for DSC encoder v1.2 engine
On 11/05/2023 20:00, Kuogee Hsieh wrote:
On 5/10/2023 9:52 PM, Dmitry Baryshkov wrote:
On 11/05/2023 01:07, Kuogee Hsieh wrote:
Add support for DSC 1.2 by providing the necessary hooks to program
the DPU DSC 1.2 encoder.
Changes in v3:
-- fixed kernel test rebot report that "__iomem *off" is declared but
not
used at dpu_hw_dsc_config_1_2()
-- unrolling thresh loops
Changes in v4:
-- delete DPU_DSC_HW_REV_1_1
-- delete off and used real register name directly
Reported-by: kernel test robot
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/Makefile | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 32 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h | 14 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c | 385
+
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 7 +-
5 files changed, 435 insertions(+), 4 deletions(-)
create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index b814fc8..b9af5e4 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -65,6 +65,7 @@ msm-$(CONFIG_DRM_MSM_DPU) += \
disp/dpu1/dpu_hw_catalog.o \
disp/dpu1/dpu_hw_ctl.o \
disp/dpu1/dpu_hw_dsc.o \
+ disp/dpu1/dpu_hw_dsc_1_2.o \
disp/dpu1/dpu_hw_interrupts.o \
disp/dpu1/dpu_hw_intf.o \
disp/dpu1/dpu_hw_lm.o \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index dc0a4da..4eda2cc 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -1,6 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0-only */
/*
- * Copyright (c) 2022. Qualcomm Innovation Center, Inc. All rights
reserved.
+ * Copyright (c) 2022-2023, Qualcomm Innovation Center, Inc. All
rights reserved.
* Copyright (c) 2015-2018, 2020 The Linux Foundation. All rights
reserved.
*/
@@ -244,12 +244,18 @@ enum {
};
/**
- * DSC features
+ * DSC sub-blocks/features
* @DPU_DSC_OUTPUT_CTRL Configure which PINGPONG block gets
* the pixel output from this DSC.
+ * @DPU_DSC_HW_REV_1_2 DSC block supports dsc 1.1 and 1.2
+ * @DPU_DSC_NATIVE_422_EN Supports native422 and native420 encoding
+ * @DPU_DSC_MAX
*/
enum {
DPU_DSC_OUTPUT_CTRL = 0x1,
+ DPU_DSC_HW_REV_1_2,
+ DPU_DSC_NATIVE_422_EN,
+ DPU_DSC_MAX
};
/**
@@ -306,6 +312,14 @@ struct dpu_pp_blk {
};
/**
+ * struct dpu_dsc_blk - DSC Encoder sub-blk information
+ * @info: HW register and features supported by this sub-blk
+ */
+struct dpu_dsc_blk {
+ DPU_HW_SUBBLK_INFO;
+};
+
+/**
* enum dpu_qos_lut_usage - define QoS LUT use cases
*/
enum dpu_qos_lut_usage {
@@ -452,6 +466,17 @@ struct dpu_pingpong_sub_blks {
};
/**
+ * struct dpu_dsc_sub_blks - DSC sub-blks
+ * @enc: DSC encoder sub block
+ * @ctl: DSC controller sub block
+ *
+ */
+struct dpu_dsc_sub_blks {
+ struct dpu_dsc_blk enc;
+ struct dpu_dsc_blk ctl;
+};
+
+/**
* dpu_clk_ctrl_type - Defines top level clock control signals
*/
enum dpu_clk_ctrl_type {
@@ -605,10 +630,13 @@ struct dpu_merge_3d_cfg {
* struct dpu_dsc_cfg - information of DSC blocks
* @id enum identifying this block
* @base register offset of this block
+ * @len: length of hardware block
* @features bit mask identifying sub-blocks/features
+ * @sblk sub-blocks information
*/
struct dpu_dsc_cfg {
DPU_HW_BLK_INFO;
+ const struct dpu_dsc_sub_blks *sblk;
};
/**
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
index 138080a..bdff74d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
@@ -1,5 +1,8 @@
/* SPDX-License-Identifier: GPL-2.0-only */
-/* Copyright (c) 2020-2022, Linaro Limited */
+/*
+ * Copyright (c) 2020-2022, Linaro Limited
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights
reserved
+ */
#ifndef _DPU_HW_DSC_H
#define _DPU_HW_DSC_H
@@ -69,6 +72,15 @@ struct dpu_hw_dsc *dpu_hw_dsc_init(const struct
dpu_dsc_cfg *cfg,
void __iomem *addr);
/**
+ * dpu_hw_dsc_init_1_2 - initializes the v1.2 DSC hw driver block
+ * @cfg: DSC catalog entry for which driver object is required
+ * @addr: Mapped register io address of MDP
+ * Returns: Error code or allocated dpu_hw_dsc context
+ */
+struct dpu_hw_dsc *dpu_hw_dsc_init_1_2(const struct dpu_dsc_cfg *cfg,
+ void __iomem *addr);
+
+/**
* dpu_hw_dsc_destroy - destroys dsc driver context
* @dsc: Pointer to dsc driver context returned by dpu_hw_dsc_init
*/
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
new file mode 100644
index ..0c77c85
--- /dev/null
+++
Re: [Freedreno] [PATCH v6 5/8] drm/msm/dpu: add support for DSC encoder v1.2 engine
On 5/10/2023 9:52 PM, Dmitry Baryshkov wrote:
On 11/05/2023 01:07, Kuogee Hsieh wrote:
Add support for DSC 1.2 by providing the necessary hooks to program
the DPU DSC 1.2 encoder.
Changes in v3:
-- fixed kernel test rebot report that "__iomem *off" is declared but
not
used at dpu_hw_dsc_config_1_2()
-- unrolling thresh loops
Changes in v4:
-- delete DPU_DSC_HW_REV_1_1
-- delete off and used real register name directly
Reported-by: kernel test robot
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/Makefile | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 32 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h | 14 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c | 385
+
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 7 +-
5 files changed, 435 insertions(+), 4 deletions(-)
create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index b814fc8..b9af5e4 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -65,6 +65,7 @@ msm-$(CONFIG_DRM_MSM_DPU) += \
disp/dpu1/dpu_hw_catalog.o \
disp/dpu1/dpu_hw_ctl.o \
disp/dpu1/dpu_hw_dsc.o \
+ disp/dpu1/dpu_hw_dsc_1_2.o \
disp/dpu1/dpu_hw_interrupts.o \
disp/dpu1/dpu_hw_intf.o \
disp/dpu1/dpu_hw_lm.o \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index dc0a4da..4eda2cc 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -1,6 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0-only */
/*
- * Copyright (c) 2022. Qualcomm Innovation Center, Inc. All rights
reserved.
+ * Copyright (c) 2022-2023, Qualcomm Innovation Center, Inc. All
rights reserved.
* Copyright (c) 2015-2018, 2020 The Linux Foundation. All rights
reserved.
*/
@@ -244,12 +244,18 @@ enum {
};
/**
- * DSC features
+ * DSC sub-blocks/features
* @DPU_DSC_OUTPUT_CTRL Configure which PINGPONG block gets
* the pixel output from this DSC.
+ * @DPU_DSC_HW_REV_1_2 DSC block supports dsc 1.1 and 1.2
+ * @DPU_DSC_NATIVE_422_EN Supports native422 and native420 encoding
+ * @DPU_DSC_MAX
*/
enum {
DPU_DSC_OUTPUT_CTRL = 0x1,
+ DPU_DSC_HW_REV_1_2,
+ DPU_DSC_NATIVE_422_EN,
+ DPU_DSC_MAX
};
/**
@@ -306,6 +312,14 @@ struct dpu_pp_blk {
};
/**
+ * struct dpu_dsc_blk - DSC Encoder sub-blk information
+ * @info: HW register and features supported by this sub-blk
+ */
+struct dpu_dsc_blk {
+ DPU_HW_SUBBLK_INFO;
+};
+
+/**
* enum dpu_qos_lut_usage - define QoS LUT use cases
*/
enum dpu_qos_lut_usage {
@@ -452,6 +466,17 @@ struct dpu_pingpong_sub_blks {
};
/**
+ * struct dpu_dsc_sub_blks - DSC sub-blks
+ * @enc: DSC encoder sub block
+ * @ctl: DSC controller sub block
+ *
+ */
+struct dpu_dsc_sub_blks {
+ struct dpu_dsc_blk enc;
+ struct dpu_dsc_blk ctl;
+};
+
+/**
* dpu_clk_ctrl_type - Defines top level clock control signals
*/
enum dpu_clk_ctrl_type {
@@ -605,10 +630,13 @@ struct dpu_merge_3d_cfg {
* struct dpu_dsc_cfg - information of DSC blocks
* @id enum identifying this block
* @base register offset of this block
+ * @len: length of hardware block
* @features bit mask identifying sub-blocks/features
+ * @sblk sub-blocks information
*/
struct dpu_dsc_cfg {
DPU_HW_BLK_INFO;
+ const struct dpu_dsc_sub_blks *sblk;
};
/**
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
index 138080a..bdff74d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
@@ -1,5 +1,8 @@
/* SPDX-License-Identifier: GPL-2.0-only */
-/* Copyright (c) 2020-2022, Linaro Limited */
+/*
+ * Copyright (c) 2020-2022, Linaro Limited
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights
reserved
+ */
#ifndef _DPU_HW_DSC_H
#define _DPU_HW_DSC_H
@@ -69,6 +72,15 @@ struct dpu_hw_dsc *dpu_hw_dsc_init(const struct
dpu_dsc_cfg *cfg,
void __iomem *addr);
/**
+ * dpu_hw_dsc_init_1_2 - initializes the v1.2 DSC hw driver block
+ * @cfg: DSC catalog entry for which driver object is required
+ * @addr: Mapped register io address of MDP
+ * Returns: Error code or allocated dpu_hw_dsc context
+ */
+struct dpu_hw_dsc *dpu_hw_dsc_init_1_2(const struct dpu_dsc_cfg *cfg,
+ void __iomem *addr);
+
+/**
* dpu_hw_dsc_destroy - destroys dsc driver context
* @dsc: Pointer to dsc driver context returned by dpu_hw_dsc_init
*/
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
new file mode 100644
index ..0c77c85
--- /dev/null
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc_1_2.c
@@ -0,0
Re: [Freedreno] [PATCH v1 0/2] enable HDP plugin/unplugged interrupts to hpd_enable/disable
On 10/05/2023 23:31, Kuogee Hsieh wrote: There is bug report on exteranl DP display does not work. This patch add below two patches to fix the problem. 1) enable HDP plugin/unplugged interrupts to hpd_enable/disable 2) add mutex to protect internal_hpd against race condition between different threads Kuogee Hsieh (2): drm/msm/dp: enable HDP plugin/unplugged interrupts to hpd_enable/disable drm/msm/dp: add mutex to protect internal_hpd against race condition between different threads drivers/gpu/drm/msm/dp/dp_display.c | 37 +++-- 1 file changed, 23 insertions(+), 14 deletions(-) BTW: Kuogee, what happened to the patchset promised at [1] ? In the reply, [2], I asked you to remove DP_HPD_INIT_SETUP completely, and then you went silent. [1] https://lore.kernel.org/dri-devel/4c733721-855a-85fd-82a9-9af0f80fc...@quicinc.com/ [2] https://lore.kernel.org/dri-devel/358262c3-e501-3c7f-7502-f0323cdcc...@linaro.org/ -- With best wishes Dmitry
Re: [Freedreno] [PATCH v1 1/2] drm/msm/dp: enable HDP plugin/unplugged interrupts to hpd_enable/disable
On 11/05/2023 18:53, Bjorn Andersson wrote:
On Thu, May 11, 2023 at 07:24:46AM +0300, Dmitry Baryshkov wrote:
On Wed, 10 May 2023 at 23:31, Kuogee Hsieh wrote:
The internal_hpd flag was introduced to handle external DP HPD derived from GPIO
pinmuxed into DP controller. HPD plug/unplug interrupts cannot be enabled until
internal_hpd flag is set to true.
At both bootup and resume time, the DP driver will enable external DP
plugin interrupts and handle plugin interrupt accordingly. Unfortunately
dp_bridge_hpd_enable() bridge ops function was called to set internal_hpd
flag to true later than where DP driver expected during bootup time.
This causes external DP plugin event to not get detected and display stays
blank.
Move enabling HDP plugin/unplugged interrupts to
dp_bridge_hpd_enable()/disable() to
set internal_hpd to true along with enabling HPD plugin/unplugged interrupts
simultaneously to avoid timing issue during bootup and resume.
Fixes: cd198caddea7 ("drm/msm/dp: Rely on hpd_enable/disable callbacks")
Signed-off-by: Kuogee Hsieh
Thanks for debugging this!
However after looking at the driver I think there is more than this.
We have several other places gated on internal_hpd flag, where we do
not have a strict ordering of events.
I see that dp_hpd_plug_handle() and dp_hpd_unplug_handle() also toggle
DP_DP_IRQ_HPD_INT_MASK and DP_DP_HPD_REPLUG_INT_MASK depending on
internal_hpd. Can we toggle all 4 interrupts from the
hpd_enable/hpd_disable functions? If we can do it, then I think we can
drop the internal_hpd flag completely.
Yes, that's what I believe the DRM framework intend us to do.
The problem, and reason why I didn't do tat in my series, was that in
order to update the INT_MASKs you need to clock the IP-block and that's
done elsewhere.
So, for the internal_hpd case, it seems appropriate to pm_runtime_get()
in hpd_enable() and unmask the HPD interrupts, and mask the interrupts
and pm_runtime_put() in hpd_disable().
But for edp and external HPD-signal we also need to make sure power is
on while something is connected...
I think this is already handled by the existing code, see calls to the
dp_display_host_init().
I went on and checked other places where it is used:
- dp_hpd_unplug_handle(), guarding DP_DP_HPD_PLUG_INT_MASK toggling. I
think we can drop these two calls completely. The function is under
the event_mutex protection, so other events can not interfere.
- dp_bridge_hpd_notify(). What is the point of this check? If some
other party informs us of the HPD event, we'd better handle it instead
of dropping it. Correct? In other words, I'd prefer seeing the
hpd_event_thread removal. Instead of that I think that on
HPD/plug/unplug/etc. IRQ the driver should call into the drm stack,
then the hpd_notify call should process those events.
I agree, that seems to be what's expected of us from the DRM framework.
Regards,
Bjorn
---
drivers/gpu/drm/msm/dp/dp_display.c | 27 ++-
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 3e13acdf..71aa944 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1088,13 +1088,6 @@ static void dp_display_config_hpd(struct
dp_display_private *dp)
dp_display_host_init(dp);
dp_catalog_ctrl_hpd_config(dp->catalog);
- /* Enable plug and unplug interrupts only if requested */
- if (dp->dp_display.internal_hpd)
- dp_catalog_hpd_config_intr(dp->catalog,
- DP_DP_HPD_PLUG_INT_MASK |
- DP_DP_HPD_UNPLUG_INT_MASK,
- true);
-
/* Enable interrupt first time
* we are leaving dp clocks on during disconnect
* and never disable interrupt
@@ -1396,12 +1389,6 @@ static int dp_pm_resume(struct device *dev)
dp_catalog_ctrl_hpd_config(dp->catalog);
- if (dp->dp_display.internal_hpd)
- dp_catalog_hpd_config_intr(dp->catalog,
- DP_DP_HPD_PLUG_INT_MASK |
- DP_DP_HPD_UNPLUG_INT_MASK,
- true);
-
if (dp_catalog_link_is_connected(dp->catalog)) {
/*
* set sink to normal operation mode -- D0
@@ -1801,15 +1788,29 @@ void dp_bridge_hpd_enable(struct drm_bridge *bridge)
{
struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
struct msm_dp *dp_display = dp_bridge->dp_display;
+ struct dp_display_private *dp;
+
+ dp = container_of(dp_display, struct dp_display_private, dp_display);
dp_display->internal_hpd = true;
+ dp_catalog_hpd_config_intr(dp->catalog,
+ DP_DP_HPD_PLUG_INT_MASK |
+ DP_DP_HPD_UNPLUG_INT_MASK,
+ true);
}
void
Re: [Freedreno] [PATCH v1 1/2] drm/msm/dp: enable HDP plugin/unplugged interrupts to hpd_enable/disable
On Thu, May 11, 2023 at 07:24:46AM +0300, Dmitry Baryshkov wrote:
> On Wed, 10 May 2023 at 23:31, Kuogee Hsieh wrote:
> >
> > The internal_hpd flag was introduced to handle external DP HPD derived from
> > GPIO
> > pinmuxed into DP controller. HPD plug/unplug interrupts cannot be enabled
> > until
> > internal_hpd flag is set to true.
> > At both bootup and resume time, the DP driver will enable external DP
> > plugin interrupts and handle plugin interrupt accordingly. Unfortunately
> > dp_bridge_hpd_enable() bridge ops function was called to set internal_hpd
> > flag to true later than where DP driver expected during bootup time.
> >
> > This causes external DP plugin event to not get detected and display stays
> > blank.
> > Move enabling HDP plugin/unplugged interrupts to
> > dp_bridge_hpd_enable()/disable() to
> > set internal_hpd to true along with enabling HPD plugin/unplugged interrupts
> > simultaneously to avoid timing issue during bootup and resume.
> >
> > Fixes: cd198caddea7 ("drm/msm/dp: Rely on hpd_enable/disable callbacks")
> > Signed-off-by: Kuogee Hsieh
>
> Thanks for debugging this!
>
> However after looking at the driver I think there is more than this.
>
> We have several other places gated on internal_hpd flag, where we do
> not have a strict ordering of events.
> I see that dp_hpd_plug_handle() and dp_hpd_unplug_handle() also toggle
> DP_DP_IRQ_HPD_INT_MASK and DP_DP_HPD_REPLUG_INT_MASK depending on
> internal_hpd. Can we toggle all 4 interrupts from the
> hpd_enable/hpd_disable functions? If we can do it, then I think we can
> drop the internal_hpd flag completely.
>
Yes, that's what I believe the DRM framework intend us to do.
The problem, and reason why I didn't do tat in my series, was that in
order to update the INT_MASKs you need to clock the IP-block and that's
done elsewhere.
So, for the internal_hpd case, it seems appropriate to pm_runtime_get()
in hpd_enable() and unmask the HPD interrupts, and mask the interrupts
and pm_runtime_put() in hpd_disable().
But for edp and external HPD-signal we also need to make sure power is
on while something is connected...
> I went on and checked other places where it is used:
> - dp_hpd_unplug_handle(), guarding DP_DP_HPD_PLUG_INT_MASK toggling. I
> think we can drop these two calls completely. The function is under
> the event_mutex protection, so other events can not interfere.
> - dp_bridge_hpd_notify(). What is the point of this check? If some
> other party informs us of the HPD event, we'd better handle it instead
> of dropping it. Correct? In other words, I'd prefer seeing the
> hpd_event_thread removal. Instead of that I think that on
> HPD/plug/unplug/etc. IRQ the driver should call into the drm stack,
> then the hpd_notify call should process those events.
>
I agree, that seems to be what's expected of us from the DRM framework.
Regards,
Bjorn
>
> > ---
> > drivers/gpu/drm/msm/dp/dp_display.c | 27 ++-
> > 1 file changed, 14 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 3e13acdf..71aa944 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -1088,13 +1088,6 @@ static void dp_display_config_hpd(struct
> > dp_display_private *dp)
> > dp_display_host_init(dp);
> > dp_catalog_ctrl_hpd_config(dp->catalog);
> >
> > - /* Enable plug and unplug interrupts only if requested */
> > - if (dp->dp_display.internal_hpd)
> > - dp_catalog_hpd_config_intr(dp->catalog,
> > - DP_DP_HPD_PLUG_INT_MASK |
> > - DP_DP_HPD_UNPLUG_INT_MASK,
> > - true);
> > -
> > /* Enable interrupt first time
> > * we are leaving dp clocks on during disconnect
> > * and never disable interrupt
> > @@ -1396,12 +1389,6 @@ static int dp_pm_resume(struct device *dev)
> >
> > dp_catalog_ctrl_hpd_config(dp->catalog);
> >
> > - if (dp->dp_display.internal_hpd)
> > - dp_catalog_hpd_config_intr(dp->catalog,
> > - DP_DP_HPD_PLUG_INT_MASK |
> > - DP_DP_HPD_UNPLUG_INT_MASK,
> > - true);
> > -
> > if (dp_catalog_link_is_connected(dp->catalog)) {
> > /*
> > * set sink to normal operation mode -- D0
> > @@ -1801,15 +1788,29 @@ void dp_bridge_hpd_enable(struct drm_bridge *bridge)
> > {
> > struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
> > struct msm_dp *dp_display = dp_bridge->dp_display;
> > + struct dp_display_private *dp;
> > +
> > + dp = container_of(dp_display, struct dp_display_private,
> > dp_display);
> >
> > dp_display->internal_hpd = true;
> > + dp_catalog_hpd_config_intr(dp->catalog,
> > +
Re: [Freedreno] [PATCH v1 1/2] drm/msm/dp: enable HDP plugin/unplugged interrupts to hpd_enable/disable
On Wed, May 10, 2023 at 05:39:07PM -0700, Abhinav Kumar wrote:
>
>
> On 5/10/2023 4:55 PM, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2023-05-10 13:31:04)
> > > The internal_hpd flag was introduced to handle external DP HPD derived
> > > from GPIO
> > > pinmuxed into DP controller.
> >
> > Was it? It looks more like it was done to differentiate between eDP and
> > DP, because internal_hpd is set only if DRM_BRIDGE_OP_HPD is set on the
> > bridge and we only set the bridge op if the connector type is DP. The
> > assumption looks like if you have DP connector_type, you have the gpio
> > pinmuxed for "dp_hot" mode, which isn't exactly true. We don't treat
> > that gpio as an irq either, because it isn't. Instead the gpio is muxed
> > to the mdss inside the SoC and then that generates an mdss interrupt
> > that's combined with non-HPD things like "video ready".
> >
> > If that all follows, then I don't quite understand why we're setting
> > internal_hpd to false at all at runtime. It should be set to true at
> > some point, but ideally that point is during probe.
> >
>
> Kuogee had the same thought originally but were not entirely sure of this
> part of the commit message in Bjorn's original commit which introduced these
> changes.
>
> "This difference is not appropriately represented by the "is_edp"
> boolean, but is properly represented by the frameworks invocation of the
> hpd_enable() and hpd_disable() callbacks. Switch the current condition
> to rely on these callbacks instead"
>
> Does this along with below documentation mean we should generate the hpd
> interrupts only after hpd_enable callback happens?
>
> " * Call _bridge_funcs.hpd_enable if implemented and register the given
> @cb
> * and @data as hot plug notification callback. From now on the @cb will be
> * called with @data when an output status change is detected by the bridge,
> * until hot plug notification gets disabled with drm_bridge_hpd_disable().
> "
>
> Bjorn, can you please clarify this?
>
We currently have 3 cases:
1) DP with GPIO: No downstream drm_bridge are connected, is_edp = false
and internal HPD-logic is in used (internal_hpd = true). Power needs to
be on at all times etc.
2) DP without GPIO: Downstream drm_bridge connected, is_edp = false and
internal HPD-logic should not be used/enabled (internal_hpd = false).
Power doesn't need to be on unless hpd_notify is invoked to tell us that
there's something connected...
3) eDP with or without HPD signal and/or HPD gpio. Downstream
drm_bridge/panel is connected, is_edp = true and internal HPD logic is
short-circuited regardless of the panel providing HPD signal or not.
In #1 dp_bridge_hpd_enable() will be invoked to indicate that the DP
controller is expected to perform HPD handling. In #2
dp_bridge_hpd_enable() will _not_ be invoked, instead some downstream
drm_bridge/panel will get the hpd_enable() callback and will be
responsible to updating the HPD state of the chain, which will cause
hpd_notify to be invoked.
Note that #3 is based entirely on the controller, it has currently no
relation to what is attached. It seems reasonable that this is just
another case of #2 (perhaps just always reporting
connector_status_connected?).
Regards,
Bjorn
> > > HPD plug/unplug interrupts cannot be enabled until
> > > internal_hpd flag is set to true.
> > > At both bootup and resume time, the DP driver will enable external DP
> > > plugin interrupts and handle plugin interrupt accordingly. Unfortunately
> > > dp_bridge_hpd_enable() bridge ops function was called to set internal_hpd
> > > flag to true later than where DP driver expected during bootup time.
> > >
> > > This causes external DP plugin event to not get detected and display
> > > stays blank.
> > > Move enabling HDP plugin/unplugged interrupts to
> > > dp_bridge_hpd_enable()/disable() to
> > > set internal_hpd to true along with enabling HPD plugin/unplugged
> > > interrupts
> > > simultaneously to avoid timing issue during bootup and resume.
> > >
> > > Fixes: cd198caddea7 ("drm/msm/dp: Rely on hpd_enable/disable callbacks")
> > > Signed-off-by: Kuogee Hsieh
> > > ---
> > > drivers/gpu/drm/msm/dp/dp_display.c | 27 ++-
> > > 1 file changed, 14 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > > b/drivers/gpu/drm/msm/dp/dp_display.c
> > > index 3e13acdf..71aa944 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > @@ -1801,15 +1788,29 @@ void dp_bridge_hpd_enable(struct drm_bridge
> > > *bridge)
> > > {
> > > struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
> > > struct msm_dp *dp_display = dp_bridge->dp_display;
> > > + struct dp_display_private *dp;
> > > +
> > > + dp = container_of(dp_display, struct dp_display_private,
> > > dp_display);
> > >
> > > dp_display->internal_hpd = true;
> >
> > Can we set internal_hpd to true
Re: [Freedreno] [PATCH v1 1/2] drm/msm/dp: enable HDP plugin/unplugged interrupts to hpd_enable/disable
On Wed, May 10, 2023 at 04:55:04PM -0700, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2023-05-10 13:31:04)
> > The internal_hpd flag was introduced to handle external DP HPD derived from
> > GPIO
> > pinmuxed into DP controller.
>
> Was it? It looks more like it was done to differentiate between eDP and
> DP, because internal_hpd is set only if DRM_BRIDGE_OP_HPD is set on the
> bridge and we only set the bridge op if the connector type is DP. The
> assumption looks like if you have DP connector_type, you have the gpio
> pinmuxed for "dp_hot" mode, which isn't exactly true. We don't treat
> that gpio as an irq either, because it isn't. Instead the gpio is muxed
> to the mdss inside the SoC and then that generates an mdss interrupt
> that's combined with non-HPD things like "video ready".
>
The purpose of "internal_hpd" is to indicate track if the internal
HPD-logic should be used or not.
> If that all follows, then I don't quite understand why we're setting
> internal_hpd to false at all at runtime. It should be set to true at
> some point, but ideally that point is during probe.
>
The DRM framework will invoke hpd_enable on the bridge furthest out that
has OP_HPD. So in the case of HPD signal being pinmuxed into the
HPD-logic, dp_bridge_hpd_enable() will be invoked.
I therefor think the appropriate thing to do is to move the enablement
of the internal HPD-logic to dp_bridge_hpd_enable(), further more I
think the correct thing to do would be to tie the power state of the DP
block to this (and to when the external hpd_notify events signals that
there's something attached).
> > HPD plug/unplug interrupts cannot be enabled until
> > internal_hpd flag is set to true.
> > At both bootup and resume time, the DP driver will enable external DP
> > plugin interrupts and handle plugin interrupt accordingly. Unfortunately
> > dp_bridge_hpd_enable() bridge ops function was called to set internal_hpd
> > flag to true later than where DP driver expected during bootup time.
> >
> > This causes external DP plugin event to not get detected and display stays
> > blank.
> > Move enabling HDP plugin/unplugged interrupts to
> > dp_bridge_hpd_enable()/disable() to
> > set internal_hpd to true along with enabling HPD plugin/unplugged interrupts
> > simultaneously to avoid timing issue during bootup and resume.
> >
> > Fixes: cd198caddea7 ("drm/msm/dp: Rely on hpd_enable/disable callbacks")
> > Signed-off-by: Kuogee Hsieh
> > ---
> > drivers/gpu/drm/msm/dp/dp_display.c | 27 ++-
> > 1 file changed, 14 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 3e13acdf..71aa944 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -1801,15 +1788,29 @@ void dp_bridge_hpd_enable(struct drm_bridge *bridge)
> > {
> > struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
> > struct msm_dp *dp_display = dp_bridge->dp_display;
> > + struct dp_display_private *dp;
> > +
> > + dp = container_of(dp_display, struct dp_display_private,
> > dp_display);
> >
> > dp_display->internal_hpd = true;
>
> Can we set internal_hpd to true during probe when we see that the hpd
> pinmux exists? Or do any of these bits toggle in the irq status register
> when the gpio isn't muxed to "dp_hot" or the controller is for eDP and
> it doesn't have any gpio connection internally? I'm wondering if we can
> get by with simply enabling the "dp_hot" pin interrupts
> (plug/unplug/replug/irq_hpd) unconditionally and not worrying about them
> if eDP is there (because the pin doesn't exist inside the SoC), or if DP
> HPD is being signalled out of band through type-c framework.
It seems logical to me that the panel driver should handle HPD signaling
(or signal it always-asserted), in which case it also seems reasonable
that hpd_enable() would not be invoked... I didn't go far enough into
that rabbit hole though, but I think it would allow us to drop the
is_edp flag (which isn't at all a property of the controller, but of
what you have connected).
I don't think we should peak into the pinmux settings to determine if
the internal HPD logic should be enabled or not, when the DRM framework
already has this callback to tell us "hey, you're the one doing HPD
detection!".
And as mentioned above, the continuation of this is to tie the power
state to hpd_enable/hpd_disable/hpd_notify and thereby allow the DP
block (and MMCX) to be powered down when nothing is connected (and we
don't need to drive the HPD logic).
Regards,
Bjorn
[Freedreno] [PATCH v3 1/2] iommu/arm-smmu-qcom: Fix missing adreno_smmu's
From: Rob Clark
When the special handling of qcom,adreno-smmu was moved into
qcom_smmu_create(), it was overlooked that we didn't have all the
required entries in qcom_smmu_impl_of_match. So we stopped getting
adreno_smmu_priv on sc7180, breaking per-process pgtables.
Fixes: 30b912a03d91 ("iommu/arm-smmu-qcom: Move the qcom,adreno-smmu check into
qcom_smmu_create")
Suggested-by: Lepton Wu
Signed-off-by: Rob Clark
---
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index d1b296b95c86..66e191773099 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -496,20 +496,21 @@ static const struct qcom_smmu_match_data
qcom_smmu_500_impl0_data = {
/*
* Do not add any more qcom,SOC-smmu-500 entries to this list, unless they need
* special handling and can not be covered by the qcom,smmu-500 entry.
*/
static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = {
{ .compatible = "qcom,msm8996-smmu-v2", .data = _smmu_data },
{ .compatible = "qcom,msm8998-smmu-v2", .data = _smmu_v2_data },
{ .compatible = "qcom,qcm2290-smmu-500", .data =
_smmu_500_impl0_data },
{ .compatible = "qcom,qdu1000-smmu-500", .data =
_smmu_500_impl0_data },
{ .compatible = "qcom,sc7180-smmu-500", .data =
_smmu_500_impl0_data },
+ { .compatible = "qcom,sc7180-smmu-v2", .data = _smmu_v2_data },
{ .compatible = "qcom,sc7280-smmu-500", .data =
_smmu_500_impl0_data },
{ .compatible = "qcom,sc8180x-smmu-500", .data =
_smmu_500_impl0_data },
{ .compatible = "qcom,sc8280xp-smmu-500", .data =
_smmu_500_impl0_data },
{ .compatible = "qcom,sdm630-smmu-v2", .data = _smmu_v2_data },
{ .compatible = "qcom,sdm845-smmu-v2", .data = _smmu_v2_data },
{ .compatible = "qcom,sdm845-smmu-500", .data = _smmu_500_data },
{ .compatible = "qcom,sm6115-smmu-500", .data =
_smmu_500_impl0_data},
{ .compatible = "qcom,sm6125-smmu-500", .data =
_smmu_500_impl0_data },
{ .compatible = "qcom,sm6350-smmu-v2", .data = _smmu_v2_data },
{ .compatible = "qcom,sm6350-smmu-500", .data =
_smmu_500_impl0_data },
@@ -540,12 +541,18 @@ struct arm_smmu_device *qcom_smmu_impl_init(struct
arm_smmu_device *smmu)
/* Match platform for ACPI boot */
if (acpi_match_platform_list(qcom_acpi_platlist) >= 0)
return qcom_smmu_create(smmu,
_smmu_500_impl0_data);
}
#endif
match = of_match_node(qcom_smmu_impl_of_match, np);
if (match)
return qcom_smmu_create(smmu, match->data);
+ /* If you hit this WARN_ON() you are missing an entry in the
+* qcom_smmu_impl_of_match[] table, and GPU per-process page-
+* tables will be broken.
+*/
+ WARN_ON(of_device_is_compatible(np, "qcom,adreno-smmu"));
+
return smmu;
}
--
2.40.1
[Freedreno] [PATCH v3 2/2] drm/msm: Be more shouty if per-process pgtables aren't working
From: Rob Clark Otherwise it is not always obvious if a dt or iommu change is causing us to fall back to global pgtable. Signed-off-by: Rob Clark --- drivers/gpu/drm/msm/msm_iommu.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c index 418e1e06cdde..9b19124c9bd0 100644 --- a/drivers/gpu/drm/msm/msm_iommu.c +++ b/drivers/gpu/drm/msm/msm_iommu.c @@ -227,21 +227,26 @@ struct msm_mmu *msm_iommu_pagetable_create(struct msm_mmu *parent) struct adreno_smmu_priv *adreno_smmu = dev_get_drvdata(parent->dev); struct msm_iommu *iommu = to_msm_iommu(parent); struct msm_iommu_pagetable *pagetable; const struct io_pgtable_cfg *ttbr1_cfg = NULL; struct io_pgtable_cfg ttbr0_cfg; int ret; /* Get the pagetable configuration from the domain */ if (adreno_smmu->cookie) ttbr1_cfg = adreno_smmu->get_ttbr1_cfg(adreno_smmu->cookie); - if (!ttbr1_cfg) + + /* +* If you hit this WARN_ONCE() you are probably missing an entry in +* qcom_smmu_impl_of_match[] in arm-smmu-qcom.c +*/ + if (WARN_ONCE(!ttbr1_cfg, "No per-process page tables")) return ERR_PTR(-ENODEV); pagetable = kzalloc(sizeof(*pagetable), GFP_KERNEL); if (!pagetable) return ERR_PTR(-ENOMEM); msm_mmu_init(>base, parent->dev, _funcs, MSM_MMU_IOMMU_PAGETABLE); /* Clone the TTBR1 cfg as starting point for TTBR0 cfg: */ -- 2.40.1
Re: [Freedreno] [PATCH 1/2] iommu/arm-smmu-qcom: Fix missing adreno_smmu's
On Thu, May 11, 2023 at 7:51 AM Dmitry Baryshkov
wrote:
>
> On Tue, 9 May 2023 at 19:37, Rob Clark wrote:
> >
> > From: Rob Clark
> >
> > When the special handling of qcom,adreno-smmu was moved into
> > qcom_smmu_create(), it was overlooked that we didn't have all the
> > required entries in qcom_smmu_impl_of_match. So we stopped getting
> > adreno_smmu_priv on sc7180, breaking per-process pgtables.
> >
> > Fixes: 30b912a03d91 ("iommu/arm-smmu-qcom: Move the qcom,adreno-smmu check
> > into qcom_smmu_create")
> > Suggested-by: Lepton Wu
> > Signed-off-by: Rob Clark
> > ---
> > drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > index d1b296b95c86..760d9c43dbd2 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > @@ -496,20 +496,21 @@ static const struct qcom_smmu_match_data
> > qcom_smmu_500_impl0_data = {
> > /*
> > * Do not add any more qcom,SOC-smmu-500 entries to this list, unless they
> > need
> > * special handling and can not be covered by the qcom,smmu-500 entry.
> > */
> > static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[]
> > = {
> > { .compatible = "qcom,msm8996-smmu-v2", .data = _smmu_data
> > },
> > { .compatible = "qcom,msm8998-smmu-v2", .data = _smmu_v2_data
> > },
> > { .compatible = "qcom,qcm2290-smmu-500", .data =
> > _smmu_500_impl0_data },
> > { .compatible = "qcom,qdu1000-smmu-500", .data =
> > _smmu_500_impl0_data },
> > { .compatible = "qcom,sc7180-smmu-500", .data =
> > _smmu_500_impl0_data },
> > + { .compatible = "qcom,sc7180-smmu-v2", .data = _smmu_v2_data },
> > { .compatible = "qcom,sc7280-smmu-500", .data =
> > _smmu_500_impl0_data },
> > { .compatible = "qcom,sc8180x-smmu-500", .data =
> > _smmu_500_impl0_data },
> > { .compatible = "qcom,sc8280xp-smmu-500", .data =
> > _smmu_500_impl0_data },
> > { .compatible = "qcom,sdm630-smmu-v2", .data = _smmu_v2_data },
> > { .compatible = "qcom,sdm845-smmu-v2", .data = _smmu_v2_data },
> > { .compatible = "qcom,sdm845-smmu-500", .data =
> > _smmu_500_data },
> > { .compatible = "qcom,sm6115-smmu-500", .data =
> > _smmu_500_impl0_data},
> > { .compatible = "qcom,sm6125-smmu-500", .data =
> > _smmu_500_impl0_data },
> > { .compatible = "qcom,sm6350-smmu-v2", .data = _smmu_v2_data },
> > { .compatible = "qcom,sm6350-smmu-500", .data =
> > _smmu_500_impl0_data },
> > @@ -540,12 +541,14 @@ struct arm_smmu_device *qcom_smmu_impl_init(struct
> > arm_smmu_device *smmu)
> > /* Match platform for ACPI boot */
> > if (acpi_match_platform_list(qcom_acpi_platlist) >= 0)
> > return qcom_smmu_create(smmu,
> > _smmu_500_impl0_data);
> > }
> > #endif
> >
> > match = of_match_node(qcom_smmu_impl_of_match, np);
> > if (match)
> > return qcom_smmu_create(smmu, match->data);
> >
> > + WARN_ON(of_device_is_compatible(np, "qcom,adreno-smmu"));
>
> Could you please add a comment here, noting the reason? Or maybe we
> should change that to:
> if (WARN_ON(...))
> return ERR_PTR(-EINVAL);
I'll add a comment. Not having an iommu is even worse, so returning
an error isn't a good idea. I just wanted to leave some breadcrumbs
so people can see where the problem actually is if per-process
pgtables break again.
BR,
-R
>
> > +
> > return smmu;
> > }
> > --
> > 2.40.1
> >
>
>
> --
> With best wishes
> Dmitry
Re: [Freedreno] [PATCH 1/2] iommu/arm-smmu-qcom: Fix missing adreno_smmu's
On Tue, 9 May 2023 at 19:37, Rob Clark wrote:
>
> From: Rob Clark
>
> When the special handling of qcom,adreno-smmu was moved into
> qcom_smmu_create(), it was overlooked that we didn't have all the
> required entries in qcom_smmu_impl_of_match. So we stopped getting
> adreno_smmu_priv on sc7180, breaking per-process pgtables.
>
> Fixes: 30b912a03d91 ("iommu/arm-smmu-qcom: Move the qcom,adreno-smmu check
> into qcom_smmu_create")
> Suggested-by: Lepton Wu
> Signed-off-by: Rob Clark
> ---
> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index d1b296b95c86..760d9c43dbd2 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -496,20 +496,21 @@ static const struct qcom_smmu_match_data
> qcom_smmu_500_impl0_data = {
> /*
> * Do not add any more qcom,SOC-smmu-500 entries to this list, unless they
> need
> * special handling and can not be covered by the qcom,smmu-500 entry.
> */
> static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = {
> { .compatible = "qcom,msm8996-smmu-v2", .data = _smmu_data },
> { .compatible = "qcom,msm8998-smmu-v2", .data = _smmu_v2_data },
> { .compatible = "qcom,qcm2290-smmu-500", .data =
> _smmu_500_impl0_data },
> { .compatible = "qcom,qdu1000-smmu-500", .data =
> _smmu_500_impl0_data },
> { .compatible = "qcom,sc7180-smmu-500", .data =
> _smmu_500_impl0_data },
> + { .compatible = "qcom,sc7180-smmu-v2", .data = _smmu_v2_data },
> { .compatible = "qcom,sc7280-smmu-500", .data =
> _smmu_500_impl0_data },
> { .compatible = "qcom,sc8180x-smmu-500", .data =
> _smmu_500_impl0_data },
> { .compatible = "qcom,sc8280xp-smmu-500", .data =
> _smmu_500_impl0_data },
> { .compatible = "qcom,sdm630-smmu-v2", .data = _smmu_v2_data },
> { .compatible = "qcom,sdm845-smmu-v2", .data = _smmu_v2_data },
> { .compatible = "qcom,sdm845-smmu-500", .data = _smmu_500_data
> },
> { .compatible = "qcom,sm6115-smmu-500", .data =
> _smmu_500_impl0_data},
> { .compatible = "qcom,sm6125-smmu-500", .data =
> _smmu_500_impl0_data },
> { .compatible = "qcom,sm6350-smmu-v2", .data = _smmu_v2_data },
> { .compatible = "qcom,sm6350-smmu-500", .data =
> _smmu_500_impl0_data },
> @@ -540,12 +541,14 @@ struct arm_smmu_device *qcom_smmu_impl_init(struct
> arm_smmu_device *smmu)
> /* Match platform for ACPI boot */
> if (acpi_match_platform_list(qcom_acpi_platlist) >= 0)
> return qcom_smmu_create(smmu,
> _smmu_500_impl0_data);
> }
> #endif
>
> match = of_match_node(qcom_smmu_impl_of_match, np);
> if (match)
> return qcom_smmu_create(smmu, match->data);
>
> + WARN_ON(of_device_is_compatible(np, "qcom,adreno-smmu"));
Could you please add a comment here, noting the reason? Or maybe we
should change that to:
if (WARN_ON(...))
return ERR_PTR(-EINVAL);
> +
> return smmu;
> }
> --
> 2.40.1
>
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH 1/2] iommu/arm-smmu-qcom: Fix missing adreno_smmu's
On Tue, May 9, 2023 at 9:37 AM Rob Clark wrote:
>
> From: Rob Clark
>
> When the special handling of qcom,adreno-smmu was moved into
> qcom_smmu_create(), it was overlooked that we didn't have all the
> required entries in qcom_smmu_impl_of_match. So we stopped getting
> adreno_smmu_priv on sc7180, breaking per-process pgtables.
>
> Fixes: 30b912a03d91 ("iommu/arm-smmu-qcom: Move the qcom,adreno-smmu check
> into qcom_smmu_create")
> Suggested-by: Lepton Wu
> Signed-off-by: Rob Clark
Any chance I could get an ack for landing this fix via msm-fixes?
Broken per-process pgtables is kind of a serious regression..
BR,
-R
> ---
> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index d1b296b95c86..760d9c43dbd2 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -496,20 +496,21 @@ static const struct qcom_smmu_match_data
> qcom_smmu_500_impl0_data = {
> /*
> * Do not add any more qcom,SOC-smmu-500 entries to this list, unless they
> need
> * special handling and can not be covered by the qcom,smmu-500 entry.
> */
> static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = {
> { .compatible = "qcom,msm8996-smmu-v2", .data = _smmu_data },
> { .compatible = "qcom,msm8998-smmu-v2", .data = _smmu_v2_data },
> { .compatible = "qcom,qcm2290-smmu-500", .data =
> _smmu_500_impl0_data },
> { .compatible = "qcom,qdu1000-smmu-500", .data =
> _smmu_500_impl0_data },
> { .compatible = "qcom,sc7180-smmu-500", .data =
> _smmu_500_impl0_data },
> + { .compatible = "qcom,sc7180-smmu-v2", .data = _smmu_v2_data },
> { .compatible = "qcom,sc7280-smmu-500", .data =
> _smmu_500_impl0_data },
> { .compatible = "qcom,sc8180x-smmu-500", .data =
> _smmu_500_impl0_data },
> { .compatible = "qcom,sc8280xp-smmu-500", .data =
> _smmu_500_impl0_data },
> { .compatible = "qcom,sdm630-smmu-v2", .data = _smmu_v2_data },
> { .compatible = "qcom,sdm845-smmu-v2", .data = _smmu_v2_data },
> { .compatible = "qcom,sdm845-smmu-500", .data = _smmu_500_data
> },
> { .compatible = "qcom,sm6115-smmu-500", .data =
> _smmu_500_impl0_data},
> { .compatible = "qcom,sm6125-smmu-500", .data =
> _smmu_500_impl0_data },
> { .compatible = "qcom,sm6350-smmu-v2", .data = _smmu_v2_data },
> { .compatible = "qcom,sm6350-smmu-500", .data =
> _smmu_500_impl0_data },
> @@ -540,12 +541,14 @@ struct arm_smmu_device *qcom_smmu_impl_init(struct
> arm_smmu_device *smmu)
> /* Match platform for ACPI boot */
> if (acpi_match_platform_list(qcom_acpi_platlist) >= 0)
> return qcom_smmu_create(smmu,
> _smmu_500_impl0_data);
> }
> #endif
>
> match = of_match_node(qcom_smmu_impl_of_match, np);
> if (match)
> return qcom_smmu_create(smmu, match->data);
>
> + WARN_ON(of_device_is_compatible(np, "qcom,adreno-smmu"));
> +
> return smmu;
> }
> --
> 2.40.1
>
Re: [Freedreno] [PATCH v9 1/8] drm/display/dsc: Add flatness and initial scale value calculations
On Wed, May 10, 2023 at 03:54:41PM -0700, Jessica Zhang wrote:
> Add helpers to calculate det_thresh_flatness and initial_scale_value as
> these calculations are defined within the DSC spec.
>
> Signed-off-by: Jessica Zhang
> Signed-off-by: Dmitry Baryshkov
> Reviewed-by: Marijn Suijten
> Signed-off-by: Jessica Zhang
> ---
> include/drm/display/drm_dsc_helper.h | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/include/drm/display/drm_dsc_helper.h
> b/include/drm/display/drm_dsc_helper.h
> index 0bb0c3afd740..422135a33d65 100644
> --- a/include/drm/display/drm_dsc_helper.h
> +++ b/include/drm/display/drm_dsc_helper.h
> @@ -25,5 +25,16 @@ void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config
> *vdsc_cfg);
> int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum
> drm_dsc_params_kind kind);
> int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);
>
> +static inline void drm_dsc_set_initial_scale_value(struct drm_dsc_config
> *dsc)
> +{
> + dsc->initial_scale_value = 8 * dsc->rc_model_size /
> + (dsc->rc_model_size - dsc->initial_offset);
> +}
I would suggest using pure functions whenever possible. Makes it much
easier to reason about the code when you know there are no side effects.
> +
> +static inline int drm_dsc_calculate_flatness_det_thresh(struct
> drm_dsc_config *dsc)
'dsc' can be const. The word "calculate" seems pretty much redundant.
> +{
> + return 2 << (dsc->bits_per_component - 8);
> +}
> +
> #endif /* _DRM_DSC_HELPER_H_ */
>
>
> --
> 2.40.1
--
Ville Syrjälä
Intel
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 2/8] drm/display/dsc: add helper to set semi-const parameters
On 11/05/2023 09:18, Marijn Suijten wrote: On 2023-05-11 07:26:28, Dmitry Baryshkov wrote: On 11/05/2023 01:35, Jessica Zhang wrote: On 5/9/2023 11:29 PM, Marijn Suijten wrote: On 2023-05-09 15:06:48, Jessica Zhang wrote: From: Dmitry Baryshkov Add a helper setting config values which are typically constant across operating modes (table E-4 of the standard) and mux_word_size (which is a const according to 3.5.2). Signed-off-by: Dmitry Baryshkov Signed-off-by: Jessica Zhang Same question about ordering. Hi Marijn, This patch was authored by Dmitry and originally part of his DRM DSC helpers series [1], but was removed from that series for mergeability reasons. Looking over the kernel documentation, the last Signed-off-by should be from the patch submitter [2], so I think my s-o-b tag should be at the bottom. That's true, but I also think the S-o-B at the top should match the From: author. I'd say, this is usual, but not the required order of tags. As for the order in the previous patch, I can add a duplicate s-o-b before Dmitry's so that it reflects the history of the patch. I think this is an overkill. Instead you can drop my SOB from the patch 1. We do not need this level of detail. For this patch the ordering of tags is correct. So indeed, that either means duplicating the S-o-B or dropping it entirely as we do not care that it was part of that series earlier. Dmitry will likely sign this off once again when picking the patches. Yes. I'd suggest the following tags: Patch 1 (Add flatness...): From: Jessica SOB: Jessica Patches 2 (add helper to set) and 3 (use DRM DSC helpers): From: Dmitry SOB: Dmitry SOB: Jessica - Marijn -- With best wishes Dmitry
Re: [Freedreno] [PATCH v7 2/8] drm/display/dsc: add helper to set semi-const parameters
On 2023-05-11 07:26:28, Dmitry Baryshkov wrote: > On 11/05/2023 01:35, Jessica Zhang wrote: > > > > > > On 5/9/2023 11:29 PM, Marijn Suijten wrote: > >> On 2023-05-09 15:06:48, Jessica Zhang wrote: > >>> From: Dmitry Baryshkov > >>> > >>> Add a helper setting config values which are typically constant across > >>> operating modes (table E-4 of the standard) and mux_word_size (which is > >>> a const according to 3.5.2). > >>> > >>> Signed-off-by: Dmitry Baryshkov > >>> Signed-off-by: Jessica Zhang > >> > >> Same question about ordering. > > > > Hi Marijn, > > > > This patch was authored by Dmitry and originally part of his DRM DSC > > helpers series [1], but was removed from that series for mergeability > > reasons. > > > > Looking over the kernel documentation, the last Signed-off-by should be > > from the patch submitter [2], so I think my s-o-b tag should be at the > > bottom. That's true, but I also think the S-o-B at the top should match the From: author. > > As for the order in the previous patch, I can add a duplicate s-o-b > > before Dmitry's so that it reflects the history of the patch. > > I think this is an overkill. Instead you can drop my SOB from the patch > 1. We do not need this level of detail. > > For this patch the ordering of tags is correct. So indeed, that either means duplicating the S-o-B or dropping it entirely as we do not care that it was part of that series earlier. Dmitry will likely sign this off once again when picking the patches. - Marijn
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:
