Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD
On Tue, Apr 09, 2024 at 01:07:57AM +0300, Dmitry Baryshkov wrote: > On Tue, 9 Apr 2024 at 00:23, Abhinav Kumar wrote: > > On 4/8/2024 2:12 PM, Dmitry Baryshkov wrote: > > > On Mon, 8 Apr 2024 at 22:43, Abhinav Kumar > > > wrote: > > >> On 4/7/2024 11:48 AM, Bjorn Andersson wrote: > > >>> On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote: > > From: Kuogee Hsieh > > >>> [..] > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > > b/drivers/gpu/drm/msm/dp/dp_display.c [..] > > >> > > >> I will need sometime to address that use-case as I need to see if we can > > >> handle that better and then drop the the DISCONNECT_PENDING state to > > >> address this fully. But it needs more testing. > > >> > > >> But, we will need this patch anyway because without this we will not be > > >> able to fix even the most regular and commonly seen case of basic > > >> connect/disconnect receiving complementary events. > > > > > > Hmm, no. We need to drop the HPD state machine, not to patch it. Once > > > the driver has proper detect() callback, there will be no > > > complementary events. That is a proper way to fix the code, not these > > > kinds of band-aids patches. > > > > > > > I had discussed this part too :) > > > > I totally agree we should fix .detect()'s behavior to just match cable > > connect/disconnect and not link_ready status. > > > > But that alone would not have fixed this issue. If HPD thread does not > > get scheduled and plug_handle() was not executed, .detect() would have > > still returned old status as we will update the cable status only in > > plug_handle() / unplug_handle() to have a common API between internal > > and external hpd execution. > > I think there should be just hpd_notify, which if the HPD is up, > attempts to read the DPCD. No need for separate plug/unplug_handle. > The detect() can be as simple as !drm_dp_is_branch() || sink_count != 0. > What is detect() supposed to return in the event that we have external HPD handler? The link state? While the external HPD bridge would return the HPD state? If a driver only drives the link inbetween atomic_enable() and atomic_disable() will the "connected state" then ever be reported as "connected"? (I'm sure I'm still missing pieces of this puzzle). Regards, Bjorn
Re: [PATCH v3 3/6] drm/msm/dsi: set VIDEO_COMPRESSION_MODE_CTRL_WC (fix video mode DSC)
Marijn Suijten 于2024年4月9日周二 00:45写道： > > Can we drop (fix video mode DSC) from this patch title? It looks like more > patches are required to get this done, such a mention is more something for > the > cover letter. > > We could also clarify further to "set Word Count for video-mode DSC". > Accepted. video-mode DSC is achieved with the patch set, not this specific patch.
Re: [PATCH 3/3] drm/msm/dsi: simplify connector creation
On 3/9/2024 7:09 AM, Dmitry Baryshkov wrote: Instead of having two functions, msm_dsi_manager_bridge_init() and msm_dsi_manager_ext_bridge_init(), merge them into msm_dsi_manager_connector_init(), moving drm_bridge_attach() to be called from the bridge's attach callback (as most other bridges do). Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/dsi/dsi.c | 10 + drivers/gpu/drm/msm/dsi/dsi.h | 5 ++--- drivers/gpu/drm/msm/dsi/dsi_manager.c | 41 +++ 3 files changed, 21 insertions(+), 35 deletions(-) LGTM, Reviewed-by: Abhinav Kumar
Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD
On Tue, 9 Apr 2024 at 00:17, Abhinav Kumar wrote:
>
>
>
> On 4/8/2024 2:13 PM, Dmitry Baryshkov wrote:
> > On Tue, 9 Apr 2024 at 00:08, Abhinav Kumar
> > wrote:
> >>
> >>
> >>
> >> On 4/8/2024 1:41 PM, Bjorn Andersson wrote:
> >>> On Mon, Apr 08, 2024 at 12:43:34PM -0700, Abhinav Kumar wrote:
>
>
> On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
> > On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
> >> From: Kuogee Hsieh
> > [..]
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >> b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index d80f89581760..bfb6dfff27e8 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge
> >> *bridge,
> >> return;
> >> if (!dp_display->link_ready && status ==
> >> connector_status_connected)
> >> - dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
> >> + dp_hpd_plug_handle(dp, 0);
> >
> > If I read the code correctly, and we get an external connect event
> > inbetween a previous disconnect and the related disable call, this
> > should result in a PLUG_INT being injected into the queue still.
> >
> > Will that not cause the same problem?
> >
> > Regards,
> > Bjorn
> >
>
> Yes, your observation is correct and I had asked the same question to
> kuogee
> before taking over this change and posting.
>
> We will have to handle that case separately. I don't have a good solution
> yet for it without requiring further rework or we drop the below snippet.
>
> if (state == ST_DISCONNECT_PENDING) {
> /* wait until ST_DISCONNECTED */
> dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1
> */
> mutex_unlock(>event_mutex);
> return 0;
> }
>
> I will need sometime to address that use-case as I need to see if we can
> handle that better and then drop the the DISCONNECT_PENDING state to
> address
> this fully. But it needs more testing.
>
> But, we will need this patch anyway because without this we will not be
> able
> to fix even the most regular and commonly seen case of basic
> connect/disconnect receiving complementary events.
>
> >>>
> >>> I did some more testing on this patch. Connecting and disconnecting the
> >>> cable while in fbcon works reliably, except for:
> >>
> >> Thanks for the tests !
> >>
> >>> - edid/modes are not read before we bring up the link so I always end up
> >>> with 640x480
> >>>
> >>
> >> hmmm, I wonder why this should be affected due to this patch. We always
> >> read the EDID during hpd_connect() and the selected resolution will be
> >> programmed with the modeset. We will retry this with our x1e80100 and see.
> >
> > BTW, why is EDID read during HPD handling? I always supposed that it
> > can be read much later, when the DRM framework calls the get_modes /
> > get_edid callbacks.
> >
>
> Well, dp_panel_read_sink_caps() is in dp_display_process_hpd_high()
> currently. We read the edid there.
My question was, why is it done this way? Can it be dropped? There is
no need to store EDID in the driver data, is it?
>
> get_modes(), parses the EDID and adds the modes using drm_add_edid_modes().
>
> >>
> >>> - if I run modetest -s : the link is brought up with the new
> >>> resolution and I get my test image on the screen.
> >>> But as we're shutting down the link for the resolution chance I end up
> >>> in dp_bridge_hpd_notify() with link_ready && state = disconnected.
> >>> This triggers an unplug which hangs on the event_mutex, such that as
> >>> soon as I get the test image, the state machine enters
> >>> DISCONNECT_PENDING. This is immediately followed by another
> >>> !link_ready && status = connected, which attempts to perform the plug
> >>> operation, but as we're in DISCONNECT_PENDING this is posted on the
> >>> event queue. From there I get a log entry from my PLUG_INT, every
> >>> 100ms stating that we're still in DISCONNECT_PENDING. If I exit
> >>> modetest the screen goes black, and no new mode can be selected,
> >>> because we're in DISCONNECT_PENDING. The only way out is to disconnect
> >>> the cable to complete the DISCONNECT_PENDING.
> >>>
> >>
> >> I am going to run this test-case and see what we can do.
> >>
> >>> Regards,
> >>> Bjorn
> >>>
>
> >> else if (dp_display->link_ready && status ==
> >> connector_status_disconnected)
> >> - dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
> >> + dp_hpd_unplug_handle(dp, 0);
> >> }
> >> --
> >> 2.43.2
> >>
> >
> >
> >
--
With best wishes
Dmitry
Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD
On Tue, 9 Apr 2024 at 00:23, Abhinav Kumar wrote:
>
>
>
> On 4/8/2024 2:12 PM, Dmitry Baryshkov wrote:
> > On Mon, 8 Apr 2024 at 22:43, Abhinav Kumar
> > wrote:
> >>
> >>
> >>
> >> On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
> >>> On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
> From: Kuogee Hsieh
> >>> [..]
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index d80f89581760..bfb6dfff27e8 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge
> *bridge,
> return;
>
> if (!dp_display->link_ready && status ==
> connector_status_connected)
> -dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
> +dp_hpd_plug_handle(dp, 0);
> >>>
> >>> If I read the code correctly, and we get an external connect event
> >>> inbetween a previous disconnect and the related disable call, this
> >>> should result in a PLUG_INT being injected into the queue still.
> >>>
> >>> Will that not cause the same problem?
> >>>
> >>> Regards,
> >>> Bjorn
> >>>
> >>
> >> Yes, your observation is correct and I had asked the same question to
> >> kuogee before taking over this change and posting.
> >
> > Should it then have the Co-developed-by trailers?
> >
>
> hmmm, perhaps but that didnt result in any code change between v2 and
> v3, so I didnt add it.
So who authored v0 of it? From your text I had an impression that it
was Kuogee. Please excuse me if I was wrong.
>
> >> We will have to handle that case separately. I don't have a good
> >> solution yet for it without requiring further rework or we drop the
> >> below snippet.
> >>
> >> if (state == ST_DISCONNECT_PENDING) {
> >> /* wait until ST_DISCONNECTED */
> >> dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
> >> mutex_unlock(>event_mutex);
> >> return 0;
> >> }
> >>
> >> I will need sometime to address that use-case as I need to see if we can
> >> handle that better and then drop the the DISCONNECT_PENDING state to
> >> address this fully. But it needs more testing.
> >>
> >> But, we will need this patch anyway because without this we will not be
> >> able to fix even the most regular and commonly seen case of basic
> >> connect/disconnect receiving complementary events.
> >
> > Hmm, no. We need to drop the HPD state machine, not to patch it. Once
> > the driver has proper detect() callback, there will be no
> > complementary events. That is a proper way to fix the code, not these
> > kinds of band-aids patches.
> >
>
> I had discussed this part too :)
>
> I totally agree we should fix .detect()'s behavior to just match cable
> connect/disconnect and not link_ready status.
>
> But that alone would not have fixed this issue. If HPD thread does not
> get scheduled and plug_handle() was not executed, .detect() would have
> still returned old status as we will update the cable status only in
> plug_handle() / unplug_handle() to have a common API between internal
> and external hpd execution.
I think there should be just hpd_notify, which if the HPD is up,
attempts to read the DPCD. No need for separate plug/unplug_handle.
The detect() can be as simple as !drm_dp_is_branch() || sink_count != 0.
>
> So we need to do both, make .detect() return correct status AND drop hpd
> event thread processing.
>
> But, dropping the hpd event thread processing alone was fixing the
> complimentary events issue. So kuogee had only this part in this patch.
I'd prefer to wait for the final patchset then. Which has the HPD
thread completely removed.
>
>
> else if (dp_display->link_ready && status ==
> connector_status_disconnected)
> -dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
> +dp_hpd_unplug_handle(dp, 0);
> }
> --
> 2.43.2
>
> >
> >
> >
--
With best wishes
Dmitry
Re: [PATCH 2/3] drm/msm/dsi: move next bridge acquisition to dsi_bind
On 4/5/2024 11:15 AM, Dmitry Baryshkov wrote:
On Fri, 5 Apr 2024 at 20:35, Abhinav Kumar wrote:
On 3/9/2024 7:09 AM, Dmitry Baryshkov wrote:
Currently the MSM DSI driver looks for the next bridge during
msm_dsi_modeset_init(). If the bridge is not registered at that point,
this might result in -EPROBE_DEFER, which can be problematic that late
during the device probe process. Move next bridge acquisition to the
dsi_bind state so that probe deferral is returned as early as possible.
But msm_dsi_modeset)init() is also called during msm_drm_bind() only.
What issue are you suggesting will be fixed by moving this from
msm_drm_bind() to dsi_bind()?
The goal is to return as early as possible as not not cause
probe-deferral loops. See commit fbc35b45f9f6 ("Add documentation on
meaning of -EPROBE_DEFER"). It discusses returning from probe() but
the same logic applies to bind().
Understood. I was trying to make sure the purpose of the patch is that
"deferral in component_bind() is better than component_master_bind()"
But yes, overall that is better since the unbounding path will be more
in the master case.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dsi/dsi.c | 16
drivers/gpu/drm/msm/dsi/dsi.h | 2 ++
drivers/gpu/drm/msm/dsi/dsi_manager.c | 8 +---
3 files changed, 19 insertions(+), 7 deletions(-)
Reviewed-by: Abhinav Kumar
Re: [PATCH v2 6/6] drm/msm/dp: Use function arguments for audio operations
On 3/28/2024 7:40 AM, Bjorn Andersson wrote: From: Bjorn Andersson The dp_audio read and write operations uses members in struct dp_catalog for passing arguments and return values. This adds unnecessary complexity to the implementation, as it turns out after detangling the logic that no state is actually held in these variables. Clean this up by using function arguments and return values for passing the data. Reviewed-by: Dmitry Baryshkov Signed-off-by: Bjorn Andersson --- drivers/gpu/drm/msm/dp/dp_audio.c | 20 +-- drivers/gpu/drm/msm/dp/dp_catalog.c | 39 + drivers/gpu/drm/msm/dp/dp_catalog.h | 18 + 3 files changed, 28 insertions(+), 49 deletions(-) One quick question, was DP audio re-tested after this patch?
Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD
On 4/8/2024 2:12 PM, Dmitry Baryshkov wrote:
On Mon, 8 Apr 2024 at 22:43, Abhinav Kumar wrote:
On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
From: Kuogee Hsieh
[..]
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index d80f89581760..bfb6dfff27e8 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
return;
if (!dp_display->link_ready && status == connector_status_connected)
-dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
+dp_hpd_plug_handle(dp, 0);
If I read the code correctly, and we get an external connect event
inbetween a previous disconnect and the related disable call, this
should result in a PLUG_INT being injected into the queue still.
Will that not cause the same problem?
Regards,
Bjorn
Yes, your observation is correct and I had asked the same question to
kuogee before taking over this change and posting.
Should it then have the Co-developed-by trailers?
hmmm, perhaps but that didnt result in any code change between v2 and
v3, so I didnt add it.
We will have to handle that case separately. I don't have a good
solution yet for it without requiring further rework or we drop the
below snippet.
if (state == ST_DISCONNECT_PENDING) {
/* wait until ST_DISCONNECTED */
dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
mutex_unlock(>event_mutex);
return 0;
}
I will need sometime to address that use-case as I need to see if we can
handle that better and then drop the the DISCONNECT_PENDING state to
address this fully. But it needs more testing.
But, we will need this patch anyway because without this we will not be
able to fix even the most regular and commonly seen case of basic
connect/disconnect receiving complementary events.
Hmm, no. We need to drop the HPD state machine, not to patch it. Once
the driver has proper detect() callback, there will be no
complementary events. That is a proper way to fix the code, not these
kinds of band-aids patches.
I had discussed this part too :)
I totally agree we should fix .detect()'s behavior to just match cable
connect/disconnect and not link_ready status.
But that alone would not have fixed this issue. If HPD thread does not
get scheduled and plug_handle() was not executed, .detect() would have
still returned old status as we will update the cable status only in
plug_handle() / unplug_handle() to have a common API between internal
and external hpd execution.
So we need to do both, make .detect() return correct status AND drop hpd
event thread processing.
But, dropping the hpd event thread processing alone was fixing the
complimentary events issue. So kuogee had only this part in this patch.
else if (dp_display->link_ready && status ==
connector_status_disconnected)
-dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
+dp_hpd_unplug_handle(dp, 0);
}
--
2.43.2
Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD
On 4/8/2024 2:13 PM, Dmitry Baryshkov wrote:
On Tue, 9 Apr 2024 at 00:08, Abhinav Kumar wrote:
On 4/8/2024 1:41 PM, Bjorn Andersson wrote:
On Mon, Apr 08, 2024 at 12:43:34PM -0700, Abhinav Kumar wrote:
On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
From: Kuogee Hsieh
[..]
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index d80f89581760..bfb6dfff27e8 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
return;
if (!dp_display->link_ready && status == connector_status_connected)
- dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
+ dp_hpd_plug_handle(dp, 0);
If I read the code correctly, and we get an external connect event
inbetween a previous disconnect and the related disable call, this
should result in a PLUG_INT being injected into the queue still.
Will that not cause the same problem?
Regards,
Bjorn
Yes, your observation is correct and I had asked the same question to kuogee
before taking over this change and posting.
We will have to handle that case separately. I don't have a good solution
yet for it without requiring further rework or we drop the below snippet.
if (state == ST_DISCONNECT_PENDING) {
/* wait until ST_DISCONNECTED */
dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
mutex_unlock(>event_mutex);
return 0;
}
I will need sometime to address that use-case as I need to see if we can
handle that better and then drop the the DISCONNECT_PENDING state to address
this fully. But it needs more testing.
But, we will need this patch anyway because without this we will not be able
to fix even the most regular and commonly seen case of basic
connect/disconnect receiving complementary events.
I did some more testing on this patch. Connecting and disconnecting the
cable while in fbcon works reliably, except for:
Thanks for the tests !
- edid/modes are not read before we bring up the link so I always end up
with 640x480
hmmm, I wonder why this should be affected due to this patch. We always
read the EDID during hpd_connect() and the selected resolution will be
programmed with the modeset. We will retry this with our x1e80100 and see.
BTW, why is EDID read during HPD handling? I always supposed that it
can be read much later, when the DRM framework calls the get_modes /
get_edid callbacks.
Well, dp_panel_read_sink_caps() is in dp_display_process_hpd_high()
currently. We read the edid there.
get_modes(), parses the EDID and adds the modes using drm_add_edid_modes().
- if I run modetest -s : the link is brought up with the new
resolution and I get my test image on the screen.
But as we're shutting down the link for the resolution chance I end up
in dp_bridge_hpd_notify() with link_ready && state = disconnected.
This triggers an unplug which hangs on the event_mutex, such that as
soon as I get the test image, the state machine enters
DISCONNECT_PENDING. This is immediately followed by another
!link_ready && status = connected, which attempts to perform the plug
operation, but as we're in DISCONNECT_PENDING this is posted on the
event queue. From there I get a log entry from my PLUG_INT, every
100ms stating that we're still in DISCONNECT_PENDING. If I exit
modetest the screen goes black, and no new mode can be selected,
because we're in DISCONNECT_PENDING. The only way out is to disconnect
the cable to complete the DISCONNECT_PENDING.
I am going to run this test-case and see what we can do.
Regards,
Bjorn
else if (dp_display->link_ready && status ==
connector_status_disconnected)
- dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
+ dp_hpd_unplug_handle(dp, 0);
}
--
2.43.2
Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD
On Tue, 9 Apr 2024 at 00:08, Abhinav Kumar wrote:
>
>
>
> On 4/8/2024 1:41 PM, Bjorn Andersson wrote:
> > On Mon, Apr 08, 2024 at 12:43:34PM -0700, Abhinav Kumar wrote:
> >>
> >>
> >> On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
> >>> On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
> From: Kuogee Hsieh
> >>> [..]
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index d80f89581760..bfb6dfff27e8 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge
> *bridge,
> return;
> if (!dp_display->link_ready && status ==
> connector_status_connected)
> - dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
> + dp_hpd_plug_handle(dp, 0);
> >>>
> >>> If I read the code correctly, and we get an external connect event
> >>> inbetween a previous disconnect and the related disable call, this
> >>> should result in a PLUG_INT being injected into the queue still.
> >>>
> >>> Will that not cause the same problem?
> >>>
> >>> Regards,
> >>> Bjorn
> >>>
> >>
> >> Yes, your observation is correct and I had asked the same question to
> >> kuogee
> >> before taking over this change and posting.
> >>
> >> We will have to handle that case separately. I don't have a good solution
> >> yet for it without requiring further rework or we drop the below snippet.
> >>
> >> if (state == ST_DISCONNECT_PENDING) {
> >> /* wait until ST_DISCONNECTED */
> >> dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
> >> mutex_unlock(>event_mutex);
> >> return 0;
> >> }
> >>
> >> I will need sometime to address that use-case as I need to see if we can
> >> handle that better and then drop the the DISCONNECT_PENDING state to
> >> address
> >> this fully. But it needs more testing.
> >>
> >> But, we will need this patch anyway because without this we will not be
> >> able
> >> to fix even the most regular and commonly seen case of basic
> >> connect/disconnect receiving complementary events.
> >>
> >
> > I did some more testing on this patch. Connecting and disconnecting the
> > cable while in fbcon works reliably, except for:
>
> Thanks for the tests !
>
> > - edid/modes are not read before we bring up the link so I always end up
> >with 640x480
> >
>
> hmmm, I wonder why this should be affected due to this patch. We always
> read the EDID during hpd_connect() and the selected resolution will be
> programmed with the modeset. We will retry this with our x1e80100 and see.
BTW, why is EDID read during HPD handling? I always supposed that it
can be read much later, when the DRM framework calls the get_modes /
get_edid callbacks.
>
> > - if I run modetest -s : the link is brought up with the new
> >resolution and I get my test image on the screen.
> >But as we're shutting down the link for the resolution chance I end up
> >in dp_bridge_hpd_notify() with link_ready && state = disconnected.
> >This triggers an unplug which hangs on the event_mutex, such that as
> >soon as I get the test image, the state machine enters
> >DISCONNECT_PENDING. This is immediately followed by another
> >!link_ready && status = connected, which attempts to perform the plug
> >operation, but as we're in DISCONNECT_PENDING this is posted on the
> >event queue. From there I get a log entry from my PLUG_INT, every
> >100ms stating that we're still in DISCONNECT_PENDING. If I exit
> >modetest the screen goes black, and no new mode can be selected,
> >because we're in DISCONNECT_PENDING. The only way out is to disconnect
> >the cable to complete the DISCONNECT_PENDING.
> >
>
> I am going to run this test-case and see what we can do.
>
> > Regards,
> > Bjorn
> >
> >>
> else if (dp_display->link_ready && status ==
> connector_status_disconnected)
> - dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
> + dp_hpd_unplug_handle(dp, 0);
> }
> --
> 2.43.2
>
--
With best wishes
Dmitry
Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD
On Mon, 8 Apr 2024 at 22:43, Abhinav Kumar wrote:
>
>
>
> On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
> > On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
> >> From: Kuogee Hsieh
> > [..]
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >> b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index d80f89581760..bfb6dfff27e8 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
> >> return;
> >>
> >> if (!dp_display->link_ready && status == connector_status_connected)
> >> -dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
> >> +dp_hpd_plug_handle(dp, 0);
> >
> > If I read the code correctly, and we get an external connect event
> > inbetween a previous disconnect and the related disable call, this
> > should result in a PLUG_INT being injected into the queue still.
> >
> > Will that not cause the same problem?
> >
> > Regards,
> > Bjorn
> >
>
> Yes, your observation is correct and I had asked the same question to
> kuogee before taking over this change and posting.
Should it then have the Co-developed-by trailers?
> We will have to handle that case separately. I don't have a good
> solution yet for it without requiring further rework or we drop the
> below snippet.
>
> if (state == ST_DISCONNECT_PENDING) {
> /* wait until ST_DISCONNECTED */
> dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
> mutex_unlock(>event_mutex);
> return 0;
> }
>
> I will need sometime to address that use-case as I need to see if we can
> handle that better and then drop the the DISCONNECT_PENDING state to
> address this fully. But it needs more testing.
>
> But, we will need this patch anyway because without this we will not be
> able to fix even the most regular and commonly seen case of basic
> connect/disconnect receiving complementary events.
Hmm, no. We need to drop the HPD state machine, not to patch it. Once
the driver has proper detect() callback, there will be no
complementary events. That is a proper way to fix the code, not these
kinds of band-aids patches.
> >> else if (dp_display->link_ready && status ==
> >> connector_status_disconnected)
> >> -dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
> >> +dp_hpd_unplug_handle(dp, 0);
> >> }
> >> --
> >> 2.43.2
> >>
--
With best wishes
Dmitry
Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD
On 4/8/2024 1:41 PM, Bjorn Andersson wrote:
On Mon, Apr 08, 2024 at 12:43:34PM -0700, Abhinav Kumar wrote:
On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
From: Kuogee Hsieh
[..]
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index d80f89581760..bfb6dfff27e8 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
return;
if (!dp_display->link_ready && status == connector_status_connected)
- dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
+ dp_hpd_plug_handle(dp, 0);
If I read the code correctly, and we get an external connect event
inbetween a previous disconnect and the related disable call, this
should result in a PLUG_INT being injected into the queue still.
Will that not cause the same problem?
Regards,
Bjorn
Yes, your observation is correct and I had asked the same question to kuogee
before taking over this change and posting.
We will have to handle that case separately. I don't have a good solution
yet for it without requiring further rework or we drop the below snippet.
if (state == ST_DISCONNECT_PENDING) {
/* wait until ST_DISCONNECTED */
dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
mutex_unlock(>event_mutex);
return 0;
}
I will need sometime to address that use-case as I need to see if we can
handle that better and then drop the the DISCONNECT_PENDING state to address
this fully. But it needs more testing.
But, we will need this patch anyway because without this we will not be able
to fix even the most regular and commonly seen case of basic
connect/disconnect receiving complementary events.
I did some more testing on this patch. Connecting and disconnecting the
cable while in fbcon works reliably, except for:
Thanks for the tests !
- edid/modes are not read before we bring up the link so I always end up
with 640x480
hmmm, I wonder why this should be affected due to this patch. We always
read the EDID during hpd_connect() and the selected resolution will be
programmed with the modeset. We will retry this with our x1e80100 and see.
- if I run modetest -s : the link is brought up with the new
resolution and I get my test image on the screen.
But as we're shutting down the link for the resolution chance I end up
in dp_bridge_hpd_notify() with link_ready && state = disconnected.
This triggers an unplug which hangs on the event_mutex, such that as
soon as I get the test image, the state machine enters
DISCONNECT_PENDING. This is immediately followed by another
!link_ready && status = connected, which attempts to perform the plug
operation, but as we're in DISCONNECT_PENDING this is posted on the
event queue. From there I get a log entry from my PLUG_INT, every
100ms stating that we're still in DISCONNECT_PENDING. If I exit
modetest the screen goes black, and no new mode can be selected,
because we're in DISCONNECT_PENDING. The only way out is to disconnect
the cable to complete the DISCONNECT_PENDING.
I am going to run this test-case and see what we can do.
Regards,
Bjorn
else if (dp_display->link_ready && status ==
connector_status_disconnected)
- dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
+ dp_hpd_unplug_handle(dp, 0);
}
--
2.43.2
Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD
On Mon, Apr 08, 2024 at 12:43:34PM -0700, Abhinav Kumar wrote:
>
>
> On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
> > On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
> > > From: Kuogee Hsieh
> > [..]
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > > b/drivers/gpu/drm/msm/dp/dp_display.c
> > > index d80f89581760..bfb6dfff27e8 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > @@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
> > > return;
> > > if (!dp_display->link_ready && status ==
> > > connector_status_connected)
> > > - dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
> > > + dp_hpd_plug_handle(dp, 0);
> >
> > If I read the code correctly, and we get an external connect event
> > inbetween a previous disconnect and the related disable call, this
> > should result in a PLUG_INT being injected into the queue still.
> >
> > Will that not cause the same problem?
> >
> > Regards,
> > Bjorn
> >
>
> Yes, your observation is correct and I had asked the same question to kuogee
> before taking over this change and posting.
>
> We will have to handle that case separately. I don't have a good solution
> yet for it without requiring further rework or we drop the below snippet.
>
> if (state == ST_DISCONNECT_PENDING) {
> /* wait until ST_DISCONNECTED */
> dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
> mutex_unlock(>event_mutex);
> return 0;
> }
>
> I will need sometime to address that use-case as I need to see if we can
> handle that better and then drop the the DISCONNECT_PENDING state to address
> this fully. But it needs more testing.
>
> But, we will need this patch anyway because without this we will not be able
> to fix even the most regular and commonly seen case of basic
> connect/disconnect receiving complementary events.
>
I did some more testing on this patch. Connecting and disconnecting the
cable while in fbcon works reliably, except for:
- edid/modes are not read before we bring up the link so I always end up
with 640x480
- if I run modetest -s : the link is brought up with the new
resolution and I get my test image on the screen.
But as we're shutting down the link for the resolution chance I end up
in dp_bridge_hpd_notify() with link_ready && state = disconnected.
This triggers an unplug which hangs on the event_mutex, such that as
soon as I get the test image, the state machine enters
DISCONNECT_PENDING. This is immediately followed by another
!link_ready && status = connected, which attempts to perform the plug
operation, but as we're in DISCONNECT_PENDING this is posted on the
event queue. From there I get a log entry from my PLUG_INT, every
100ms stating that we're still in DISCONNECT_PENDING. If I exit
modetest the screen goes black, and no new mode can be selected,
because we're in DISCONNECT_PENDING. The only way out is to disconnect
the cable to complete the DISCONNECT_PENDING.
Regards,
Bjorn
>
> > > else if (dp_display->link_ready && status ==
> > > connector_status_disconnected)
> > > - dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
> > > + dp_hpd_unplug_handle(dp, 0);
> > > }
> > > --
> > > 2.43.2
> > >
Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD
On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
From: Kuogee Hsieh
[..]
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index d80f89581760..bfb6dfff27e8 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
return;
if (!dp_display->link_ready && status == connector_status_connected)
- dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
+ dp_hpd_plug_handle(dp, 0);
If I read the code correctly, and we get an external connect event
inbetween a previous disconnect and the related disable call, this
should result in a PLUG_INT being injected into the queue still.
Will that not cause the same problem?
Regards,
Bjorn
Yes, your observation is correct and I had asked the same question to
kuogee before taking over this change and posting.
We will have to handle that case separately. I don't have a good
solution yet for it without requiring further rework or we drop the
below snippet.
if (state == ST_DISCONNECT_PENDING) {
/* wait until ST_DISCONNECTED */
dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
mutex_unlock(>event_mutex);
return 0;
}
I will need sometime to address that use-case as I need to see if we can
handle that better and then drop the the DISCONNECT_PENDING state to
address this fully. But it needs more testing.
But, we will need this patch anyway because without this we will not be
able to fix even the most regular and commonly seen case of basic
connect/disconnect receiving complementary events.
else if (dp_display->link_ready && status ==
connector_status_disconnected)
- dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
+ dp_hpd_unplug_handle(dp, 0);
}
--
2.43.2
[linux-next:master] BUILD REGRESSION 11cb68ad52ac78c81e33b806b531f097e68edfa2
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master branch HEAD: 11cb68ad52ac78c81e33b806b531f097e68edfa2 Add linux-next specific files for 20240408 Error/Warning: (recently discovered and may have been fixed) drivers/gpu/drm/drm_mm.c:152:1: error: unused function 'drm_mm_interval_tree_insert' [-Werror,-Wunused-function] drivers/gpu/drm/drm_mm.c:152:1: error: unused function 'drm_mm_interval_tree_iter_next' [-Werror,-Wunused-function] drivers/gpu/drm/lima/lima_drv.c:387:13: error: cast to smaller integer type 'enum lima_gpu_id' from 'const void *' [-Werror,-Wvoid-pointer-to-enum-cast] drivers/gpu/drm/pl111/pl111_versatile.c:488:24: error: cast to smaller integer type 'enum versatile_clcd' from 'const void *' [-Werror,-Wvoid-pointer-to-enum-cast] Error/Warning ids grouped by kconfigs: gcc_recent_errors |-- alpha-allyesconfig | |-- drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size |-- arm-allmodconfig | |-- drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size |-- arm-allyesconfig | |-- drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size |-- csky-allmodconfig | |-- drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size |-- csky-allyesconfig | |-- drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size |-- loongarch-allmodconfig | |-- drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size |-- microblaze-allmodconfig | |-- drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size |-- microblaze-allyesconfig | |-- drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size |-- openrisc-allyesconfig | |-- drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size |-- parisc-allmodconfig | |-- drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | |-- drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size | `-- drivers-gpu-drm-nouveau-nvif-object.c:error:memcpy-accessing-or-more-bytes-at-offsets-and-overlaps-bytes-at-offset |-- parisc-allyesconfig | |-- drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | |-- drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size | `-- drivers-gpu-drm-nouveau-nvif-object.c:error:memcpy-accessing-or-more-bytes-at-offsets-and-overlaps-bytes-at-offset |-- powerpc-allmodconfig | |-- drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size |-- s390-allyesconfig | |-- drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | `-- drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d
Re: [PATCH] drm: ci: fix the xfails for apq8016
Hi Helen Gentle reminder on this. If you are okay, we can land it via msm-next tree... Thanks Abhinav On 4/1/2024 1:48 PM, Abhinav Kumar wrote: After IGT migrating to dynamic sub-tests, the pipe prefixes in the expected fails list are incorrect. Lets drop those to accurately match the expected fails. In addition, update the xfails list to match the current passing list. This should have ideally failed in the CI run because some tests were marked as fail even though they passed but due to the mismatch in test names, the matching didn't correctly work and was resulting in those failures not being seen. Here is the passing pipeline for apq8016 with this change: https://gitlab.freedesktop.org/drm/msm/-/jobs/57050562 Signed-off-by: Abhinav Kumar --- drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt | 13 + 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt b/drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt index 44a5c62dedad..b14d4e884971 100644 --- a/drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt +++ b/drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt @@ -1,17 +1,6 @@ kms_3d,Fail kms_addfb_basic@addfb25-bad-modifier,Fail -kms_cursor_legacy@all-pipes-forked-bo,Fail -kms_cursor_legacy@all-pipes-forked-move,Fail -kms_cursor_legacy@all-pipes-single-bo,Fail -kms_cursor_legacy@all-pipes-single-move,Fail -kms_cursor_legacy@all-pipes-torture-bo,Fail -kms_cursor_legacy@all-pipes-torture-move,Fail -kms_cursor_legacy@pipe-A-forked-bo,Fail -kms_cursor_legacy@pipe-A-forked-move,Fail -kms_cursor_legacy@pipe-A-single-bo,Fail -kms_cursor_legacy@pipe-A-single-move,Fail -kms_cursor_legacy@pipe-A-torture-bo,Fail -kms_cursor_legacy@pipe-A-torture-move,Fail +kms_cursor_legacy@torture-bo,Fail kms_force_connector_basic@force-edid,Fail kms_hdmi_inject@inject-4k,Fail kms_selftest@drm_format,Timeout
Re: [PATCH] drm/msm/dpu: Add callback function pointer check before its call
On 4/8/2024 1:55 AM, Aleksandr Mishin wrote:
In dpu_core_irq_callback_handler() callback function pointer is compared to
NULL,
but then callback function is unconditionally called by this pointer.
Fix this bug by adding conditional return.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Yes , as dmitry wrote, this should be Reported-by.
But rest LGTM.
Fixes: c929ac60b3ed ("drm/msm/dpu: allow just single IRQ callback")
Signed-off-by: Aleksandr Mishin
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
index 946dd0135dff..03a16fbd4c99 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
@@ -223,9 +223,11 @@ static void dpu_core_irq_callback_handler(struct dpu_kms
*dpu_kms, unsigned int
VERB("IRQ=[%d, %d]\n", DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
- if (!irq_entry->cb)
+ if (!irq_entry->cb) {
DRM_ERROR("no registered cb, IRQ=[%d, %d]\n",
DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
+ return;
+ }
atomic_inc(_entry->count);
Re: [PATCH v3 3/6] drm/msm/dsi: set VIDEO_COMPRESSION_MODE_CTRL_WC (fix video mode DSC)
Can we drop (fix video mode DSC) from this patch title? It looks like more
patches are required to get this done, such a mention is more something for the
cover letter.
We could also clarify further to "set Word Count for video-mode DSC".
- Marijn
On 2024-04-03 17:10:59, Jun Nie wrote:
> From: Jonathan Marek
>
> Video mode DSC won't work if this field is not set correctly. Set it to fix
> video mode DSC (for slice_per_pkt==1 cases at least).
>
> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> Signed-off-by: Jonathan Marek
> Reviewed-by: Dmitry Baryshkov
> ---
> drivers/gpu/drm/msm/dsi/dsi_host.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 2a0422cad6de..80ea4f1d8274 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -858,6 +858,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
> *msm_host, bool is_cmd_mod
> u32 slice_per_intf, total_bytes_per_intf;
> u32 pkt_per_line;
> u32 eol_byte_num;
> + u32 bytes_per_pkt;
>
> /* first calculate dsc parameters and then program
>* compress mode registers
> @@ -865,6 +866,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
> *msm_host, bool is_cmd_mod
> slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay);
>
> total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
> + bytes_per_pkt = dsc->slice_chunk_size; /* * slice_per_pkt; */
>
> eol_byte_num = total_bytes_per_intf % 3;
>
> @@ -902,6 +904,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
> *msm_host, bool is_cmd_mod
> dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL,
> reg_ctrl);
> dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2,
> reg_ctrl2);
> } else {
> + reg |= DSI_VIDEO_COMPRESSION_MODE_CTRL_WC(bytes_per_pkt);
> dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
> }
> }
>
> --
> 2.34.1
>
Re: [PATCH v3 5/6] drm/display: Add slice_per_pkt for dsc
On 2024-04-08 17:58:29, Jun Nie wrote:
> Dmitry Baryshkov 于2024年4月3日周三 17:41写道：
> >
> > On Wed, 3 Apr 2024 at 12:11, Jun Nie wrote:
> > >
> > > Add variable for slice number of a DSC compression bit stream packet.
> > > Its value shall be specified in panel driver, or default value can be set
> > > in display controller driver if panel driver does not set it.
> >
> > This is not a part of the standard. Please justify it.
>
> Right, I read the standard but did not find any details of packet description.
> Looks like msm silicon support tuning of number of slice packing per
> downstream
> code.
> The slice_per_pkt can be set in the downstream msm device tree. And I test the
> values 1 and 2 on vtdr6130 panel and both work. So I guess this is related to
> performance or something like that. I will have more test with different panel
> to check the impact.
> drivers/gpu/drm/panel/panel-raydium-rm692e5.c also mentions to pass new value
> to slice_per_pkt.
>
> Hi Konrad,
> Do you remember why value 2 is TODO for slice_per_pkt for panel rm692e5?
Hi Jun,
I think I should indirectly answer that question, as I indirectly via "the"
MDSS panel generator place that comment there based on the suggested downstream
value:
https://github.com/msm8916-mainline/linux-mdss-dsi-panel-driver-generator/commit/5c82e613d987d05feca423412f6de625f9c99bae#diff-dba3766d7cec900b8de500f888c64a392cd9780f9baf00aae7e3f87a7d3fefc4R458
So I don't think Konrad's answer will be any different than "that's what
downstream does, and that's what the generator put there".
---
I was fairly certain that it used for performance reasons, but panels were found
(e.g. on the FairPhone 5) that don't seem to function without combining multiple
(2) slices in one packet at all?
- Marijn
> > > Signed-off-by: Jun Nie
> > > ---
> > > include/drm/display/drm_dsc.h | 4
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/include/drm/display/drm_dsc.h b/include/drm/display/drm_dsc.h
> > > index bc90273d06a6..4fac0a2746ae 100644
> > > --- a/include/drm/display/drm_dsc.h
> > > +++ b/include/drm/display/drm_dsc.h
> > > @@ -82,6 +82,10 @@ struct drm_dsc_config {
> > > * @bits_per_component: Bits per component to code (8/10/12)
> > > */
> > > u8 bits_per_component;
> > > + /**
> > > +* @slice_per_pkt: slice number per DSC bit stream packet
> > > +*/
> > > + u8 slice_per_pkt;
> > > /**
> > > * @convert_rgb:
> > > * Flag to indicate if RGB - YCoCg conversion is needed
> > >
> > > --
> > > 2.34.1
> > >
> >
> >
> > --
> > With best wishes
> > Dmitry
Re: [PATCH v1 1/1] drm/msm/hdmi: Replace of_gpio.h by proper one
On Mon, 4 Mar 2024 at 19:51, Andy Shevchenko wrote: > > of_gpio.h is deprecated and subject to remove. > The driver doesn't use it directly, replace it > with what is really being used. > > Signed-off-by: Andy Shevchenko > --- > drivers/gpu/drm/msm/hdmi/hdmi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c > index c8ebd75176bb..24abcb7254cc 100644 > --- a/drivers/gpu/drm/msm/hdmi/hdmi.c > +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c > @@ -5,8 +5,8 @@ > * Author: Rob Clark > */ > > +#include > #include > -#include > #include > #include > > -- > 2.43.0.rc1.1.gbec44491f096 > First one didn't reach the PW, let's try again: Reviewed-by: Dmitry Baryshkov -- With best wishes Dmitry
[PATCH] drm/msm/dpu: Add callback function pointer check before its call
In dpu_core_irq_callback_handler() callback function pointer is compared to
NULL,
but then callback function is unconditionally called by this pointer.
Fix this bug by adding conditional return.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: c929ac60b3ed ("drm/msm/dpu: allow just single IRQ callback")
Signed-off-by: Aleksandr Mishin
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
index 946dd0135dff..03a16fbd4c99 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
@@ -223,9 +223,11 @@ static void dpu_core_irq_callback_handler(struct dpu_kms
*dpu_kms, unsigned int
VERB("IRQ=[%d, %d]\n", DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
- if (!irq_entry->cb)
+ if (!irq_entry->cb) {
DRM_ERROR("no registered cb, IRQ=[%d, %d]\n",
DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
+ return;
+ }
atomic_inc(_entry->count);
--
2.30.2
Re: [PATCH v3 6/6] drm/msm/dsi: support DSC configurations with slice_per_pkt > 1
Dmitry Baryshkov 于2024年4月3日周三 18:51写道： > > On Wed, 3 Apr 2024 at 12:11, Jun Nie wrote: > > > > From: Jonathan Marek > > > > Support slice_per_pkt in msm driver. > > > > Note that the removed "pkt_per_line = slice_per_intf * slice_per_pkt" > > comment is incorrect. > > > > Also trim the code to simplify the dsc reference. > > > > Signed-off-by: Jonathan Marek > > Signed-off-by: Jun Nie > > --- > > drivers/gpu/drm/msm/dsi/dsi_host.c | 35 ++- > > 1 file changed, 14 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c > > b/drivers/gpu/drm/msm/dsi/dsi_host.c > > index b0507a42ee6a..0c6f40dbd25c 100644 > > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > > @@ -866,17 +866,10 @@ static void dsi_update_dsc_timing(struct msm_dsi_host > > *msm_host, bool is_cmd_mod > > slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay); > > > > total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf; > > - bytes_per_pkt = dsc->slice_chunk_size; /* * slice_per_pkt; */ > > - > > + bytes_per_pkt = dsc->slice_chunk_size * dsc->slice_per_pkt; > > Please don't mix cleanup and functional changes. OK. Will fix this.
Re: [PATCH v3 5/6] drm/display: Add slice_per_pkt for dsc
Dmitry Baryshkov 于2024年4月3日周三 17:41写道：
>
> On Wed, 3 Apr 2024 at 12:11, Jun Nie wrote:
> >
> > Add variable for slice number of a DSC compression bit stream packet.
> > Its value shall be specified in panel driver, or default value can be set
> > in display controller driver if panel driver does not set it.
>
> This is not a part of the standard. Please justify it.
Right, I read the standard but did not find any details of packet description.
Looks like msm silicon support tuning of number of slice packing per downstream
code.
The slice_per_pkt can be set in the downstream msm device tree. And I test the
values 1 and 2 on vtdr6130 panel and both work. So I guess this is related to
performance or something like that. I will have more test with different panel
to check the impact.
drivers/gpu/drm/panel/panel-raydium-rm692e5.c also mentions to pass new value
to slice_per_pkt.
Hi Konrad,
Do you remember why value 2 is TODO for slice_per_pkt for panel rm692e5?
>
> >
> > Signed-off-by: Jun Nie
> > ---
> > include/drm/display/drm_dsc.h | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/include/drm/display/drm_dsc.h b/include/drm/display/drm_dsc.h
> > index bc90273d06a6..4fac0a2746ae 100644
> > --- a/include/drm/display/drm_dsc.h
> > +++ b/include/drm/display/drm_dsc.h
> > @@ -82,6 +82,10 @@ struct drm_dsc_config {
> > * @bits_per_component: Bits per component to code (8/10/12)
> > */
> > u8 bits_per_component;
> > + /**
> > +* @slice_per_pkt: slice number per DSC bit stream packet
> > +*/
> > + u8 slice_per_pkt;
> > /**
> > * @convert_rgb:
> > * Flag to indicate if RGB - YCoCg conversion is needed
> >
> > --
> > 2.34.1
> >
>
>
> --
> With best wishes
> Dmitry
Re: [PATCH] drm/msm: convert all pixel format logging to use %p4cc
On Mon, 08 Apr 2024, Dmitry Baryshkov wrote: > On Mon, 8 Apr 2024 at 11:09, Jani Nikula wrote: >> Thanks! Do you take this via the msm tree? > > Yes, I will Forgot to mention, there's a Tested-by at [1]. Tested-by: Aishwarya TCV [1] https://lore.kernel.org/r/157e69d4-850f-40d2-84ff-d25dd2c12...@arm.com -- Jani Nikula, Intel
Re: [PATCH] phy: qcom: qmp-combo: Fix VCO div offset on v3
On Thu, Apr 04, 2024 at 04:43:44PM -0700, Stephen Boyd wrote:
> Commit ec17373aebd0 ("phy: qcom: qmp-combo: extract common function to
> setup clocks") changed the offset that is used to write to
> DP_PHY_VCO_DIV from QSERDES_V3_DP_PHY_VCO_DIV to
> QSERDES_V4_DP_PHY_VCO_DIV. Unfortunately, this offset is different
> between v3 and v4 phys:
>
> #define QSERDES_V3_DP_PHY_VCO_DIV 0x064
> #define QSERDES_V4_DP_PHY_VCO_DIV 0x070
>
> meaning that we write the wrong register on v3 phys now. Add another
> generic register to 'regs' and use it here instead of a version specific
> define to fix this.
>
> This was discovered after Abhinav looked over register dumps with me
> from sc7180 Trogdor devices that started failing to light up the
> external display with v6.6 based kernels. It turns out that some
> monitors are very specific about their link clk frequency and if the
> default power on reset value is still there the monitor will show a
> blank screen or a garbled display. Other monitors are perfectly happy to
> get a bad clock signal.
> Fixes: ec17373aebd0 ("phy: qcom: qmp-combo: extract common function to setup
> clocks")
> Signed-off-by: Stephen Boyd
> ---
> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 7d585a4a..3b19d8ebf467 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -77,6 +77,7 @@ enum qphy_reg_layout {
> QPHY_COM_BIAS_EN_CLKBUFLR_EN,
>
> QPHY_DP_PHY_STATUS,
> + QPHY_DP_PHY_VCO_DIV,
>
> QPHY_TX_TX_POL_INV,
> QPHY_TX_TX_DRV_LVL,
> @@ -102,6 +103,7 @@ static const unsigned int
> qmp_v3_usb3phy_regs_layout[QPHY_LAYOUT_SIZE] = {
> + [QPHY_DP_PHY_VCO_DIV] = QSERDES_V3_DP_PHY_VCO_DIV,
> @@ -126,6 +128,7 @@ static const unsigned int
> qmp_v45_usb3phy_regs_layout[QPHY_LAYOUT_SIZE] = {
> + [QPHY_DP_PHY_VCO_DIV] = QSERDES_V4_DP_PHY_VCO_DIV,
I happened to skim this patch on the list and noticed that you added a
new register abstraction but only updated two tables.
A quick look at the driver reveals that there are currently four such
tables, which means that the v5_5nm (e.g. the Lenovo ThinkPad X13s) and
v6 hardware would now be broken instead as they would write to offset 0.
Clearly the hardware abstraction in this driver leaves a lot to wish
for when it's this fragile, but how can three people including the
maintainer review this change without this being noticed?
I just sent a follow-up fix here:
https://lore.kernel.org/lkml/20240408093023.506-1-johan+lin...@kernel.org/
> @@ -2184,7 +2188,7 @@ static int qmp_combo_configure_dp_clocks(struct
> qmp_combo *qmp)
> /* Other link rates aren't supported */
> return -EINVAL;
> }
> - writel(phy_vco_div, qmp->dp_dp_phy + QSERDES_V4_DP_PHY_VCO_DIV);
> + writel(phy_vco_div, qmp->dp_dp_phy + cfg->regs[QPHY_DP_PHY_VCO_DIV]);
Johan
Re: [PATCH v3 2/6] drm/msm/dsi: set video mode widebus enable bit when widebus is enabled
Dmitry Baryshkov 于2024年4月3日周三 18:10写道：
>
> On Wed, 3 Apr 2024 at 12:11, Jun Nie wrote:
> >
> > From: Jonathan Marek
> >
> > The value returned by msm_dsi_wide_bus_enabled() doesn't match what the
> > driver is doing in video mode. Fix that by actually enabling widebus for
> > video mode.
> >
> > Fixes: efcbd6f9cdeb ("drm/msm/dsi: Enable widebus for DSI")
> > Signed-off-by: Jonathan Marek
> > Signed-off-by: Jun Nie
>
> You have ignored all the review comments that were provided for v1.
> None of the tags were picked up either.
Just find that It was an accident that I cherry-picked wrong patch.
>
> Have you posted this for internal review like I have explicitly asked you?
Sorry, I guess I skipped your word in depression when I read the email.
>
> > ---
> > drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
> > drivers/gpu/drm/msm/dsi/dsi_host.c | 2 ++
> > 2 files changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h
> > b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> > index 2a7d980e12c3..f0b3cdc020a1 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
> > +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> > @@ -231,6 +231,7 @@ static inline uint32_t DSI_VID_CFG0_TRAFFIC_MODE(enum
> > dsi_traffic_mode val)
> > #define DSI_VID_CFG0_HSA_POWER_STOP0x0001
> > #define DSI_VID_CFG0_HBP_POWER_STOP0x0010
> > #define DSI_VID_CFG0_HFP_POWER_STOP0x0100
> > +#define DSI_VID_CFG0_DATABUS_WIDEN 0x0200
> > #define DSI_VID_CFG0_PULSE_MODE_HSA_HE 0x1000
>
> From the top of the file:
>
> /* Autogenerated file, DO NOT EDIT manually!
This is my fault, I did not notice the top of this file totally. Will
fix it in next version.
>
> >
> > #define REG_DSI_VID_CFG1 0x001c
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > index 9d86a6aca6f2..2a0422cad6de 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > @@ -754,6 +754,8 @@ static void dsi_ctrl_enable(struct msm_dsi_host
> > *msm_host,
> > data |=
> > DSI_VID_CFG0_TRAFFIC_MODE(dsi_get_traffic_mode(flags));
> > data |= DSI_VID_CFG0_DST_FORMAT(dsi_get_vid_fmt(mipi_fmt));
> > data |= DSI_VID_CFG0_VIRT_CHANNEL(msm_host->channel);
> > + if (msm_dsi_host_is_wide_bus_enabled(_host->base))
> > + data |= DSI_VID_CFG0_DATABUS_WIDEN;
> > dsi_write(msm_host, REG_DSI_VID_CFG0, data);
> >
> > /* Do not swap RGB colors */
> >
> > --
> > 2.34.1
> >
>
>
> --
> With best wishes
> Dmitry
Re: [PATCH] drm/msm/dpu: Add callback function pointer check before its call
On Mon, 8 Apr 2024 at 11:57, Aleksandr Mishin wrote:
>
> In dpu_core_irq_callback_handler() callback function pointer is compared to
> NULL,
> but then callback function is unconditionally called by this pointer.
> Fix this bug by adding conditional return.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
This should be converted to a proper Reported-by: trailer.
>
> Fixes: c929ac60b3ed ("drm/msm/dpu: allow just single IRQ callback")
> Signed-off-by: Aleksandr Mishin
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> index 946dd0135dff..03a16fbd4c99 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> @@ -223,9 +223,11 @@ static void dpu_core_irq_callback_handler(struct dpu_kms
> *dpu_kms, unsigned int
>
> VERB("IRQ=[%d, %d]\n", DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
>
> - if (!irq_entry->cb)
> + if (!irq_entry->cb) {
> DRM_ERROR("no registered cb, IRQ=[%d, %d]\n",
> DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
> + return;
> + }
>
> atomic_inc(_entry->count);
>
> --
> 2.30.2
>
>
--
With best wishes
Dmitry
Re: [PATCH] drm/msm: convert all pixel format logging to use %p4cc
On Mon, 8 Apr 2024 at 11:09, Jani Nikula wrote: > > On Fri, 05 Apr 2024, Dmitry Baryshkov wrote: > > On Fri, Apr 05, 2024 at 12:29:07PM +0300, Jani Nikula wrote: > >> Logging u32 pixel formats using %4.4s format string with a pointer to > >> the u32 is somewhat questionable, as well as dependent on byte > >> order. There's a kernel extension format specifier %p4cc to format 4cc > >> codes. Use it across the board in msm for pixel format logging. > >> > >> This should also fix the reported build warning: > >> > >> include/drm/drm_print.h:536:35: warning: '%4.4s' directive argument is > >> null [-Wformat-overflow=] > >> > >> Reported-by: Aishwarya TCV > >> Closes: > >> https://lore.kernel.org/r/2ac758ce-a196-4e89-a397-488ba3101...@arm.com > >> Signed-off-by: Jani Nikula > >> > >> --- > >> > >> Tip: 'git show --color-words -w' might be the easiest way to review. > >> --- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 8 +++ > >> .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 +- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 4 ++-- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 24 +-- > >> drivers/gpu/drm/msm/msm_fb.c | 10 > >> 5 files changed, 24 insertions(+), 24 deletions(-) > > > > Reviewed-by: Dmitry Baryshkov > > Thanks! Do you take this via the msm tree? Yes, I will -- With best wishes Dmitry
Re: [PATCH] drm/msm: convert all pixel format logging to use %p4cc
On Fri, 05 Apr 2024, Dmitry Baryshkov wrote: > On Fri, Apr 05, 2024 at 12:29:07PM +0300, Jani Nikula wrote: >> Logging u32 pixel formats using %4.4s format string with a pointer to >> the u32 is somewhat questionable, as well as dependent on byte >> order. There's a kernel extension format specifier %p4cc to format 4cc >> codes. Use it across the board in msm for pixel format logging. >> >> This should also fix the reported build warning: >> >> include/drm/drm_print.h:536:35: warning: '%4.4s' directive argument is >> null [-Wformat-overflow=] >> >> Reported-by: Aishwarya TCV >> Closes: >> https://lore.kernel.org/r/2ac758ce-a196-4e89-a397-488ba3101...@arm.com >> Signed-off-by: Jani Nikula >> >> --- >> >> Tip: 'git show --color-words -w' might be the easiest way to review. >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 8 +++ >> .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 +- >> drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 4 ++-- >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 24 +-- >> drivers/gpu/drm/msm/msm_fb.c | 10 >> 5 files changed, 24 insertions(+), 24 deletions(-) > > Reviewed-by: Dmitry Baryshkov Thanks! Do you take this via the msm tree? BR, Jani. -- Jani Nikula, Intel
