Re: [PATCH] drm/msm/dp: allow voltage swing / pre emphasis of 3

[Doug Anderson]

Hi,

On Sat, Feb 3, 2024 at 5:47 AM Dmitry Baryshkov
 wrote:
>
> Both dp_link_adjust_levels() and dp_ctrl_update_vx_px() limit swing and
> pre-emphasis to 2, while the real maximum value for the sum of the
> voltage swing and pre-emphasis is 3. Fix the DP code to remove this
> limitation.
>
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> Signed-off-by: Dmitry Baryshkov 
> ---
>  drivers/gpu/drm/msm/dp/dp_ctrl.c |  6 +++---
>  drivers/gpu/drm/msm/dp/dp_link.c | 22 +++---
>  drivers/gpu/drm/msm/dp/dp_link.h | 14 +-
>  3 files changed, 15 insertions(+), 27 deletions(-)

What ever happened with this patch? It seemed important so I've been
trying to check back on it, but it seems to still be in limbo. I was
assuming that (maybe?) Abhinav would check things against the hardware
documentation and give it a Reviewed-by and then it would land...

-Doug

Re: [PATCH v2 3/4] drm/msm/dp: Delete the old 500 ms wait for eDP HPD in aux transfer

[Doug Anderson]

Hi,

On Tue, Mar 19, 2024 at 10:27 AM Dmitry Baryshkov
 wrote:
>
> On Tue, 19 Mar 2024 at 19:13, Abhinav Kumar  wrote:
> >
> >
> >
> > On 3/18/2024 5:55 PM, Dmitry Baryshkov wrote:
> > > On Tue, 19 Mar 2024 at 02:19, Abhinav Kumar  
> > > wrote:
> > >>
> > >> +bjorn, johan as fyi for sc8280xp
> > >>
> > >> On 3/15/2024 2:36 PM, Douglas Anderson wrote:
> > >>> Before the introduction of the wait_hpd_asserted() callback in commit
> > >>> 841d742f094e ("drm/dp: Add wait_hpd_asserted() callback to struct
> > >>> drm_dp_aux") the API between panel drivers and DP AUX bus drivers was
> > >>> that it was up to the AUX bus driver to wait for HPD in the transfer()
> > >>> function.
> > >>>
> > >>> Now wait_hpd_asserted() has been added. The two panel drivers that are
> > >>> DP AUX endpoints use it. See commit 2327b13d6c47 ("drm/panel-edp: Take
> > >>> advantage of wait_hpd_asserted() in struct drm_dp_aux") and commit
> > >>> 3b5765df375c ("drm/panel: atna33xc20: Take advantage of
> > >>> wait_hpd_asserted() in struct drm_dp_aux"). We've implemented
> > >>> wait_hpd_asserted() in the MSM DP driver as of commit e2969ee30252
> > >>> ("drm/msm/dp: move of_dp_aux_populate_bus() to eDP probe()"). There is
> > >>> no longer any reason for long wait in the AUX transfer() function.
> > >>> Remove it.
> > >>>
> > >>> NOTE: the wait_hpd_asserted() is listed as "optional". That means it's
> > >>> optional for the DP AUX bus to implement. In the case of the MSM DP
> > >>> driver we implement it so we can assume it will be called.
> > >>>
> > >>
> > >> How do we enforce that for any new edp panels to be used with MSM, the
> > >> panels should atleast invoke wait_hpd_asserted()?
> > >>
> > >> I agree that since MSM implements it, even though its listed as
> > >> optional, we can drop this additional wait. So nothing wrong with this
> > >> patch for current users including sc8280xp, sc7280 and sc7180.
> > >>
> > >> But, does there need to be some documentation that the edp panels not
> > >> using the panel-edp framework should still invoke wait_hpd_asserted()?
> > >>
> > >> Since its marked as optional, what happens if the edp panel driver,
> > >> skips calling wait_hpd_asserted()?
> > >
> > > It is optional for the DP AUX implementations, not for the panel to be 
> > > called.
> > >
> >
> > Yes, I understood that part, but is there anything from the panel side
> > which mandates calling wait_hpd_asserted()?
> >
> > Is this documented somewhere for all edp panels to do:
> >
> > if (aux->wait_hpd_asserted)
> > aux->wait_hpd_asserted(aux, wait_us);
>
> That's obviously not true, e.g. if panel-edp.c handled HPD signal via
> the GPIO pin.
>
> But the documentation explicitly says that the host will be powered up
> automatically, but the caller must ensure that the panel is powered
> on. `It's up to the caller of this code to make sure that the panel is
> powered on if getting an error back is not OK.'

It wouldn't hurt to send out a documentation patch that makes this
more explicit. OK, I sent:

https://lore.kernel.org/r/20240319111432.1.I521dad0693cc24fe4dd14cba0c7048d94f5b6b41@changeid

-Doug

Re: [PATCH v2 4/4] drm/msm/dp: Fix typo in static function (ststus => status)

[Doug Anderson]

Hi,

On Mon, Mar 18, 2024 at 12:26 PM Stephen Boyd  wrote:
>
> Quoting Douglas Anderson (2024-03-15 14:36:32)
> > This is a no-op change to just fix a typo in the name of a static function.
> >
> > Signed-off-by: Douglas Anderson 
> > ---
> >
> > Changes in v2:
> > - ("Fix typo in static function (ststus => status)") new for v2.
>
> This was sent at
> https://lore.kernel.org/r/20240306193515.455388-1-quic_abhin...@quicinc.com

Whoops! I guess we both noticed it at about the same time. My patch
should be dropped then. The rest of my series (patches #1 - #3) are
still relevant. I won't repost them since they can be applied just
fine even if this patch is dropped.

-Doug

Re: [PATCH 1/3] drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected

[Doug Anderson]

Hi,

On Wed, Mar 13, 2024 at 1:41 PM Abhinav Kumar  wrote:
>
>
>
> On 3/12/2024 5:13 PM, Douglas Anderson wrote:
> > As documented in the description of the transfer() function of
> > "struct drm_dp_aux", the transfer() function can be called at any time
> > regardless of the state of the DP port. Specifically if the kernel has
> > the DP AUX character device enabled and userspace accesses
> > "/dev/drm_dp_auxN" directly then the AUX transfer function will be
> > called regardless of whether a DP device is connected.
> >
>
> I do see
>
> "
> * Also note that this callback can be called no matter the
> * state @dev is in and also no matter what state the panel is
> * in. It's expected:
> "
>
> I understand about the host state that we need to allow the transfers by
> powering on if the host was off.
>
> But I wonder why we should allow the transfer if the sink is not
> connected because it will anyway timeout.

We shouldn't! That's what this patch is about. ;-)


> Does it make sense to have get_hpd_status() from the aux dev and not
> issue the transfers if the sink was not connected?
>
> This is more of questioning the intent of drm_dp_helpers to allow
> transfers without checking the sink status.

It's a good question. I guess some of this just comes from the
abstraction that we currently have.

Thinking about this, the ideal would be to somehow query back to the
"drm_connector" since it already has a "->detect" function. ...but we
can't really do this since the AUX bus needs to be able to do
transfers early before all the DRM components aren't initialized. This
is, for instance, how the eDP panel code queries the EDID while being
probed.

We could consider adding a new callback to "struct drm_dp_aux" that
would allow checking the HPD status, but it feels to me like this adds
unneeded complexity. We'd be adding a callback that people need to
think about just to avoid them adding an "if" statement to their AUX
transfer routine. I'm not totally convinced.

Interestingly, we actually _could_ use the infrastructure I just
introduced in commit 8df1ddb5bf11 ("drm/dp: Don't attempt AUX
transfers when eDP panels are not powered") here, at least if we're in
the DP case and not the eDP case. When we're in the DP case there is
no panel involved so the DP driver itself knows when things are
"powered". For now I'm _not_ going to do this since it feels to me
like the "if" test makes it clearer what's happening, but yell if you
want me to change it.


> > For eDP panels we have a special rule where we wait (with a 5 second
> > timeout) for HPD to go high. This rule was important before all panels
> > drivers were converted to call wait_hpd_asserted() and actually can be
> > removed in a future commit.
> >
> > For external DP devices we never checked for HPD. That means that
> > trying to access the DP AUX character device (AKA `hexdump -C
> > /dev/drm_dp_auxN`) would very, very slowly timeout. Specifically on my
> > system:
> >$ time hexdump -C /dev/drm_dp_aux0
> >hexdump: /dev/drm_dp_aux0: Connection timed out
> >
> >real0m8.200s
> >
>
> IIUC, we want to timeout faster by not bailing out if not connected right?

Correct. I can try to clarify the commit message for v2 to make this
more obvious.


> > Let's add a check for HPD to avoid the slow timeout. This matches
> > what, for instance, the intel_dp_aux_xfer() function does when it
> > calls intel_tc_port_connected_locked(). That call has a document by it
> > explaining that it's important to avoid the long timeouts.
> >
> > Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> > Signed-off-by: Douglas Anderson 
> > ---
> >
> >   drivers/gpu/drm/msm/dp/dp_aux.c |  8 +++-
> >   drivers/gpu/drm/msm/dp/dp_catalog.c | 10 ++
> >   drivers/gpu/drm/msm/dp/dp_catalog.h |  1 +
> >   3 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
> > b/drivers/gpu/drm/msm/dp/dp_aux.c
> > index 03f4951c49f4..de0b0eabced9 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> > @@ -307,7 +307,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
> > *dp_aux,
> >* turned on the panel and then tried to do an AUX transfer. The panel
> >* driver has no way of knowing when the panel is ready, so it's up
> >* to us to wait. For DP we never get into this situation so let's
> > -  * avoid ever doing the extra long wait for DP.
> > +  * avoid ever doing the extra long wait for DP and just query HPD
> > +  * directly.
> >*/
> >   if (aux->is_edp) {
> >   ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
> > @@ -315,6 +316,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
> > *dp_aux,
> >   DRM_DEBUG_DP("Panel not ready for aux 
> > transactions\n");
> >   goto exit;
> >   }
> > + } else {
> > + if 

Re: [PATCH][next] drm/msm: remove unused variable 'out'

[Doug Anderson]

Hi,

On Thu, Mar 7, 2024 at 1:37 AM Colin Ian King  wrote:
>
> The variable out is being initialized and incremented but it is never
> actually referenced in any other way. The variable is redundant and can
> be removed.
>
> Cleans up clang scan build warning:
> drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c:843:6: warning: variable
> 'out' set but not used [-Wunused-but-set-variable]
>
> Signed-off-by: Colin Ian King 
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 3 ---
>  1 file changed, 3 deletions(-)

Tested-by: Douglas Anderson 

Re: [PATCH 1/3] drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected

[Doug Anderson]

Hi,

On Tue, Mar 12, 2024 at 5:47 PM Guenter Roeck  wrote:
>
> On Tue, Mar 12, 2024 at 5:14 PM Douglas Anderson  
> wrote:
> >
> > As documented in the description of the transfer() function of
> > "struct drm_dp_aux", the transfer() function can be called at any time
> > regardless of the state of the DP port. Specifically if the kernel has
> > the DP AUX character device enabled and userspace accesses
> > "/dev/drm_dp_auxN" directly then the AUX transfer function will be
> > called regardless of whether a DP device is connected.
> >
> > For eDP panels we have a special rule where we wait (with a 5 second
> > timeout) for HPD to go high. This rule was important before all panels
> > drivers were converted to call wait_hpd_asserted() and actually can be
> > removed in a future commit.
> >
> > For external DP devices we never checked for HPD. That means that
> > trying to access the DP AUX character device (AKA `hexdump -C
> > /dev/drm_dp_auxN`) would very, very slowly timeout. Specifically on my
> > system:
> >   $ time hexdump -C /dev/drm_dp_aux0
> >   hexdump: /dev/drm_dp_aux0: Connection timed out
> >
> >   real0m8.200s
> >
> > Let's add a check for HPD to avoid the slow timeout. This matches
> > what, for instance, the intel_dp_aux_xfer() function does when it
> > calls intel_tc_port_connected_locked(). That call has a document by it
> > explaining that it's important to avoid the long timeouts.
> >
> > Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> > Signed-off-by: Douglas Anderson 
> > ---
> >
> >  drivers/gpu/drm/msm/dp/dp_aux.c |  8 +++-
> >  drivers/gpu/drm/msm/dp/dp_catalog.c | 10 ++
> >  drivers/gpu/drm/msm/dp/dp_catalog.h |  1 +
> >  3 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
> > b/drivers/gpu/drm/msm/dp/dp_aux.c
> > index 03f4951c49f4..de0b0eabced9 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> > @@ -307,7 +307,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
> > *dp_aux,
> >  * turned on the panel and then tried to do an AUX transfer. The 
> > panel
> >  * driver has no way of knowing when the panel is ready, so it's up
> >  * to us to wait. For DP we never get into this situation so let's
> > -* avoid ever doing the extra long wait for DP.
> > +* avoid ever doing the extra long wait for DP and just query HPD
> > +* directly.
> >  */
> > if (aux->is_edp) {
> > ret = 
> > dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
> > @@ -315,6 +316,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
> > *dp_aux,
> > DRM_DEBUG_DP("Panel not ready for aux 
> > transactions\n");
> > goto exit;
> > }
> > +   } else {
> > +   if (!dp_catalog_aux_is_hpd_connected(aux->catalog)) {
> > +   ret = -ENXIO;
> > +   goto exit;
> > +   }
> > }
> >
> > dp_aux_update_offset_and_segment(aux, msg);
> > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c 
> > b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > index 5142aeb705a4..93e2d413a1e7 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > @@ -266,6 +266,16 @@ int dp_catalog_aux_wait_for_hpd_connect_state(struct 
> > dp_catalog *dp_catalog)
> > 2000, 50);
> >  }
> >
> > +bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog)
> > +{
> > +   struct dp_catalog_private *catalog = container_of(dp_catalog,
> > +   struct dp_catalog_private, dp_catalog);
> > +
> > +   /* poll for hpd connected status every 2ms and timeout after 500ms 
> > */
>
> Maybe I am missing something, but the comment doesn't seem to match
> the code below.
>
> Guenter
>
> > +   return readl(catalog->io->dp_controller.aux.base + 
> > REG_DP_DP_HPD_INT_STATUS) &
> > +  DP_DP_HPD_STATE_STATUS_CONNECTED;

LOL. I guess I overlooked that. Thanks for catching! The comment got
copied from the dp_catalog_aux_wait_for_hpd_connect_state(). I'll
remove the comment and send a v2, but I'll wait a little bit to see if
there is additional feedback.

-Doug

Re: [PATCH 3/9] arm64: dts: qcom: sc7280: Enable MDP turbo mode

[Doug Anderson]

Hi,

On Thu, Feb 22, 2024 at 9:32 AM Dmitry Baryshkov
 wrote:
>
> On Thu, 22 Feb 2024 at 17:04, Bjorn Andersson  
> wrote:
> >
> > On Thu, Feb 22, 2024 at 11:46:26AM +0200, Dmitry Baryshkov wrote:
> > > On Thu, 22 Feb 2024 at 11:28, Konrad Dybcio  
> > > wrote:
> > > >
> > > >
> > > >
> > > > On 2/22/24 10:04, Dmitry Baryshkov wrote:
> > > > > On Thu, 22 Feb 2024 at 10:56, Konrad Dybcio 
> > > > >  wrote:
> > > > >>
> > > > >>
> > > > >>
> > > > >> On 2/22/24 00:41, Dmitry Baryshkov wrote:
> > > > >>> On Thu, 22 Feb 2024 at 01:19, Bjorn Andersson 
> > > > >>>  wrote:
> > > > 
> > > >  The max frequency listed in the DPU opp-table is 506MHz, this is 
> > > >  not
> > > >  sufficient to drive a 4k@60 display, resulting in constant 
> > > >  underrun.
> > > > 
> > > >  Add the missing MDP_CLK turbo frequency of 608MHz to the opp-table 
> > > >  to
> > > >  fix this.
> > > > >>>
> > > > >>> I think we might want to keep this disabled for ChromeOS devices. 
> > > > >>> Doug?
> > > > >>
> > > > >> ChromeOS devices don't get a special SoC
> > > > >
> > > > > But they have the sc7280-chrome-common.dtsi, which might contain a
> > > > > corresponding /delete-node/ .
> > > >
> > > > What does that change? The clock rates are bound to the
> > > > SoC and the effective values are limited by link-frequencies
> > > > or the panel driver.
> > >
> > > Preventing the DPU from overheating? Or spending too much power?
> > >
> >
> > Perhaps I'm misunderstanding the implementation then, are we always
> > running at the max opp? I thought the opp was selected based on the
> > current need for performance?
>
> Yes. My concern was whether the Chrome people purposely skipped this
> top/turbo freq for any reason. In such a case, surprising them by
> adding it to all platforms might be not the best idea. I hope Doug can
> comment here.

Thanks for thinking of us! In this case, I think the only users left
of the sc7280 Chrome devices are folks like Rob and then a few folks
on Qualcomm's display team (like Abhinav), so if they're happy with
the change then I have no objections.

In any case, I'm not aware of any reason why this would have been
skipped for Chrome. The Chrome devices were always intended to support
4K so I assume this was an oversight and nothing more. ...of course,
as Abhinav points out Chrome devices are currently limited to HBR2 + 2
lanes DP so they can't go 4K60 anyway.

In any case, in case it matters, feel free to have:

Acked-by: Douglas Anderson 

Re: [Freedreno] [PATCH] drm/msm/a6xx: Fix GMU lockdep splat

[Doug Anderson]

Hi,

On Thu, Aug 3, 2023 at 10:34 AM Rob Clark  wrote:
>
> From: Rob Clark 
>
> For normal GPU devfreq, we need to acquire the GMU lock while already
> holding devfreq locks.  But in the teardown path, we were calling
> dev_pm_domain_detach() while already holding the GMU lock, resulting in
> this lockdep splat:
>
>==
>WARNING: possible circular locking dependency detected
>6.4.3-debug+ #3 Not tainted
>--
>ring0/391 is trying to acquire lock:
>ff80a025c078 (>lock){+.+.}-{3:3}, at: 
> qos_notifier_call+0x30/0x74
>
>but task is already holding lock:
>ff809b8c1ce8 (&(c->notifiers)->rwsem){}-{3:3}, at: 
> blocking_notifier_call_chain+0x34/0x78
>
>which lock already depends on the new lock.
>
>the existing dependency chain (in reverse order) is:
>
>-> #4 (&(c->notifiers)->rwsem){}-{3:3}:
>   down_write+0x58/0x74
>   __blocking_notifier_chain_register+0x64/0x84
>   blocking_notifier_chain_register+0x1c/0x28
>   freq_qos_add_notifier+0x5c/0x7c
>   dev_pm_qos_add_notifier+0xd4/0xf0
>   devfreq_add_device+0x42c/0x560
>   devm_devfreq_add_device+0x6c/0xb8
>   msm_devfreq_init+0xa8/0x16c [msm]
>   msm_gpu_init+0x368/0x54c [msm]
>   adreno_gpu_init+0x248/0x2b0 [msm]
>   a6xx_gpu_init+0x2d0/0x384 [msm]
>   adreno_bind+0x264/0x2bc [msm]
>   component_bind_all+0x124/0x1f4
>   msm_drm_bind+0x2d0/0x5f4 [msm]
>   try_to_bring_up_aggregate_device+0x88/0x1a4
>   __component_add+0xd4/0x128
>   component_add+0x1c/0x28
>   dp_display_probe+0x37c/0x3c0 [msm]
>   platform_probe+0x70/0xc0
>   really_probe+0x148/0x280
>   __driver_probe_device+0xfc/0x114
>   driver_probe_device+0x44/0x100
>   __device_attach_driver+0x64/0xdc
>   bus_for_each_drv+0xb0/0xd8
>   __device_attach+0xe4/0x140
>   device_initial_probe+0x1c/0x28
>   bus_probe_device+0x44/0xb0
>   deferred_probe_work_func+0xb0/0xc8
>   process_one_work+0x288/0x3d8
>   worker_thread+0x1f0/0x260
>   kthread+0xf0/0x100
>   ret_from_fork+0x10/0x20
>
>-> #3 (dev_pm_qos_mtx){+.+.}-{3:3}:
>   __mutex_lock+0xc8/0x388
>   mutex_lock_nested+0x2c/0x38
>   dev_pm_qos_remove_notifier+0x3c/0xc8
>   genpd_remove_device+0x40/0x11c
>   genpd_dev_pm_detach+0x88/0x130
>   dev_pm_domain_detach+0x2c/0x3c
>   a6xx_gmu_remove+0x44/0xdc [msm]
>   a6xx_destroy+0x7c/0xa4 [msm]
>   adreno_unbind+0x50/0x64 [msm]
>   component_unbind+0x44/0x64
>   component_unbind_all+0xb4/0xbc
>   msm_drm_uninit.isra.0+0x124/0x17c [msm]
>   msm_drm_bind+0x340/0x5f4 [msm]
>   try_to_bring_up_aggregate_device+0x88/0x1a4
>   __component_add+0xd4/0x128
>   component_add+0x1c/0x28
>   dp_display_probe+0x37c/0x3c0 [msm]
>   platform_probe+0x70/0xc0
>   really_probe+0x148/0x280
>   __driver_probe_device+0xfc/0x114
>   driver_probe_device+0x44/0x100
>   __device_attach_driver+0x64/0xdc
>   bus_for_each_drv+0xb0/0xd8
>   __device_attach+0xe4/0x140
>   device_initial_probe+0x1c/0x28
>   bus_probe_device+0x44/0xb0
>   deferred_probe_work_func+0xb0/0xc8
>   process_one_work+0x288/0x3d8
>   worker_thread+0x1f0/0x260
>   kthread+0xf0/0x100
>   ret_from_fork+0x10/0x20
>
>-> #2 (_gpu->gmu.lock){+.+.}-{3:3}:
>   __mutex_lock+0xc8/0x388
>   mutex_lock_nested+0x2c/0x38
>   a6xx_gpu_set_freq+0x38/0x64 [msm]
>   msm_devfreq_target+0x170/0x18c [msm]
>   devfreq_set_target+0x90/0x1e4
>   devfreq_update_target+0xb4/0xf0
>   update_devfreq+0x1c/0x28
>   devfreq_monitor+0x3c/0x10c
>   process_one_work+0x288/0x3d8
>   worker_thread+0x1f0/0x260
>   kthread+0xf0/0x100
>   ret_from_fork+0x10/0x20
>
>-> #1 (>lock){+.+.}-{3:3}:
>   __mutex_lock+0xc8/0x388
>   mutex_lock_nested+0x2c/0x38
>   msm_devfreq_get_dev_status+0x4c/0x104 [msm]
>   devfreq_simple_ondemand_func+0x5c/0x128
>   devfreq_update_target+0x68/0xf0
>   update_devfreq+0x1c/0x28
>   devfreq_monitor+0x3c/0x10c
>   process_one_work+0x288/0x3d8
>   worker_thread+0x1f0/0x260
>   kthread+0xf0/0x100
>   ret_from_fork+0x10/0x20
>
>-> #0 (>lock){+.+.}-{3:3}:
>   __lock_acquire+0xdf8/0x109c
>   lock_acquire+0x234/0x284
>   __mutex_lock+0xc8/0x388
>   mutex_lock_nested+0x2c/0x38
>   qos_notifier_call+0x30/0x74
>   qos_min_notifier_call+0x1c/0x28
>   notifier_call_chain+0xf4/0x114
>   

Re: [Freedreno] patches dropped from drm-misc-next [Was: Re: [PATCH 00/53] drm: Convert to platform remove callback returning] void

[Doug Anderson]

Hi,

On Sat, Jun 17, 2023 at 9:15 AM Uwe Kleine-König
 wrote:
>
> [expanding recipents by the other affected persons]
>
> On Thu, Jun 08, 2023 at 09:08:15AM -0700, Doug Anderson wrote:
> > On Thu, Jun 1, 2023 at 8:40 AM Uwe Kleine-König
> >  wrote:
> > >
> > > Hello,
> > >
> > > On Sun, May 07, 2023 at 06:25:23PM +0200, Uwe Kleine-König wrote:
> > > > this patch series adapts the platform drivers below drivers/gpu/drm
> > > > to use the .remove_new() callback. Compared to the traditional .remove()
> > > > callback .remove_new() returns no value. This is a good thing because
> > > > the driver core doesn't (and cannot) cope for errors during remove. The
> > > > only effect of a non-zero return value in .remove() is that the driver
> > > > core emits a warning. The device is removed anyhow and an early return
> > > > from .remove() usually yields a resource leak.
> > > >
> > > > By changing the remove callback to return void driver authors cannot
> > > > reasonably (but wrongly) assume any more that there happens some kind of
> > > > cleanup later.
> > >
> > > I wonder if someone would volunteer to add the whole series to
> > > drm-misc-next?!
> >
> > It looks as if Neil applied quite a few of them already, so I looked
> > at what was left...
> >
> > I'm a little hesitant to just apply the whole kit-and-caboodle to
> > drm-misc-next since there are specific DRM trees for a bunch of them
> > and it would be better if they landed there. ...so I went through all
> > the patches that still applied to drm-misc-next, then used
> > 'scripts/get_maintainer.pl --scm' to check if they were maintained
> > through drm-misc. That still left quite a few patches. I've applied
> > those ones and pushed to drm-misc-next:
> >
> > 71722685cd17 drm/xlnx/zynqmp_dpsub: Convert to platform remove
> > callback returning void
> > 1ed54a19f3b3 drm/vc4: Convert to platform remove callback returning void
> > b957812839f8 drm/v3d: Convert to platform remove callback returning void
> > e2fd3192e267 drm/tve200: Convert to platform remove callback returning void
> > 84e6da7ad553 drm/tiny: Convert to platform remove callback returning void
> > 34cdd1f691ad drm/tidss: Convert to platform remove callback returning void
> > d665e3c9d37a drm/sun4i: Convert to platform remove callback returning void
> > 0c259ab19146 drm/stm: Convert to platform remove callback returning void
> > 9a865e45884a drm/sti: Convert to platform remove callback returning void
> > 3c855610840e drm/rockchip: Convert to platform remove callback returning 
> > void
> > e41977a83b71 drm/panfrost: Convert to platform remove callback returning 
> > void
> > cef3776d0b5a drm/panel: Convert to platform remove callback returning void
> > bd296a594e87 drm/mxsfb: Convert to platform remove callback returning void
> > 38ca2d93d323 drm/meson: Convert to platform remove callback returning void
> > fd1457d84bae drm/mcde: Convert to platform remove callback returning void
> > 41a56a18615c drm/logicvc: Convert to platform remove callback returning void
> > 980ec6444372 drm/lima: Convert to platform remove callback returning void
> > 82a2c0cc1a22 drm/hisilicon: Convert to platform remove callback returning 
> > void
> > c3b28b29ac0a drm/fsl-dcu: Convert to platform remove callback returning void
> > a118fc6e71f9 drm/atmel-hlcdc: Convert to platform remove callback returning 
> > void
> > 9a32dd324c46 drm/aspeed: Convert to platform remove callback returning void
> > 2c7d291c498c drm/arm/malidp: Convert to platform remove callback returning 
> > void
> > a920028df679 drm/arm/hdlcd: Convert to platform remove callback returning 
> > void
> > 1bf3d76a7d15 drm/komeda: Convert to platform remove callback returning void
>
> Together with the patches that were applied later the topmost commit
> from this series is c2807ecb5290 ("drm/omap: Convert to platform remove
> callback returning void"). This commit was part for the following next
> tags:
>
> $ git tag -l --contains c2807ecb5290
> next-20230609
> next-20230613
> next-20230614
> next-20230615
>
> However in next-20230616 they are missing. In next-20230616
> drm-misc/for-linux-next was cf683e8870bd4be0fd6b98639286700a35088660.
> Compared to c2807ecb5290 this adds 1149 patches but drops 37 (that are
> also not included with a different commit id). The 37 patches dropped
> are 13cdd12a9f934158f4ec817cf048fcb4384aa9dc..c2807ecb5290:
>
> $ git shortlog -s 
> 13c

Re: [Freedreno] [PATCH] drm/msm/adreno: make adreno_is_a690()'s argument const

[Doug Anderson]

Hi,

On Mon, Jun 12, 2023 at 11:25 AM Dmitry Baryshkov
 wrote:
>
> Change adreno_is_a690() prototype to accept the const struct adreno_gpu
> pointer instead of a non-const one. This fixes the following warning:
>
> In file included from drivers/gpu/drm/msm/msm_drv.c:33:
> drivers/gpu/drm/msm/adreno/adreno_gpu.h: In function ‘adreno_is_a660_family’:
> drivers/gpu/drm/msm/adreno/adreno_gpu.h:303:54: warning: passing argument 1 
> of ‘adreno_is_a690’ discards ‘const’ qualifier from pointer target type 
> [-Wdiscarded-qualifiers]
>   303 | return adreno_is_a660(gpu) || adreno_is_a690(gpu) || 
> adreno_is_7c3(gpu);
>
> Fixes: 1b90e8f8879c ("drm/msm/adreno: change adreno_is_* functions to accept 
> const argument")
> Signed-off-by: Dmitry Baryshkov 
> ---
>  drivers/gpu/drm/msm/adreno/adreno_gpu.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Douglas Anderson 

Re: [Freedreno] [PATCH] drm/msm/dp: Drop aux devices together with DP controller

[Doug Anderson]

Hi,

On Mon, Jun 12, 2023 at 3:40 PM Dmitry Baryshkov
 wrote:
>
> On 13/06/2023 01:01, Bjorn Andersson wrote:
> > Using devres to depopulate the aux bus made sure that upon a probe
> > deferral the EDP panel device would be destroyed and recreated upon next
> > attempt.
> >
> > But the struct device which the devres is tied to is the DPUs
> > (drm_dev->dev), which may be happen after the DP controller is torn
> > down.
> >
> > Indications of this can be seen in the commonly seen EDID-hexdump full
> > of zeros in the log, or the occasional/rare KASAN fault where the
> > panel's attempt to read the EDID information causes a use after free on
> > DP resources.
> >
> > It's tempting to move the devres to the DP controller's struct device,
> > but the resources used by the device(s) on the aux bus are explicitly
> > torn down in the error path.
>
> I hoped that proper usage of of_dp_aux_populate_bus(), with the callback
> function being non-NULL would have solved at least this part. But it
> seems I'll never see this patch.

Agreed. This has been pending for > 1 year now with no significant
progress. Abhinav: Is there anything that can be done about this? Not
following up on agreed-to cleanups in a timely manner doesn't set a
good precedent. Next time the Qualcomm display wants to land something
and promises to land a followup people will be less likely to believe
them...


> > The KASAN-reported use-after-free also
> > remains, as the DP aux "module" explicitly frees its devres-allocated
> > memory in this code path.
> >
> > As such, explicitly depopulate the aux bus in the error path, and in the
> > component unbind path, to avoid these issues.
> >
> > Fixes: 2b57f726611e ("drm/msm/dp: fix aux-bus EP lifetime")
> > Signed-off-by: Bjorn Andersson 
>
> Reviewed-by: Dmitry Baryshkov 

Reviewed-by: Douglas Anderson 

Re: [Freedreno] [PATCH 00/53] drm: Convert to platform remove callback returning void

[Doug Anderson]

Hi,

On Thu, Jun 8, 2023 at 9:26 AM Laurent Pinchart
 wrote:
>
> > The following ones appeared to apply to the top of drm-misc-next, but
> > I didn't apply them since get_maintainer didn't say they were part of
> > drm-misc-next:
> >
> > drm/tiny: Convert to platform remove callback returning void
> > drm/tilcdc: Convert to platform remove callback returning void
> > drm/sprd: Convert to platform remove callback returning void
> > drm/shmobile: Convert to platform remove callback returning void
> > drm/rcar-du: Convert to platform remove callback returning void
>
> If you don't mind, could you take the rcar-du patch through drm-misc too
> ? I don't plan to send another pull request for v6.5.

Done.

2510a2579324 drm/rcar-du: Convert to platform remove callback returning void

Re: [Freedreno] [PATCH 00/53] drm: Convert to platform remove callback returning void

[Doug Anderson]

Hi,

On Thu, Jun 8, 2023 at 10:19 AM Tomi Valkeinen
 wrote:
>
> On 08/06/2023 19:26, Laurent Pinchart wrote:
> > Hi Doug,
> >
> > On Thu, Jun 08, 2023 at 09:08:15AM -0700, Doug Anderson wrote:
> >> On Thu, Jun 1, 2023 at 8:40 AM Uwe Kleine-König wrote:
> >>> On Sun, May 07, 2023 at 06:25:23PM +0200, Uwe Kleine-König wrote:
> >>>> this patch series adapts the platform drivers below drivers/gpu/drm
> >>>> to use the .remove_new() callback. Compared to the traditional .remove()
> >>>> callback .remove_new() returns no value. This is a good thing because
> >>>> the driver core doesn't (and cannot) cope for errors during remove. The
> >>>> only effect of a non-zero return value in .remove() is that the driver
> >>>> core emits a warning. The device is removed anyhow and an early return
> >>>> from .remove() usually yields a resource leak.
> >>>>
> >>>> By changing the remove callback to return void driver authors cannot
> >>>> reasonably (but wrongly) assume any more that there happens some kind of
> >>>> cleanup later.
> >>>
> >>> I wonder if someone would volunteer to add the whole series to
> >>> drm-misc-next?!
> >>
> >> It looks as if Neil applied quite a few of them already, so I looked
> >> at what was left...
> >>
> >> I'm a little hesitant to just apply the whole kit-and-caboodle to
> >> drm-misc-next since there are specific DRM trees for a bunch of them
> >> and it would be better if they landed there. ...so I went through all
> >> the patches that still applied to drm-misc-next, then used
> >> 'scripts/get_maintainer.pl --scm' to check if they were maintained
> >> through drm-misc. That still left quite a few patches. I've applied
> >> those ones and pushed to drm-misc-next:
> >>
> >> 71722685cd17 drm/xlnx/zynqmp_dpsub: Convert to platform remove
> >> callback returning void
> >> 1ed54a19f3b3 drm/vc4: Convert to platform remove callback returning void
> >> b957812839f8 drm/v3d: Convert to platform remove callback returning void
> >> e2fd3192e267 drm/tve200: Convert to platform remove callback returning void
> >> 84e6da7ad553 drm/tiny: Convert to platform remove callback returning void
> >> 34cdd1f691ad drm/tidss: Convert to platform remove callback returning void
> >> d665e3c9d37a drm/sun4i: Convert to platform remove callback returning void
> >> 0c259ab19146 drm/stm: Convert to platform remove callback returning void
> >> 9a865e45884a drm/sti: Convert to platform remove callback returning void
> >> 3c855610840e drm/rockchip: Convert to platform remove callback returning 
> >> void
> >> e41977a83b71 drm/panfrost: Convert to platform remove callback returning 
> >> void
> >> cef3776d0b5a drm/panel: Convert to platform remove callback returning void
> >> bd296a594e87 drm/mxsfb: Convert to platform remove callback returning void
> >> 38ca2d93d323 drm/meson: Convert to platform remove callback returning void
> >> fd1457d84bae drm/mcde: Convert to platform remove callback returning void
> >> 41a56a18615c drm/logicvc: Convert to platform remove callback returning 
> >> void
> >> 980ec6444372 drm/lima: Convert to platform remove callback returning void
> >> 82a2c0cc1a22 drm/hisilicon: Convert to platform remove callback returning 
> >> void
> >> c3b28b29ac0a drm/fsl-dcu: Convert to platform remove callback returning 
> >> void
> >> a118fc6e71f9 drm/atmel-hlcdc: Convert to platform remove callback 
> >> returning void
> >> 9a32dd324c46 drm/aspeed: Convert to platform remove callback returning void
> >> 2c7d291c498c drm/arm/malidp: Convert to platform remove callback returning 
> >> void
> >> a920028df679 drm/arm/hdlcd: Convert to platform remove callback returning 
> >> void
> >> 1bf3d76a7d15 drm/komeda: Convert to platform remove callback returning void
> >>
> >> The following ones appeared to apply to the top of drm-misc-next, but
> >> I didn't apply them since get_maintainer didn't say they were part of
> >> drm-misc-next:
> >>
> >> drm/tiny: Convert to platform remove callback returning void
> >> drm/tilcdc: Convert to platform remove callback returning void
> >> drm/sprd: Convert to platform remove callback returning void
> >> drm/shmobile: Convert to platform remove callback returning void
> >> drm/rcar-du: Convert to platform remove callback returning void
>

Re: [Freedreno] [PATCH 00/53] drm: Convert to platform remove callback returning void

[Doug Anderson]

Hi,

On Thu, Jun 1, 2023 at 8:40 AM Uwe Kleine-König
 wrote:
>
> Hello,
>
> On Sun, May 07, 2023 at 06:25:23PM +0200, Uwe Kleine-König wrote:
> > this patch series adapts the platform drivers below drivers/gpu/drm
> > to use the .remove_new() callback. Compared to the traditional .remove()
> > callback .remove_new() returns no value. This is a good thing because
> > the driver core doesn't (and cannot) cope for errors during remove. The
> > only effect of a non-zero return value in .remove() is that the driver
> > core emits a warning. The device is removed anyhow and an early return
> > from .remove() usually yields a resource leak.
> >
> > By changing the remove callback to return void driver authors cannot
> > reasonably (but wrongly) assume any more that there happens some kind of
> > cleanup later.
>
> I wonder if someone would volunteer to add the whole series to
> drm-misc-next?!

It looks as if Neil applied quite a few of them already, so I looked
at what was left...

I'm a little hesitant to just apply the whole kit-and-caboodle to
drm-misc-next since there are specific DRM trees for a bunch of them
and it would be better if they landed there. ...so I went through all
the patches that still applied to drm-misc-next, then used
'scripts/get_maintainer.pl --scm' to check if they were maintained
through drm-misc. That still left quite a few patches. I've applied
those ones and pushed to drm-misc-next:

71722685cd17 drm/xlnx/zynqmp_dpsub: Convert to platform remove
callback returning void
1ed54a19f3b3 drm/vc4: Convert to platform remove callback returning void
b957812839f8 drm/v3d: Convert to platform remove callback returning void
e2fd3192e267 drm/tve200: Convert to platform remove callback returning void
84e6da7ad553 drm/tiny: Convert to platform remove callback returning void
34cdd1f691ad drm/tidss: Convert to platform remove callback returning void
d665e3c9d37a drm/sun4i: Convert to platform remove callback returning void
0c259ab19146 drm/stm: Convert to platform remove callback returning void
9a865e45884a drm/sti: Convert to platform remove callback returning void
3c855610840e drm/rockchip: Convert to platform remove callback returning void
e41977a83b71 drm/panfrost: Convert to platform remove callback returning void
cef3776d0b5a drm/panel: Convert to platform remove callback returning void
bd296a594e87 drm/mxsfb: Convert to platform remove callback returning void
38ca2d93d323 drm/meson: Convert to platform remove callback returning void
fd1457d84bae drm/mcde: Convert to platform remove callback returning void
41a56a18615c drm/logicvc: Convert to platform remove callback returning void
980ec6444372 drm/lima: Convert to platform remove callback returning void
82a2c0cc1a22 drm/hisilicon: Convert to platform remove callback returning void
c3b28b29ac0a drm/fsl-dcu: Convert to platform remove callback returning void
a118fc6e71f9 drm/atmel-hlcdc: Convert to platform remove callback returning void
9a32dd324c46 drm/aspeed: Convert to platform remove callback returning void
2c7d291c498c drm/arm/malidp: Convert to platform remove callback returning void
a920028df679 drm/arm/hdlcd: Convert to platform remove callback returning void
1bf3d76a7d15 drm/komeda: Convert to platform remove callback returning void

The following ones appeared to apply to the top of drm-misc-next, but
I didn't apply them since get_maintainer didn't say they were part of
drm-misc-next:

drm/tiny: Convert to platform remove callback returning void
drm/tilcdc: Convert to platform remove callback returning void
drm/sprd: Convert to platform remove callback returning void
drm/shmobile: Convert to platform remove callback returning void
drm/rcar-du: Convert to platform remove callback returning void
drm/omap: Convert to platform remove callback returning void
drm/nouveau: Convert to platform remove callback returning void
drm/mediatek: Convert to platform remove callback returning void
drm/kmb: Convert to platform remove callback returning void
drm/ingenic: Convert to platform remove callback returning void
drm/imx/ipuv3: Convert to platform remove callback returning void
drm/imx/dcss: Convert to platform remove callback returning void
drm/etnaviv: Convert to platform remove callback returning void
drm/armada: Convert to platform remove callback returning void

-Doug

Re: [Freedreno] [PATCH] drm/msm/a6xx: fix uninitialised lock in init error path

[Doug Anderson]

Hi,

On Wed, May 31, 2023 at 1:00 AM Johan Hovold  wrote:
>
> A recent commit started taking the GMU lock in the GPU destroy path,
> which on GPU initialisation failure is called before the GMU and its
> lock have been initialised.
>
> Make sure that the GMU has been initialised before taking the lock in
> a6xx_destroy() and drop the now redundant check from a6xx_gmu_remove().
>
> Fixes: 4cd15a3e8b36 ("drm/msm/a6xx: Make GPU destroy a bit safer")
> Cc: sta...@vger.kernel.org  # 6.3
> Cc: Douglas Anderson 
> Signed-off-by: Johan Hovold 
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 3 ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 9 ++---
>  2 files changed, 6 insertions(+), 6 deletions(-)

I think Dmitry already posted a patch 1.5 months ago to fix this.

https://lore.kernel.org/r/20230410165908.3094626-1-dmitry.barysh...@linaro.org

Can you confirm that works for you?

-Doug

Re: [Freedreno] [PATCH v10 00/10] drm/hdcp: Pull HDCP auth/exchange/check into helpers

[Doug Anderson]

Mark,

On Mon, May 22, 2023 at 5:59 AM Rodrigo Vivi  wrote:
>
> On Sat, May 20, 2023 at 02:07:51AM +0300, Dmitry Baryshkov wrote:
> > On 20/05/2023 00:16, Rodrigo Vivi wrote:
> > > On Fri, May 19, 2023 at 07:55:47PM +0300, Dmitry Baryshkov wrote:
> > > > On 19/04/2023 18:43, Mark Yacoub wrote:
> > > > > Hi all,
> > > > > This is v10 of the HDCP patches. The patches are authored by Sean 
> > > > > Paul.
> > > > > I rebased and addressed the review comments in v6-v10.
> > > > >
> > > > > Main change in v10 is handling the kernel test bot warnings.
> > > > >
> > > > > Patches 1-4 focus on moving the common HDCP helpers to common DRM.
> > > > > This introduces a slight change in the original intel flow
> > > > > as it splits the unique driver protocol from the generic 
> > > > > implementation.
> > > > >
> > > > > Patches 5-7 split the HDCP flow on the i915 driver to make use of the 
> > > > > common DRM helpers.
> > > > >
> > > > > Patches 8-10 implement HDCP on MSM driver.
> > > > >
> > > > > Thanks,
> > > > > -Mark Yacoub
> > > > >
> > > > > Sean Paul (10):
> > > > > drm/hdcp: Add drm_hdcp_atomic_check()
> > > > > drm/hdcp: Avoid changing crtc state in hdcp atomic check
> > > > > drm/hdcp: Update property value on content type and user changes
> > > > > drm/hdcp: Expand HDCP helper library for enable/disable/check
> > > > > drm/i915/hdcp: Consolidate HDCP setup/state cache
> > > > > drm/i915/hdcp: Retain hdcp_capable return codes
> > > > > drm/i915/hdcp: Use HDCP helpers for i915
> > > > > dt-bindings: msm/dp: Add bindings for HDCP registers
> > > > > arm64: dts: qcom: sc7180: Add support for HDCP in dp-controller
> > > >
> > > > Dear i915 maintainers,
> > > >
> > > > I wanted to ping you regarding this patch series. If there are no 
> > > > comments
> > > > for the series from you side, would it be possible to land 
> > > > Intel-specific
> > > > and generic patches into drm-intel tree? We will continue working on 
> > > > the msm
> > > > specific parts and merge them through the msm tree.
> > >
> > > pushed to drm-intel-next.
> > >
> > > should be propagated in a few weeks to drm-next on our next pull request.
> >
> > Probably there is some kind of confusion here. You've pushed the DSC
> > patches, while the response was sent to the HDCP series.
>
> I'm sorry, my confusion for replying to the wrong thread.
>
> So, on this one here I believe it would be helpful if there's a split
> in the series with the already reviewed ones related to i915 are resent
> separated from the rest, send with --subject-prefix="CI" so when that
> pass CI we just merge it through drm-intel-next

It sounds like this is waiting on you to post just the first 7 patches
with the proper subject prefix so they can land through the Intel
tree.

-Doug

Re: [Freedreno] [PATCH] drm/msm/dp: add module parameter for PSR

[Doug Anderson]

Hi,

On Wed, May 24, 2023 at 1:06 AM Dmitry Baryshkov
 wrote:
>
> On 24/05/2023 09:59, Johan Hovold wrote:
> > On Tue, May 23, 2023 at 12:23:04PM -0700, Abhinav Kumar wrote:
> >> On 5/23/2023 8:24 AM, Johan Hovold wrote:
> >>> On Fri, May 12, 2023 at 09:13:04PM +0300, Dmitry Baryshkov wrote:
>  On 28/04/2023 02:28, Abhinav Kumar wrote:
> > On sc7280 where eDP is the primary display, PSR is causing
> > IGT breakage even for basic test cases like kms_atomic and
> > kms_atomic_transition. Most often the issue starts with below
> > stack so providing that as reference
> >
> > Call trace:
> >
> > ---[ end trace  ]---
> > [drm-dp] dp_ctrl_push_idle: PUSH_IDLE pattern timedout
> >
> > Other basic use-cases still seem to work fine hence add a
> > a module parameter to allow toggling psr enable/disable till
> > PSR related issues are hashed out with IGT.
> 
>  For the reference: Bjorn reported that he has issues with VT on a
>  PSR-enabled laptops. This patch fixes the issue for him
> >>>
> >>> Module parameters are almost never warranted, and it is definitely not
> >>> the right way to handle a broken implementation.
> >>>
> >>> I've just sent a revert that unconditionally disables PSR support until
> >>> the implementation has been fixed:
> >>>
> >>> 
> >>> https://lore.kernel.org/lkml/20230523151646.28366-1-johan+lin...@kernel.org/
> >>
> >> I dont completely agree with this. Even the virtual terminal case was
> >> reported to be fixed by one user but not the other. So it was probably
> >> something missed out either in validation or reproduction steps of the
> >> user who reported it to be fixed OR the user who reported it not fixed.
> >> That needs to be investigated now.
> >
> > Yes, there may still be some time left to fix it, but it's pretty damn
> > annoying to find that an issue reported two months ago still is not
> > fixed at 6.4-rc3. (I even waited to make the switch to 6.4 so that I
> > would not have to spend time on this.)
> >
> > I didn't see any mail from Bjorn saying that the series that claimed to
> > fix the VT issue actually did fix the VT issue. There's only the comment
> > above from Dmitry suggesting that disabling this feature is the only way
> > to get a working terminal back.
>
> Originally this issue was reported by Doug, and at [1] he reported that
> an issue is fixed for him. So, for me it looks like we have hardware
> where VT works and hardware where it doesn't.

As I understand it, the problem was originally reported by Bjorn over
IRC. When he reported the problem on VT2, Stephen Boyd confirmed that
he could see the same problem on our hardware when using VT2. I
confirmed I could reproduce and also tested the fix. I don't remember
if Bjorn ever tested the fix. I think many of us assumed that it was
the same issue so a fix for one person would also fix the other.


> Doug, can you please confirm whether you can reproduce the PSR+VT issue
> on 6.4-rc (without extra patches) or if the issue is fixed for you?
>
> [1]
> https://lore.kernel.org/dri-devel/CAD=FV=vshmqptsqfwjviezeerms-veotmfozejasuc9zsmj...@mail.gmail.com/

Ugh. Unfortunately, it's not easy for me to test this particular
feature directly on upstream Linux. :( I typically run with a ChromeOS
root filesystem, which works pretty well with mainline. One place
where it doesn't work with mainline is VT2. The VT2 feature for
Chromebooks is implemented with "frecon" and takes advantage of a
downstream patch that we've carried in the ChromeOS kernel trees for
years allowing handoff of who the DRM "master" is.

For testing the fix previously, I confirmed that I could reproduce the
problem on our downstream kernel (which had the PSR patches picked)
and that the fixes worked for me in that context.

Ah, but it shouldn't be too hard to pick that one patch. Let's see if
that works. I'll pick ("CHROMIUM: drm: Add drm_master_relax debugfs
file (non-root set/drop master ioctls)"). Indeed, it works!

OK, so with the top of Linus's tree (v6.4-rc3-17-g9d646009f65d) plus
the CHROMIUM patch to enable my VT2, I can confirm that I don't see
the issue. I can switch to VT2 and typing works fine. I will say that
on herobrine the backlight is all messed up, but I assume that's an
unrelated issue.

-Doug



> > Regressions happen and sometimes there are corner cases that are harder
> > to find, but this is a breakage of a fundamental feature that was
> > reported before the code was even merged into mainline.
> >
> >> We should have ideally gone with the modparam with the feature patches
> >> itself knowing that it gets enabled for all sinks if PSR is supported.
> >
> > Modparams are things of the past should not be used to enable broken
> > features so that some vendor can tick of their internal lists of
> > features that have been "mainlined".
>
> We have had a history of using modparam with i915 and IIRC amdgpu /
> radeon drivers to allow users to easily check 

Re: [Freedreno] [PATCH v10 00/10] drm/hdcp: Pull HDCP auth/exchange/check into helpers

[Doug Anderson]

Hi,

On Wed, Apr 19, 2023 at 8:43 AM Mark Yacoub  wrote:
>
> Hi all,
> This is v10 of the HDCP patches. The patches are authored by Sean Paul.
> I rebased and addressed the review comments in v6-v10.
>
> Main change in v10 is handling the kernel test bot warnings.
>
> Patches 1-4 focus on moving the common HDCP helpers to common DRM.
> This introduces a slight change in the original intel flow
> as it splits the unique driver protocol from the generic implementation.
>
> Patches 5-7 split the HDCP flow on the i915 driver to make use of the common 
> DRM helpers.
>
> Patches 8-10 implement HDCP on MSM driver.
>
> Thanks,
> -Mark Yacoub
>
> Sean Paul (10):
>   drm/hdcp: Add drm_hdcp_atomic_check()
>   drm/hdcp: Avoid changing crtc state in hdcp atomic check
>   drm/hdcp: Update property value on content type and user changes
>   drm/hdcp: Expand HDCP helper library for enable/disable/check
>   drm/i915/hdcp: Consolidate HDCP setup/state cache
>   drm/i915/hdcp: Retain hdcp_capable return codes
>   drm/i915/hdcp: Use HDCP helpers for i915
>   dt-bindings: msm/dp: Add bindings for HDCP registers
>   arm64: dts: qcom: sc7180: Add support for HDCP in dp-controller
>   drm/msm: Implement HDCP 1.x using the new drm HDCP helpers
>
>  .../bindings/display/msm/dp-controller.yaml   |7 +-
>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi  |8 +
>  drivers/gpu/drm/display/drm_hdcp_helper.c | 1224 +
>  drivers/gpu/drm/i915/display/intel_atomic.c   |8 +-
>  drivers/gpu/drm/i915/display/intel_ddi.c  |   32 +-
>  .../drm/i915/display/intel_display_debugfs.c  |   12 +-
>  .../drm/i915/display/intel_display_types.h|   51 +-
>  drivers/gpu/drm/i915/display/intel_dp_hdcp.c  |  352 ++---
>  drivers/gpu/drm/i915/display/intel_dp_mst.c   |   16 +-
>  drivers/gpu/drm/i915/display/intel_hdcp.c | 1060 +++---
>  drivers/gpu/drm/i915/display/intel_hdcp.h |   48 +-
>  drivers/gpu/drm/i915/display/intel_hdmi.c |  267 ++--
>  drivers/gpu/drm/msm/Kconfig   |1 +
>  drivers/gpu/drm/msm/Makefile  |1 +
>  drivers/gpu/drm/msm/dp/dp_catalog.c   |  156 +++
>  drivers/gpu/drm/msm/dp/dp_catalog.h   |   18 +
>  drivers/gpu/drm/msm/dp/dp_debug.c |   46 +-
>  drivers/gpu/drm/msm/dp/dp_debug.h |   11 +-
>  drivers/gpu/drm/msm/dp/dp_display.c   |   39 +-
>  drivers/gpu/drm/msm/dp/dp_display.h   |5 +
>  drivers/gpu/drm/msm/dp/dp_drm.c   |   39 +-
>  drivers/gpu/drm/msm/dp/dp_drm.h   |7 +
>  drivers/gpu/drm/msm/dp/dp_hdcp.c  |  389 ++
>  drivers/gpu/drm/msm/dp/dp_hdcp.h  |   33 +
>  drivers/gpu/drm/msm/dp/dp_parser.c|   14 +
>  drivers/gpu/drm/msm/dp/dp_parser.h|4 +
>  drivers/gpu/drm/msm/dp/dp_reg.h   |   30 +-
>  drivers/gpu/drm/msm/msm_atomic.c  |   19 +
>  include/drm/display/drm_hdcp.h|  296 
>  include/drm/display/drm_hdcp_helper.h |   23 +
>  30 files changed, 2867 insertions(+), 1349 deletions(-)

Mark asked me if I had any advice for getting this patch series
landed. I haven't been following the patch series super closely, but
as I understand it:

1. The first several patches (the generic ones) seem fairly well
reviewed and haven't changed in any significant ways in a while. The
ideal place to land these would be drm-misc, I think.

2. The i915 patches also seem OK to land. The ideal place would be the
Intel DRM tree, I think.

3. The msm patches are not fully baked yet. Not only is there a kernel
bot complaint on patch #10, but Mark also said that comments from v6
haven't yet fully been addressed and he's talked with Dmitry on IRC
about this and has a plan to move forward.


The question becomes: can/should we land the generic and maybe the
i915 patches now while the msm patches are reworked. Do folks have an
opinion here? If we're OK landing some of the patches, I guess we have
a few options:

a) Just land the generic patches to drm-misc and put the i915 ones on
the backburner until drm-misc has made it to somewhere that the
drm-intel tree is based on. If we want to go this route and nobody
objects, I don't mind being the person who does the gruntwork of
actually landing them on drm-misc-next, though I certainly wouldn't
rush to make sure that nobody is unhappy with this idea.

b) Land the generic patches in some type of immutable branch so they
can be pulled into drm-misc and the Intel DRM tree. Someone more
senior to me would need to help with this, but if we really want to go
this way I can poke folks on IRC.

c) Land the generic and Intel patches in the Intel tree. The msm
patches would not be able to land until these trickled up the chain,
but the msm patches aren't fully ready yet anyway so maybe this is OK.

d) Land the generic and Intel patches in the drm-misc tree. If folks
are OK with this I can be the person to pull the trigger, but I'd want
to 

Re: [Freedreno] [PATCH] drm/msm/a6xx: initialize GMU mutex earlier

[Doug Anderson]

Hi,

On Mon, Apr 10, 2023 at 9:59 AM Dmitry Baryshkov
 wrote:
>
> Move GMU mutex initialization earlier to make sure that it is always
> initialized. a6xx_destroy can be called from ther failure path before
> GMU initialization.
>
> This fixes the following backtrace:
>
> [ cut here ]
> DEBUG_LOCKS_WARN_ON(lock->magic != lock)
> WARNING: CPU: 0 PID: 58 at kernel/locking/mutex.c:582 __mutex_lock+0x1ec/0x3d0
> Modules linked in:
> CPU: 0 PID: 58 Comm: kworker/u16:1 Not tainted 6.3.0-rc5-00155-g187c06436519 
> #565
> Hardware name: Qualcomm Technologies, Inc. SM8350 HDK (DT)
> Workqueue: events_unbound deferred_probe_work_func
> pstate: 6045 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : __mutex_lock+0x1ec/0x3d0
> lr : __mutex_lock+0x1ec/0x3d0
> sp : 88993620
> x29: 88993620 x28: 0002 x27: 47b253c52800
> x26: 01000606 x25: 47b240bb2810 x24: fff4
> x23:  x22: c38bba15ac14 x21: 0002
> x20: 88993690 x19: 47b2430cc668 x18: fffe98f0
> x17: 6f74616c75676572 x16: 20796d6d75642067 x15: 0038
> x14:  x13: c38bbba050b8 x12: 0666
> x11: 0222 x10: c38bbba603e8 x9 : c38bbba050b8
> x8 : efff x7 : c38bbba5d0b8 x6 : 0222
> x5 : bff4 x4 : 4000f222 x3 : 
> x2 :  x1 :  x0 : 47b240cb1880
> Call trace:
>  __mutex_lock+0x1ec/0x3d0
>  mutex_lock_nested+0x2c/0x38
>  a6xx_destroy+0xa0/0x138
>  a6xx_gpu_init+0x41c/0x618
>  adreno_bind+0x188/0x290
>  component_bind_all+0x118/0x248
>  msm_drm_bind+0x1c0/0x670
>  try_to_bring_up_aggregate_device+0x164/0x1d0
>  __component_add+0xa8/0x16c
>  component_add+0x14/0x20
>  dsi_dev_attach+0x20/0x2c
>  dsi_host_attach+0x9c/0x144
>  devm_mipi_dsi_attach+0x34/0xac
>  lt9611uxc_attach_dsi.isra.0+0x84/0xfc
>  lt9611uxc_probe+0x5b8/0x67c
>  i2c_device_probe+0x1ac/0x358
>  really_probe+0x148/0x2ac
>  __driver_probe_device+0x78/0xe0
>  driver_probe_device+0x3c/0x160
>  __device_attach_driver+0xb8/0x138
>  bus_for_each_drv+0x84/0xe0
>  __device_attach+0x9c/0x188
>  device_initial_probe+0x14/0x20
>  bus_probe_device+0xac/0xb0
>  deferred_probe_work_func+0x8c/0xc8
>  process_one_work+0x2bc/0x594
>  worker_thread+0x228/0x438
>  kthread+0x108/0x10c
>  ret_from_fork+0x10/0x20
> irq event stamp: 299345
> hardirqs last  enabled at (299345): [] 
> put_cpu_partial+0x1c8/0x22c
> hardirqs last disabled at (299344): [] 
> put_cpu_partial+0x1c0/0x22c
> softirqs last  enabled at (296752): [] _stext+0x434/0x4e8
> softirqs last disabled at (296741): [] 
> do_softirq+0x10/0x1c
> ---[ end trace  ]---
>
> Fixes: 4cd15a3e8b36 ("drm/msm/a6xx: Make GPU destroy a bit safer")
> Cc: Douglas Anderson 
> Signed-off-by: Dmitry Baryshkov 
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 2 --
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 ++
>  2 files changed, 2 insertions(+), 2 deletions(-)

Sorry for the breakage and thanks for the fix!

Reviewed-by: Douglas Anderson 

Re: [Freedreno] [PATCH v1 2/3] msm/disp/dpu: allow atomic_check in PSR usecase

[Doug Anderson]

Hi,

On Fri, Mar 31, 2023 at 6:59 AM Vinod Polimera
 wrote:
>
> Certain flags like dirty_fb will be updated into the plane state
> during crtc atomic_check. Allow those updates during PSR commit.
>
> Reported-by: Bjorn Andersson 
> Link: https://lore.kernel.org/all/20230326162723.3lo6pnsfdwzsvbhj@ripper/
> Signed-off-by: Vinod Polimera 
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I can confirm that this patch plus patch #1 fixes the typing problems
seen on VT2 on a Chromebook using PSR.

Tested-by: Douglas Anderson 

Re: [Freedreno] [PATCH v1 1/3] drm/msm/dpu: set dirty_fb flag while in self refresh mode

[Doug Anderson]

Hi,

On Fri, Mar 31, 2023 at 6:59 AM Vinod Polimera
 wrote:
>
> While in virtual terminal mode with PSR enabled, there will be
> no atomic commits triggered without dirty_fb being set. This
> will create a notion of no screen update. Allow atomic commit
> when dirty_fb ioctl is issued, so that it can trigger a PSR exit
> and shows update on the screen.
>
> Reported-by: Bjorn Andersson 
> Link: https://lore.kernel.org/all/20230326162723.3lo6pnsfdwzsvbhj@ripper/
> Signed-off-by: Vinod Polimera 
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 3 +++
>  1 file changed, 3 insertions(+)

I can confirm that this patch plus patch #2 fixes the typing problems
seen on VT2 on a Chromebook using PSR.

Tested-by: Douglas Anderson 


-Doug

Re: [Freedreno] [PATCH v7 09/10] arm64: dts: qcom: sc7180: Add support for HDCP in dp-controller

[Doug Anderson]

Hi,

On Fri, Mar 24, 2023 at 12:56 PM Mark Yacoub  wrote:
>
> From: Sean Paul 
>
> Add the register ranges required for HDCP key injection and
> HDCP TrustZone interaction as described in the dt-bindings for the
> sc7180 dp controller.
>
> Signed-off-by: Sean Paul 
> Signed-off-by: Mark Yacoub 
>
> ---
> Changes in v3:
> -Split off into a new patch containing just the dts change (Stephen)
> -Add hdcp compatible string (Stephen)
> Changes in v4:
> -Rebase on Bjorn's multi-dp patchset
> Changes in v5:
> -Put the tz register offsets in trogdor dtsi (Rob C)
> Changes in v6:
> -Rebased: Removed modifications in sc7180.dtsi as it's already upstream
> Changes in v7:
> -Change registers offset
>
>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> index 47f39c547c41a..63183ac9c3c48 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> @@ -816,6 +816,14 @@ _dp {
> status = "okay";
> pinctrl-names = "default";
> pinctrl-0 = <_hot_plug_det>;
> +
> +   reg = <0 0x0ae9 0 0x200>,
> + <0 0x0ae90200 0 0x200>,
> + <0 0x0ae90400 0 0xc00>,
> + <0 0x0ae91000 0 0x400>,
> + <0 0x0ae91400 0 0x400>,
> + <0 0x0aed1000 0 0x174>,
> + <0 0x0aee1000 0 0x2c>;

Reviewed-by: Douglas Anderson 

Re: [Freedreno] [PATCH v7 08/10] dt-bindings: msm/dp: Add bindings for HDCP registers

[Doug Anderson]

Hi,

On Fri, Mar 24, 2023 at 12:56 PM Mark Yacoub  wrote:
>
> From: Sean Paul 
>
> Add the bindings for the MSM DisplayPort HDCP registers
> which are required to write the HDCP key into the display controller as
> well as the registers to enable HDCP authentication/key
> exchange/encryption.
>
> Cc: Rob Herring 
> Cc: Stephen Boyd 
> Reviewed-by: Rob Herring 
> Signed-off-by: Sean Paul 
> Signed-off-by: Mark Yacoub 
>
> ---
> Changes in v2:
> -Drop register range names (Stephen)
> -Fix yaml errors (Rob)
> Changes in v3:
> -Add new compatible string for dp-hdcp
> -Add descriptions to reg
> -Add minItems/maxItems to reg
> -Make reg depend on the new hdcp compatible string
> Changes in v4:
> -Rebase on Bjorn's multi-dp patchset
> Changes in v4.5:
> -Remove maxItems from reg (Rob)
> -Remove leading zeros in example (Rob)
> Changes in v5:
> -None
> Changes in v6:
> -Rebased: modify minItems instead of adding it as new line.
> Changes in v7:
> -Revert the change to minItems
> -Added the maxItems to Reg
>
>  .../devicetree/bindings/display/msm/dp-controller.yaml | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Reviewed-by: Douglas Anderson 

Re: [Freedreno] [PATCH] arm64: dts: qcom: sc7280: remove hbr3 support on herobrine boards

[Doug Anderson]

Hi,

On Wed, Mar 29, 2023 at 4:34 PM Abhinav Kumar  wrote:
>
> There are some interop issues seen across a few DP monitors with
> HBR3 and herobrine boards where the DP display stays blank with hbr3.
> This is still under investigation but in preparation for supporting
> higher resolutions, its better to disable HBR3 till the issues are
> root-caused as there is really no guarantee which monitors will show
> the issue and which would not.
>
> This can be enabled back after successful validation across more DP
> sinks.
>
> Signed-off-by: Abhinav Kumar 
> ---
>  arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi 
> b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> index b6137816f2f3..313083ec1f39 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> @@ -464,7 +464,7 @@ _dp {
>
>  _dp_out {
> data-lanes = <0 1>;
> -   link-frequencies = /bits/ 64 <162000 27 54 
> 81>;
> +   link-frequencies = /bits/ 64 <162000 27 54>;

This seems OK to me.

Reviewed-by: Douglas Anderson 

Re: [Freedreno] [PATCH v14 14/14] drm/msm/dp: set self refresh aware based on PSR support

[Doug Anderson]

Hi,

On Wed, Mar 29, 2023 at 8:47 AM Doug Anderson  wrote:
>
> Hi,
>
> On Wed, Mar 29, 2023 at 8:16 AM Vinod Polimera
>  wrote:
> >
> >
> >
> > > -Original Message-
> > > From: Stephen Boyd 
> > > Sent: Monday, March 27, 2023 9:58 PM
> > > To: Bjorn Andersson ; Vinod Polimera (QUIC)
> > > 
> > > Cc: dri-de...@lists.freedesktop.org; linux-arm-...@vger.kernel.org;
> > > freedreno@lists.freedesktop.org; devicet...@vger.kernel.org; linux-
> > > ker...@vger.kernel.org; robdcl...@gmail.com; diand...@chromium.org;
> > > Kalyan Thota (QUIC) ;
> > > dmitry.barysh...@linaro.org; Kuogee Hsieh (QUIC)
> > > ; Vishnuvardhan Prodduturi (QUIC)
> > > ; Bjorn Andersson (QUIC)
> > > ; Abhinav Kumar (QUIC)
> > > ; Sankeerth Billakanti (QUIC)
> > > 
> > > Subject: Re: [PATCH v14 14/14] drm/msm/dp: set self refresh aware based
> > > on PSR support
> > >
> > > Quoting Bjorn Andersson (2023-03-26 09:35:56)
> > > > On Sun, Mar 26, 2023 at 09:27:23AM -0700, Bjorn Andersson wrote:
> > > > > On Thu, Mar 02, 2023 at 10:03:17PM +0530, Vinod Polimera wrote:
> > > > > > For the PSR to kick in, self_refresh_aware has to be set.
> > > > > > Initialize it based on the PSR support for the eDP interface.
> > > > > >
> > > > >
> > > > > When I boot my sc8280xp devices (CRD and X13s) to console with this
> > > > > patch included I get a login prompt, and then there are no more screen
> > > > > updates.
> > > > >
> > > > > Switching virtual terminal (ctrl+alt+fN) causes the screen to redraw.
> > > > >
> > > > > Blindly login in and launching Wayland works and from then on screen
> > > > > updates works as expected.
> > > > >
> > > > > Switching from Wayland to another virtual terminal causes the problem
> > > to
> > > > > re-appear, no updates after the initial refresh, switching back go the
> > > > > Wayland-terminal crashed the machine.
> > > > >
> > > >
> > > > Also, trying to bring the eDP-screen back from DPMS gives me:
> > > >
> > > > <3>[ 2355.218099] [drm:dp_catalog_ctrl_set_pattern_state_bit [msm]]
> > > *ERROR* set state_bit for link_train=1 failed
> > > > <3>[ 2355.218926] [drm:dp_ctrl_setup_main_link [msm]] *ERROR* link
> > > training #1 failed. ret=-110
> > > > <3>[ 2355.262859] [drm:dp_catalog_ctrl_set_pattern_state_bit [msm]]
> > > *ERROR* set state_bit for link_train=1 failed
> > > > <3>[ 2355.263600] [drm:dp_ctrl_setup_main_link [msm]] *ERROR* link
> > > training #1 failed. ret=-110
> > > > <3>[ 2355.305211] [drm:dp_catalog_ctrl_set_pattern_state_bit [msm]]
> > > *ERROR* set state_bit for link_train=1 failed
> > > > <3>[ 2355.305955] [drm:dp_ctrl_setup_main_link [msm]] *ERROR* link
> > > training #1 failed. ret=-110
> > > > <3>[ 2355.345250] [drm:dp_catalog_ctrl_set_pattern_state_bit [msm]]
> > > *ERROR* set state_bit for link_train=1 failed
> > > > <3>[ 2355.346026] [drm:dp_ctrl_setup_main_link [msm]] *ERROR* link
> > > training #1 failed. ret=-110
> > > > <3>[ 2355.405650] [drm:dp_display_process_hpd_high [msm]] *ERROR*
> > > failed to complete DP link training
> > > > <3>[ 2355.668988]
> > > [drm:dpu_encoder_phys_vid_wait_for_commit_done:488] [dpu
> > > error]vblank timeout
> > > > <3>[ 2355.669030] [drm:dpu_kms_wait_for_commit_done:510] [dpu
> > > error]wait for commit done returned -110
> > > > <3>[ 2355.699989] [drm:dpu_encoder_frame_done_timeout:2398] [dpu
> > > error]enc35 frame done timeout
> > > >
> > > > And then the machine just resets.
> > > >
> > >
> > > I saw similar behavior on ChromeOS after we picked the PSR patches into
> > > our kernel. I suspect it's the same problem. I switched back and forth
> > > between VT2 and the OOBE screen with ctrl+alt+forward and that showed
> > > what I typed on the virtual terminal after switching back and forth.
> > > It's like the redraw only happens once on the switch and never again. I
> > > switched back and forth enough times that it eventually crashed the
> > > kernel and rebooted. This was on CRD (sc7280-herobrine-crd.dts).
> > >
> > > There's an animation on the OOBE screen that i

Re: [Freedreno] [PATCH v14 14/14] drm/msm/dp: set self refresh aware based on PSR support

[Doug Anderson]

Hi,

On Wed, Mar 29, 2023 at 8:16 AM Vinod Polimera
 wrote:
>
>
>
> > -Original Message-
> > From: Stephen Boyd 
> > Sent: Monday, March 27, 2023 9:58 PM
> > To: Bjorn Andersson ; Vinod Polimera (QUIC)
> > 
> > Cc: dri-de...@lists.freedesktop.org; linux-arm-...@vger.kernel.org;
> > freedreno@lists.freedesktop.org; devicet...@vger.kernel.org; linux-
> > ker...@vger.kernel.org; robdcl...@gmail.com; diand...@chromium.org;
> > Kalyan Thota (QUIC) ;
> > dmitry.barysh...@linaro.org; Kuogee Hsieh (QUIC)
> > ; Vishnuvardhan Prodduturi (QUIC)
> > ; Bjorn Andersson (QUIC)
> > ; Abhinav Kumar (QUIC)
> > ; Sankeerth Billakanti (QUIC)
> > 
> > Subject: Re: [PATCH v14 14/14] drm/msm/dp: set self refresh aware based
> > on PSR support
> >
> > Quoting Bjorn Andersson (2023-03-26 09:35:56)
> > > On Sun, Mar 26, 2023 at 09:27:23AM -0700, Bjorn Andersson wrote:
> > > > On Thu, Mar 02, 2023 at 10:03:17PM +0530, Vinod Polimera wrote:
> > > > > For the PSR to kick in, self_refresh_aware has to be set.
> > > > > Initialize it based on the PSR support for the eDP interface.
> > > > >
> > > >
> > > > When I boot my sc8280xp devices (CRD and X13s) to console with this
> > > > patch included I get a login prompt, and then there are no more screen
> > > > updates.
> > > >
> > > > Switching virtual terminal (ctrl+alt+fN) causes the screen to redraw.
> > > >
> > > > Blindly login in and launching Wayland works and from then on screen
> > > > updates works as expected.
> > > >
> > > > Switching from Wayland to another virtual terminal causes the problem
> > to
> > > > re-appear, no updates after the initial refresh, switching back go the
> > > > Wayland-terminal crashed the machine.
> > > >
> > >
> > > Also, trying to bring the eDP-screen back from DPMS gives me:
> > >
> > > <3>[ 2355.218099] [drm:dp_catalog_ctrl_set_pattern_state_bit [msm]]
> > *ERROR* set state_bit for link_train=1 failed
> > > <3>[ 2355.218926] [drm:dp_ctrl_setup_main_link [msm]] *ERROR* link
> > training #1 failed. ret=-110
> > > <3>[ 2355.262859] [drm:dp_catalog_ctrl_set_pattern_state_bit [msm]]
> > *ERROR* set state_bit for link_train=1 failed
> > > <3>[ 2355.263600] [drm:dp_ctrl_setup_main_link [msm]] *ERROR* link
> > training #1 failed. ret=-110
> > > <3>[ 2355.305211] [drm:dp_catalog_ctrl_set_pattern_state_bit [msm]]
> > *ERROR* set state_bit for link_train=1 failed
> > > <3>[ 2355.305955] [drm:dp_ctrl_setup_main_link [msm]] *ERROR* link
> > training #1 failed. ret=-110
> > > <3>[ 2355.345250] [drm:dp_catalog_ctrl_set_pattern_state_bit [msm]]
> > *ERROR* set state_bit for link_train=1 failed
> > > <3>[ 2355.346026] [drm:dp_ctrl_setup_main_link [msm]] *ERROR* link
> > training #1 failed. ret=-110
> > > <3>[ 2355.405650] [drm:dp_display_process_hpd_high [msm]] *ERROR*
> > failed to complete DP link training
> > > <3>[ 2355.668988]
> > [drm:dpu_encoder_phys_vid_wait_for_commit_done:488] [dpu
> > error]vblank timeout
> > > <3>[ 2355.669030] [drm:dpu_kms_wait_for_commit_done:510] [dpu
> > error]wait for commit done returned -110
> > > <3>[ 2355.699989] [drm:dpu_encoder_frame_done_timeout:2398] [dpu
> > error]enc35 frame done timeout
> > >
> > > And then the machine just resets.
> > >
> >
> > I saw similar behavior on ChromeOS after we picked the PSR patches into
> > our kernel. I suspect it's the same problem. I switched back and forth
> > between VT2 and the OOBE screen with ctrl+alt+forward and that showed
> > what I typed on the virtual terminal after switching back and forth.
> > It's like the redraw only happens once on the switch and never again. I
> > switched back and forth enough times that it eventually crashed the
> > kernel and rebooted. This was on CRD (sc7280-herobrine-crd.dts).
> >
> > There's an animation on the OOBE screen that is working though, so
> > perhaps PSR is working with the chrome compositor but not the virtual
> > terminal? I haven't investigated.
>
> I was able to reproduce the issue where in virtual terminal, I don't see any 
> screen refresh despite typing in.
> In the VT mode, I see that PSR is entered, but despite typing in there are no 
> atomic commits triggered, hence the last buffer was always refreshed.
>
> Queries from my side to Rob & Doug:
> 1) In VT mode, does the framework operates in single buffer mode without any 
> commit for new updates ?
> 2) if above is true then, how does driver know if the framework operates in 
> single buffer mode, to make any appropriate action
> 3) what is the expected behavior with the driver here ? should it return 
> atomic_check failure, for single buffer mode operation or, it should exit PSR 
> ?
> 4) is there any HINT passed down to the driver so that we can bank on it and 
> act accordingly?

I haven't looked at this detail about PSR before, and I left my
PSR-enabled device at home so I can't easily test this right now. That
being said, from a bit of searching I would guess that
msm_framebuffer_dirtyfb() is somehow involved here. Are things better
if you get rid of the test 

Re: [Freedreno] [PATCH v14 00/14] Add PSR support for eDP

[Doug Anderson]

Hi,

On Thu, Mar 2, 2023 at 8:33 AM Vinod Polimera  wrote:
>
> Changes in v2:
>   - Use dp bridge to set psr entry/exit instead of dpu_enocder.
>   - Don't modify whitespaces.
>   - Set self refresh aware from atomic_check.
>   - Set self refresh aware only if psr is supported.
>   - Provide a stub for msm_dp_display_set_psr.
>   - Move dp functions to bridge code.
>
> Changes in v3:
>   - Change callback names to reflect atomic interfaces.
>   - Move bridge callback change to separate patch as suggested by Dmitry.
>   - Remove psr function declaration from msm_drv.h.
>   - Set self_refresh_aware flag only if psr is supported.
>   - Modify the variable names to simpler form.
>   - Define bit fields for PSR settings.
>   - Add comments explaining the steps to enter/exit psr.
>   - Change DRM_INFO to drm_dbg_db.
>
> Changes in v4:
>   - Move the get crtc functions to drm_atomic.
>   - Add atomic functions for DP bridge too.
>   - Add ternary operator to choose eDP or DP ops.
>   - Return true/false instead of 1/0.
>   - mode_valid missing in the eDP bridge ops.
>   - Move the functions to get crtc into drm_atomic.c.
>   - Fix compilation issues.
>   - Remove dpu_assign_crtc and get crtc from drm_enc instead of dpu_enc.
>   - Check for crtc state enable while reserving resources.
>
> Changes in v5:
>   - Move the mode_valid changes into a different patch.
>   - Complete psr_op_comp only when isr is set.
>   - Move the DP atomic callback changes to a different patch.
>   - Get crtc from drm connector state crtc.
>   - Move to separate patch for check for crtc state enable while
> reserving resources.
>
> Changes in v6:
>   - Remove crtc from dpu_encoder_virt struct.
>   - fix crtc check during vblank toggle crtc.
>   - Misc changes.
>
> Changes in v7:
>   - Add fix for underrun issue on kasan build.
>
> Changes in v8:
>   - Drop the enc spinlock as it won't serve any purpose in
> protetcing conn state.(Dmitry/Doug)
>
> Changes in v9:
>   - Update commit message and fix alignment using spaces.(Marijn)
>   - Misc changes.(Marijn)
>
> Changes in v10:
>   - Get crtc cached in dpu_enc during obj init.(Dmitry)
>
> Changes in v11:
>   - Remove crtc cached in dpu_enc during obj init.
>   - Update dpu_enc crtc state on crtc enable/disable during self refresh.
>
> Changes in v12:
>   - Update sc7180 intf mask to get intf timing gen status
> based on DPU_INTF_STATUS_SUPPORTED bit.(Dmitry)
>   - Remove "clear active interface in the datapath cleanup" change
> as it is already included.
>
> Changes in v13:
>   - Move core changes to top of the series.(Dmitry)
>   - Drop self refresh aware disable change after psr entry.(Dmitry)
>
> Changes in v14:
>   - Set self_refresh_aware for the PSR to kick in.
>
> Vinod Polimera (14):
>   drm: add helper functions to retrieve old and new crtc
>   drm/bridge: use atomic enable/disable callbacks for panel bridge
>   drm/bridge: add psr support for panel bridge callbacks
>   drm/msm/disp/dpu: check for crtc enable rather than crtc active to
> release shared resources
>   drm/msm/disp/dpu: get timing engine status from intf status register
>   drm/msm/disp/dpu: wait for extra vsync till timing engine status is
> disabled
>   drm/msm/disp/dpu: reset the datapath after timing engine disable
>   drm/msm/dp: use atomic callbacks for DP bridge ops
>   drm/msm/dp: Add basic PSR support for eDP
>   drm/msm/dp: use the eDP bridge ops to validate eDP modes
>   drm/msm/disp/dpu: use atomic enable/disable callbacks for encoder
> functions
>   drm/msm/disp/dpu: add PSR support for eDP interface in dpu driver
>   drm/msm/disp/dpu: update dpu_enc crtc state on crtc enable/disable
> during self refresh
>   drm/msm/dp: set self refresh aware based on psr support
>
>  drivers/gpu/drm/bridge/panel.c |  68 +++-
>  drivers/gpu/drm/drm_atomic.c   |  60 +++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c   |  40 -
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c|  26 +++-
>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   |  22 +++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c |   3 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |  12 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c|   8 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c|   2 +-
>  drivers/gpu/drm/msm/dp/dp_catalog.c|  80 ++
>  drivers/gpu/drm/msm/dp/dp_catalog.h|   4 +
>  drivers/gpu/drm/msm/dp/dp_ctrl.c   |  80 ++
>  drivers/gpu/drm/msm/dp/dp_ctrl.h   |   3 +
>  drivers/gpu/drm/msm/dp/dp_display.c|  36 +++--
>  drivers/gpu/drm/msm/dp/dp_display.h|   2 +
>  drivers/gpu/drm/msm/dp/dp_drm.c| 173 
> -
>  drivers/gpu/drm/msm/dp/dp_drm.h|   9 +-
>  drivers/gpu/drm/msm/dp/dp_link.c   |  36 +
>  drivers/gpu/drm/msm/dp/dp_panel.c  

Re: [Freedreno] [RFT PATCH v2 1/3] drm/bridge: tc358762: Set pre_enable_prev_first

[Doug Anderson]

Hi,

On Mon, Feb 27, 2023 at 5:24 PM Dmitry Baryshkov
 wrote:
>
> On 28/02/2023 02:26, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Feb 1, 2023 at 1:51 AM Dave Stevenson
> >  wrote:
> >>
> >> On Tue, 31 Jan 2023 at 22:22, Douglas Anderson  
> >> wrote:
> >>>
> >>> Set the "pre_enable_prev_first" as provided by commit 4fb912e5e190
> >>> ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init
> >>> order"). This should allow us to revert commit ec7981e6c614
> >>> ("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") and
> >>> commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> >>> time").
> >>
> >> I see no reference in the TC358762 datasheet to requiring the DSI
> >> interface to be in any particular state.
> >> However, setting this flag does mean that the DSI host doesn't need to
> >> power up and down for each host_transfer request from
> >> tc358762_pre_enable/tc358762_init, so on that basis I'm good with it.
> >>
> >> Reviewed-by: Dave Stevenson 
> >>
> >>> Cc: Dave Stevenson 
> >>> Cc: Dmitry Baryshkov 
> >>> Cc: Abhinav Kumar 
> >>> Signed-off-by: Douglas Anderson 
> >>> ---
> >>>
> >>> (no changes since v1)
> >>>
> >>>   drivers/gpu/drm/bridge/tc358762.c | 1 +
> >>>   1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/bridge/tc358762.c 
> >>> b/drivers/gpu/drm/bridge/tc358762.c
> >>> index 0b6a28436885..77f7f7f54757 100644
> >>> --- a/drivers/gpu/drm/bridge/tc358762.c
> >>> +++ b/drivers/gpu/drm/bridge/tc358762.c
> >>> @@ -229,6 +229,7 @@ static int tc358762_probe(struct mipi_dsi_device *dsi)
> >>>  ctx->bridge.funcs = _bridge_funcs;
> >>>  ctx->bridge.type = DRM_MODE_CONNECTOR_DPI;
> >>>  ctx->bridge.of_node = dev->of_node;
> >>> +   ctx->bridge.pre_enable_prev_first = true;
> >>>
> >>>  drm_bridge_add(>bridge);
> >
> > Abhinav asked what the plan was for landing this [1]. Since this isn't
> > urgent, I guess the plan is to land patch #1 in drm-misc-next. Then we
> > sit and wait until it percolates into mainline and, once it does, then
> > patch #2 and #3 can land.
> >
> > Since I have Dave's review I can commit this to drm-misc-next myself.
> > My plan will be to wait until Thursday or Friday of this week (to give
> > people a bit of time to object) and then land patch #1. Then I'll
> > snooze things for a while and poke Abhinav and Dmitry to land patch #2
> > / #3 when I notice it in mainline. If, at any point, someone comes out
> > of the woodwork and yells that this is breaking them then, worst case,
> > we can revert.
>
> This plan sounds good to me.

Pushed to drm-misc-next:

55cac10739d5 drm/bridge: tc358762: Set pre_enable_prev_first

If my math is right then I'd expect that to get into mainline for
6.4-rc1. I guess that means it'll be in Linus's tree mid-May. I'll
schedule a reminder to suggest landing at patches #2 and #3 again in
late May.

-Doug

Re: [Freedreno] [PATCH v13 00/13] Add PSR support for eDP

[Doug Anderson]

Hi,

On Wed, Mar 1, 2023 at 11:06 AM Doug Anderson  wrote:
>
> Hi,
>
> On Sun, Feb 12, 2023 at 8:29 AM Vinod Polimera
>  wrote:
> >
> > Changes in v2:
> >   - Use dp bridge to set psr entry/exit instead of dpu_enocder.
> >   - Don't modify whitespaces.
> >   - Set self refresh aware from atomic_check.
> >   - Set self refresh aware only if psr is supported.
> >   - Provide a stub for msm_dp_display_set_psr.
> >   - Move dp functions to bridge code.
> >
> > Changes in v3:
> >   - Change callback names to reflect atomic interfaces.
> >   - Move bridge callback change to separate patch as suggested by Dmitry.
> >   - Remove psr function declaration from msm_drv.h.
> >   - Set self_refresh_aware flag only if psr is supported.
> >   - Modify the variable names to simpler form.
> >   - Define bit fields for PSR settings.
> >   - Add comments explaining the steps to enter/exit psr.
> >   - Change DRM_INFO to drm_dbg_db.
> >
> > Changes in v4:
> >   - Move the get crtc functions to drm_atomic.
> >   - Add atomic functions for DP bridge too.
> >   - Add ternary operator to choose eDP or DP ops.
> >   - Return true/false instead of 1/0.
> >   - mode_valid missing in the eDP bridge ops.
> >   - Move the functions to get crtc into drm_atomic.c.
> >   - Fix compilation issues.
> >   - Remove dpu_assign_crtc and get crtc from drm_enc instead of dpu_enc.
> >   - Check for crtc state enable while reserving resources.
> >
> > Changes in v5:
> >   - Move the mode_valid changes into a different patch.
> >   - Complete psr_op_comp only when isr is set.
> >   - Move the DP atomic callback changes to a different patch.
> >   - Get crtc from drm connector state crtc.
> >   - Move to separate patch for check for crtc state enable while
> > reserving resources.
> >
> > Changes in v6:
> >   - Remove crtc from dpu_encoder_virt struct.
> >   - fix crtc check during vblank toggle crtc.
> >   - Misc changes.
> >
> > Changes in v7:
> >   - Add fix for underrun issue on kasan build.
> >
> > Changes in v8:
> >   - Drop the enc spinlock as it won't serve any purpose in
> > protetcing conn state.(Dmitry/Doug)
> >
> > Changes in v9:
> >   - Update commit message and fix alignment using spaces.(Marijn)
> >   - Misc changes.(Marijn)
> >
> > Changes in v10:
> >   - Get crtc cached in dpu_enc during obj init.(Dmitry)
> >
> > Changes in v11:
> >   - Remove crtc cached in dpu_enc during obj init.
> >   - Update dpu_enc crtc state on crtc enable/disable during self refresh.
> >
> > Changes in v12:
> >   - Update sc7180 intf mask to get intf timing gen status
> > based on DPU_INTF_STATUS_SUPPORTED bit.(Dmitry)
> >   - Remove "clear active interface in the datapath cleanup" change
> > as it is already included.
> >
> > Changes in v13:
> >   - Move core changes to top of the series.(Dmitry)
> >   - Drop self refresh aware disable change after psr entry.(Dmitry)
> >
> > Vinod Polimera (13):
> >   drm: add helper functions to retrieve old and new crtc
> >   drm/bridge: use atomic enable/disable callbacks for panel bridge
> >   drm/bridge: add psr support for panel bridge callbacks
> >   drm/msm/disp/dpu: check for crtc enable rather than crtc active to
> > release shared resources
> >   drm/msm/disp/dpu: get timing engine status from intf status register
> >   drm/msm/disp/dpu: wait for extra vsync till timing engine status is
> > disabled
> >   drm/msm/disp/dpu: reset the datapath after timing engine disable
> >   drm/msm/dp: use atomic callbacks for DP bridge ops
> >   drm/msm/dp: Add basic PSR support for eDP
> >   drm/msm/dp: use the eDP bridge ops to validate eDP modes
> >   drm/msm/disp/dpu: use atomic enable/disable callbacks for encoder
> > functions
> >   drm/msm/disp/dpu: add PSR support for eDP interface in dpu driver
> >   drm/msm/disp/dpu: update dpu_enc crtc state on crtc enable/disable
> > during self refresh
>
> I'm curious what the plan is for landing this series. I could land the
> first two in drm-misc if you want, but I'm a lowly committer and so I
> couldn't make an immutable branch for you nor can I officially Ack the
> changes to land in your branch. That means you'd be blocked for an
> extra version. Do you already have a plan? If not, then maybe we need
> to get in touch with one of the maintainers [1] of drm-misc? That's
> documented [2] to be in their set of responsibilities.
>
> [1] 
> https://drm.pages.freedesktop.org/ma

Re: [Freedreno] [PATCH v13 00/13] Add PSR support for eDP

[Doug Anderson]

Hi,

On Sun, Feb 12, 2023 at 8:29 AM Vinod Polimera
 wrote:
>
> Changes in v2:
>   - Use dp bridge to set psr entry/exit instead of dpu_enocder.
>   - Don't modify whitespaces.
>   - Set self refresh aware from atomic_check.
>   - Set self refresh aware only if psr is supported.
>   - Provide a stub for msm_dp_display_set_psr.
>   - Move dp functions to bridge code.
>
> Changes in v3:
>   - Change callback names to reflect atomic interfaces.
>   - Move bridge callback change to separate patch as suggested by Dmitry.
>   - Remove psr function declaration from msm_drv.h.
>   - Set self_refresh_aware flag only if psr is supported.
>   - Modify the variable names to simpler form.
>   - Define bit fields for PSR settings.
>   - Add comments explaining the steps to enter/exit psr.
>   - Change DRM_INFO to drm_dbg_db.
>
> Changes in v4:
>   - Move the get crtc functions to drm_atomic.
>   - Add atomic functions for DP bridge too.
>   - Add ternary operator to choose eDP or DP ops.
>   - Return true/false instead of 1/0.
>   - mode_valid missing in the eDP bridge ops.
>   - Move the functions to get crtc into drm_atomic.c.
>   - Fix compilation issues.
>   - Remove dpu_assign_crtc and get crtc from drm_enc instead of dpu_enc.
>   - Check for crtc state enable while reserving resources.
>
> Changes in v5:
>   - Move the mode_valid changes into a different patch.
>   - Complete psr_op_comp only when isr is set.
>   - Move the DP atomic callback changes to a different patch.
>   - Get crtc from drm connector state crtc.
>   - Move to separate patch for check for crtc state enable while
> reserving resources.
>
> Changes in v6:
>   - Remove crtc from dpu_encoder_virt struct.
>   - fix crtc check during vblank toggle crtc.
>   - Misc changes.
>
> Changes in v7:
>   - Add fix for underrun issue on kasan build.
>
> Changes in v8:
>   - Drop the enc spinlock as it won't serve any purpose in
> protetcing conn state.(Dmitry/Doug)
>
> Changes in v9:
>   - Update commit message and fix alignment using spaces.(Marijn)
>   - Misc changes.(Marijn)
>
> Changes in v10:
>   - Get crtc cached in dpu_enc during obj init.(Dmitry)
>
> Changes in v11:
>   - Remove crtc cached in dpu_enc during obj init.
>   - Update dpu_enc crtc state on crtc enable/disable during self refresh.
>
> Changes in v12:
>   - Update sc7180 intf mask to get intf timing gen status
> based on DPU_INTF_STATUS_SUPPORTED bit.(Dmitry)
>   - Remove "clear active interface in the datapath cleanup" change
> as it is already included.
>
> Changes in v13:
>   - Move core changes to top of the series.(Dmitry)
>   - Drop self refresh aware disable change after psr entry.(Dmitry)
>
> Vinod Polimera (13):
>   drm: add helper functions to retrieve old and new crtc
>   drm/bridge: use atomic enable/disable callbacks for panel bridge
>   drm/bridge: add psr support for panel bridge callbacks
>   drm/msm/disp/dpu: check for crtc enable rather than crtc active to
> release shared resources
>   drm/msm/disp/dpu: get timing engine status from intf status register
>   drm/msm/disp/dpu: wait for extra vsync till timing engine status is
> disabled
>   drm/msm/disp/dpu: reset the datapath after timing engine disable
>   drm/msm/dp: use atomic callbacks for DP bridge ops
>   drm/msm/dp: Add basic PSR support for eDP
>   drm/msm/dp: use the eDP bridge ops to validate eDP modes
>   drm/msm/disp/dpu: use atomic enable/disable callbacks for encoder
> functions
>   drm/msm/disp/dpu: add PSR support for eDP interface in dpu driver
>   drm/msm/disp/dpu: update dpu_enc crtc state on crtc enable/disable
> during self refresh

I'm curious what the plan is for landing this series. I could land the
first two in drm-misc if you want, but I'm a lowly committer and so I
couldn't make an immutable branch for you nor can I officially Ack the
changes to land in your branch. That means you'd be blocked for an
extra version. Do you already have a plan? If not, then maybe we need
to get in touch with one of the maintainers [1] of drm-misc? That's
documented [2] to be in their set of responsibilities.

[1] 
https://drm.pages.freedesktop.org/maintainer-tools/repositories.html#drm-misc-repository
[2] 
https://drm.pages.freedesktop.org/maintainer-tools/maintainer-drm-misc.html#maintainer-s-duties

-Doug

Re: [Freedreno] [RFT PATCH v2 1/3] drm/bridge: tc358762: Set pre_enable_prev_first

[Doug Anderson]

Hi,

On Wed, Feb 1, 2023 at 1:51 AM Dave Stevenson
 wrote:
>
> On Tue, 31 Jan 2023 at 22:22, Douglas Anderson  wrote:
> >
> > Set the "pre_enable_prev_first" as provided by commit 4fb912e5e190
> > ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init
> > order"). This should allow us to revert commit ec7981e6c614
> > ("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") and
> > commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> > time").
>
> I see no reference in the TC358762 datasheet to requiring the DSI
> interface to be in any particular state.
> However, setting this flag does mean that the DSI host doesn't need to
> power up and down for each host_transfer request from
> tc358762_pre_enable/tc358762_init, so on that basis I'm good with it.
>
> Reviewed-by: Dave Stevenson 
>
> > Cc: Dave Stevenson 
> > Cc: Dmitry Baryshkov 
> > Cc: Abhinav Kumar 
> > Signed-off-by: Douglas Anderson 
> > ---
> >
> > (no changes since v1)
> >
> >  drivers/gpu/drm/bridge/tc358762.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358762.c 
> > b/drivers/gpu/drm/bridge/tc358762.c
> > index 0b6a28436885..77f7f7f54757 100644
> > --- a/drivers/gpu/drm/bridge/tc358762.c
> > +++ b/drivers/gpu/drm/bridge/tc358762.c
> > @@ -229,6 +229,7 @@ static int tc358762_probe(struct mipi_dsi_device *dsi)
> > ctx->bridge.funcs = _bridge_funcs;
> > ctx->bridge.type = DRM_MODE_CONNECTOR_DPI;
> > ctx->bridge.of_node = dev->of_node;
> > +   ctx->bridge.pre_enable_prev_first = true;
> >
> > drm_bridge_add(>bridge);

Abhinav asked what the plan was for landing this [1]. Since this isn't
urgent, I guess the plan is to land patch #1 in drm-misc-next. Then we
sit and wait until it percolates into mainline and, once it does, then
patch #2 and #3 can land.

Since I have Dave's review I can commit this to drm-misc-next myself.
My plan will be to wait until Thursday or Friday of this week (to give
people a bit of time to object) and then land patch #1. Then I'll
snooze things for a while and poke Abhinav and Dmitry to land patch #2
/ #3 when I notice it in mainline. If, at any point, someone comes out
of the woodwork and yells that this is breaking them then, worst case,
we can revert.

[1] https://lore.kernel.org/r/1f204585-88e2-abae-1216-92f739ac9...@quicinc.com/

Re: [Freedreno] [RFT PATCH v2 3/3] drm/msm/dsi: More properly handle errors in regards to dsi_mgr_bridge_power_on()

[Doug Anderson]

Hi,

On Mon, Feb 13, 2023 at 6:02 PM Abhinav Kumar  wrote:
>
> Hi Doug
>
> Sorry for the delayed response.
>
> On 2/2/2023 2:46 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Feb 2, 2023 at 2:37 PM Abhinav Kumar  
> > wrote:
> >>
> >> Hi Doug
> >>
> >> On 1/31/2023 2:18 PM, Douglas Anderson wrote:
> >>> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> >>> time") the error handling with regards to dsi_mgr_bridge_power_on()
> >>> got a bit worse. Specifically if we failed to power the bridge on then
> >>> nothing would really notice. The modeset function couldn't return an
> >>> error and thus we'd blindly go forward and try to do the pre-enable.
> >>>
> >>> In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time
> >>> for parade-ps8640") we added a special case to move the powerup back
> >>> to pre-enable time for ps8640. When we did that, we didn't try to
> >>> recover the old/better error handling just for ps8640.
> >>>
> >>> In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts
> >>> at modeset") we've now moved the powering up back to exclusively being
> >>> during pre-enable. That means we can add the better error handling
> >>> back in, so let's do it. To do so we'll add a new function
> >>> dsi_mgr_bridge_power_off() that's matches how errors were handled
> >>> prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to
> >>> modeset time").
> >>>
> >>> NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we
> >>> should be calling it in dsi_mgr_bridge_post_disable(). That would make
> >>> some sense, but doing so would change the current behavior and thus
> >>> should be a separate patch. Specifically:
> >>> * dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable()
> >>> even in the slave-DSI case of bonded DSI. We'd need to add special
> >>> handling for this if it's truly needed.
> >>> * dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state()
> >>> midway through the poweroff.
> >>> * dsi_mgr_bridge_post_disable() has a different order of some of the
> >>> poweroffs / IRQ disables.
> >>> For now we'll leave dsi_mgr_bridge_post_disable() alone.
> >>>
> >>> Signed-off-by: Douglas Anderson 
> >>> ---
> >>>
> >>> Changes in v2:
> >>> - ("More properly handle errors...") new for v2.
> >>>
> >>>drivers/gpu/drm/msm/dsi/dsi_manager.c | 32 ++-
> >>>1 file changed, 26 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
> >>> b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> >>> index 2197a54b9b96..28b8012a21f2 100644
> >>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> >>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> >>> @@ -228,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id)
> >>>}
> >>>}
> >>>
> >>> -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> >>> +static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> >>>{
> >>>int id = dsi_mgr_bridge_get_id(bridge);
> >>>struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> >>> @@ -268,14 +268,31 @@ static void dsi_mgr_bridge_power_on(struct 
> >>> drm_bridge *bridge)
> >>>if (is_bonded_dsi && msm_dsi1)
> >>>msm_dsi_host_enable_irq(msm_dsi1->host);
> >>>
> >>> - return;
> >>> + return 0;
> >>>
> >>>host1_on_fail:
> >>>msm_dsi_host_power_off(host);
> >>>host_on_fail:
> >>>dsi_mgr_phy_disable(id);
> >>>phy_en_fail:
> >>> - return;
> >>> + return ret;
> >>> +}
> >>> +
> >>> +static void dsi_mgr_bridge_power_off(struct drm_bridge *bridge)
> >>> +{
> >>> + int id = dsi_mgr_bridge_get_id(bridge);
> >>> + struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> >>> + struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
> >>> + struct mipi_dsi_host *host = msm_dsi->host;
> >>> + bool

Re: [Freedreno] [PATCH v4 0/4] Reserve DSPPs based on user request

[Doug Anderson]

Hi,

On Mon, Feb 13, 2023 at 3:11 AM Kalyan Thota  wrote:
>
> This series will enable color features on sc7280 target which has
> primary panel as eDP
>
> The series removes DSPP allocation based on encoder type and allows
> the DSPP reservation based on user request via CTM.
>
> The series will release/reserve the dpu resources whenever there is
> a CTM enable/disable change so that DSPPs are allocated appropriately.
>
> Kalyan Thota (4):
>   drm/msm/dpu: clear DSPP reservations in rm release
>   drm/msm/dpu: add DSPPs into reservation upon a CTM request
>   drm/msm/dpu: avoid unnecessary check in DPU reservations
>   drm/msm/dpu: manage DPU resources if CTM is requested
>
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 38 
> -
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c  |  2 ++
>  drivers/gpu/drm/msm/msm_atomic.c| 18 ++
>  drivers/gpu/drm/msm/msm_drv.c   |  2 +-
>  drivers/gpu/drm/msm/msm_drv.h   |  1 +
>  5 files changed, 38 insertions(+), 23 deletions(-)

For whatever reason when I use "b4 shazam" on your series it yells:

Patch failed at 0002 drm/msm/dpu: add DSPPs into reservation upon a CTM request
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
error: patch failed: drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c:573
error: drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c: patch does not apply
hint: Use 'git am --show-current-patch=diff' to see the failed patch

...but then I can apply it with `git am --show-current-patch=diff |
patch -p1` and it just applies with fuzz. Presumably Abhniav / Dmitry
can do the same but it would be nice if your patch applied cleanly to
msm-next.

In any case, I tried this patch on both a herobrine (sc7280) and
trogdor (sc7180) based board running msm-next (including the sc7280
patch [1]). In both cases the night light on the internal display
worked fine when using ChromeOS. External night light didn't work on
either of them (as expected) because we don't have the compositor
support yet.

I'm happy enough with:

Tested-by: Douglas Anderson 


[1] 
https://lore.kernel.org/r/1674814487-2112-1-git-send-email-quic_kaly...@quicinc.com/

Re: [Freedreno] [v12] drm/msm/disp/dpu1: add support for dspp sub block flush in sc7280

[Doug Anderson]

Hi,

On Fri, Jan 27, 2023 at 2:15 AM Kalyan Thota  wrote:
>
> Flush mechanism for DSPP blocks has changed in sc7280 family, it
> allows individual sub blocks to be flushed in coordination with
> master flush control.
>
> Representation: master_flush && (PCC_flush | IGC_flush .. etc )
>
> This change adds necessary support for the above design.
>
> Changes in v1:
> - Few nits (Doug, Dmitry)
> - Restrict sub-block flush programming to dpu_hw_ctl file (Dmitry)
>
> Changes in v2:
> - Move the address offset to flush macro (Dmitry)
> - Separate ops for the sub block flush (Dmitry)
>
> Changes in v3:
> - Reuse the DPU_DSPP_xx enum instead of a new one (Dmitry)
>
> Changes in v4:
> - Use shorter version for unsigned int (Stephen)
>
> Changes in v5:
> - Spurious patch please ignore.
>
> Changes in v6:
> - Add SOB tag (Doug, Dmitry)
>
> Changes in v7:
> - Cache flush mask per dspp (Dmitry)
> - Few nits (Marijn)
>
> Changes in v8:
> - Few nits (Marijn)
>
> Changes in v9:
> - Use DSPP enum while accessing flush mask to make it readable (Dmitry)
> - Few nits (Dmitry)
>
> Changes in v10:
> - Fix white spaces in a separate patch (Dmitry)
>
> Changes in v11:
> - Define a macro for dspp flush selection (Marijn)
> - Few nits (Marijn)
>
> Changes in v12:
> - Minor comments (reorder macros and a condition) (Marijn)
>
> Signed-off-by: Kalyan Thota 
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c   |  2 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c |  5 ++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |  4 +++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 49 
> +++---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h |  5 ++-
>  5 files changed, 58 insertions(+), 7 deletions(-)

There's a (trivial to resolve) merge conflict when applying this patch
against msm-next. I dunno if that means you should send a v13?

In any case, when using this patch together with the DSPP series [1]
the internal night light works on sc7280-herobrine based boards. Thus:

Tested-by: Douglas Anderson 


[1] 
https://lore.kernel.org/r/1676286704-818-1-git-send-email-quic_kaly...@quicinc.com/

Re: [Freedreno] [PATCH v3 0/4] Reserve DSPPs based on user request

[Doug Anderson]

Hi,

On Thu, Feb 9, 2023 at 3:26 AM Kalyan Thota  wrote:
>
> Kindly ignore my previous email. Sent too early !!
>
> We have tested the changes on top of tip: 
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+log/refs/heads/chromeos-5.15
>  + 5 CTM patches ( that you have quoted )
> We didn’t see the issue that you have reported on herobrine. Night light 
> always came up on primary display as the reservation with dspp was 
> successful.  Are you seeing any reservation failures for primary display ?
>
> Attached a debug patch, can you share the logs when you see the issue.

Sounds good. Since officially this hardware is not available to the
public at this time, I have shared the `dmesg` privately to your (and
Abhinav's) Google partner accounts. Yell if you don't see the
notificatioin. I don't have any reason to believe there's anything
secret in the dmesg, but it didn't seem worth the time to fully audit
it.

For that dmesg, I:

1. Made sure that night light was off.

2. Updated the kernel with the 5 patches + the debug patch.

3. Booted up and logged into ChromeOS

4. Tried turning night light off/on several times and saw nothing on
dmesg while I did this (and night light didn't work)

5. Unplugged power and servo (just to make sure they didn't interfere)

6. Echoed "DOUG: plug in external display now" several times to "/dev/kmsg"

7. Plugged in my external display, which is behind a Type C dock

8. Turned night light on/off several times. Night light worked on the
internal display.

In case it matters, my ChromeOS root filesystem is R111-15328.0.0


> >-Original Message-
> >From: Kalyan Thota
> >Sent: Thursday, February 9, 2023 9:47 AM
> >To: Doug Anderson ; Kalyan Thota (QUIC)
> >
> >Cc: dri-de...@lists.freedesktop.org; linux-arm-...@vger.kernel.org;
> >freedreno@lists.freedesktop.org; devicet...@vger.kernel.org; linux-
> >ker...@vger.kernel.org; robdcl...@chromium.org; swb...@chromium.org;
> >Vinod Polimera (QUIC) ;
> >dmitry.barysh...@linaro.org; Abhinav Kumar (QUIC)
> >; marijn.suij...@somainline.org
> >Subject: RE: [PATCH v3 0/4] Reserve DSPPs based on user request
> >
> >Hi Doug,
> >
> >Have you picked the core change to program dspp's  (below) ? the current 
> >series
> >will go on top of it.
> >https://patchwork.kernel.org/project/linux-arm-msm/patch/1671542719-12655-
> >1-git-send-email-quic_kaly...@quicinc.com/
> >
> >Thanks,
> >Kalyan
> >
> >>-Original Message-
> >>From: Doug Anderson 
> >>Sent: Wednesday, February 8, 2023 10:44 PM
> >>To: Kalyan Thota (QUIC) 
> >>Cc: dri-de...@lists.freedesktop.org; linux-arm-...@vger.kernel.org;
> >>freedreno@lists.freedesktop.org; devicet...@vger.kernel.org; linux-
> >>ker...@vger.kernel.org; robdcl...@chromium.org; swb...@chromium.org;
> >>Vinod Polimera (QUIC) ;
> >>dmitry.barysh...@linaro.org; Abhinav Kumar (QUIC)
> >>; marijn.suij...@somainline.org
> >>Subject: Re: [PATCH v3 0/4] Reserve DSPPs based on user request
> >>
> >>WARNING: This email originated from outside of Qualcomm. Please be wary
> >>of any links or attachments, and do not enable macros.
> >>
> >>Hi,
> >>
> >>On Wed, Feb 8, 2023 at 5:42 AM Kalyan Thota 
> >>wrote:
> >>>
> >>> This series will enable color features on sc7280 target which has
> >>> primary panel as eDP
> >>>
> >>> The series removes DSPP allocation based on encoder type and allows
> >>> the DSPP reservation based on user request via CTM.
> >>>
> >>> The series will release/reserve the dpu resources when ever there is
> >>> a topology change to suit the new requirements.
> >>>
> >>> Kalyan Thota (4):
> >>>   drm/msm/dpu: clear DSPP reservations in rm release
> >>>   drm/msm/dpu: add DSPPs into reservation upon a CTM request
> >>>   drm/msm/dpu: avoid unnecessary check in DPU reservations
> >>>   drm/msm/dpu: reserve the resources on topology change
> >>>
> >>>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h|  2 +
> >>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 58
> >>> --
> >>---
> >>>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c  |  2 +
> >>>  3 files changed, 37 insertions(+), 25 deletions(-)
> >>
> >>I tried out your changes, but unfortunately it seems like there's something
> >wrong.
> >>:( I did this:
> >>
> >>1. Picked your 5 patches to the chromeos-5.15 tree (this series plus

Re: [Freedreno] [PATCH v3 0/4] Reserve DSPPs based on user request

[Doug Anderson]

Hi,

On Wed, Feb 8, 2023 at 8:16 PM Kalyan Thota  wrote:
>
> Hi Doug,
>
> Have you picked the core change to program dspp's  (below) ? the current 
> series will go on top of it.
> https://patchwork.kernel.org/project/linux-arm-msm/patch/1671542719-12655-1-git-send-email-quic_kaly...@quicinc.com/

I didn't pick v11 of it like you link, but I did pick v12 of the same
patch. In my response I said that I picked 5 patches, this series plus
[1] where [1] is:

[1] 
https://lore.kernel.org/all/1674814487-2112-1-git-send-email-quic_kaly...@quicinc.com/


> Thanks,
> Kalyan
>
> >-----Original Message-
> >From: Doug Anderson 
> >Sent: Wednesday, February 8, 2023 10:44 PM
> >To: Kalyan Thota (QUIC) 
> >Cc: dri-de...@lists.freedesktop.org; linux-arm-...@vger.kernel.org;
> >freedreno@lists.freedesktop.org; devicet...@vger.kernel.org; linux-
> >ker...@vger.kernel.org; robdcl...@chromium.org; swb...@chromium.org;
> >Vinod Polimera (QUIC) ;
> >dmitry.barysh...@linaro.org; Abhinav Kumar (QUIC)
> >; marijn.suij...@somainline.org
> >Subject: Re: [PATCH v3 0/4] Reserve DSPPs based on user request
> >
> >WARNING: This email originated from outside of Qualcomm. Please be wary of
> >any links or attachments, and do not enable macros.
> >
> >Hi,
> >
> >On Wed, Feb 8, 2023 at 5:42 AM Kalyan Thota 
> >wrote:
> >>
> >> This series will enable color features on sc7280 target which has
> >> primary panel as eDP
> >>
> >> The series removes DSPP allocation based on encoder type and allows
> >> the DSPP reservation based on user request via CTM.
> >>
> >> The series will release/reserve the dpu resources when ever there is a
> >> topology change to suit the new requirements.
> >>
> >> Kalyan Thota (4):
> >>   drm/msm/dpu: clear DSPP reservations in rm release
> >>   drm/msm/dpu: add DSPPs into reservation upon a CTM request
> >>   drm/msm/dpu: avoid unnecessary check in DPU reservations
> >>   drm/msm/dpu: reserve the resources on topology change
> >>
> >>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h|  2 +
> >>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 58 --
> >---
> >>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c  |  2 +
> >>  3 files changed, 37 insertions(+), 25 deletions(-)
> >
> >I tried out your changes, but unfortunately it seems like there's something 
> >wrong.
> >:( I did this:
> >
> >1. Picked your 5 patches to the chromeos-5.15 tree (this series plus [1])
> >
> >2. Put them on herobrine villager.
> >
> >3. Booted up with no external display plugged in.
> >
> >4. Tried to enable night light in the ChromeOS UI.
> >
> >5. Night light didn't turn on for the internal display.
> >
> >
> >I also tried applying them to the top of msm-next (had to resolve some small
> >conflicts). Same thing, night light didn't work.
> >
> >
> >I thought maybe this was because the Chrome browser hasn't been updated to
> >properly use atomic_check for testing for night light, so I hacked my 
> >herobrine
> >device tree to not mark "mdss_dp" as "okay". Now there's _only_ an eDP 
> >display.
> >Same thing, night light didn't work.
> >
> >
> >I could only get night light to work for the internal display if I plugged 
> >and
> >unplugged an external display in.
> >
> >
> >Is the above the behavior that's expected right now?
> >
> >
> >[1] https://lore.kernel.org/all/1674814487-2112-1-git-send-email-
> >quic_kaly...@quicinc.com/

Re: [Freedreno] [PATCH v3 0/4] Reserve DSPPs based on user request

[Doug Anderson]

Hi,

On Wed, Feb 8, 2023 at 5:42 AM Kalyan Thota  wrote:
>
> This series will enable color features on sc7280 target which has
> primary panel as eDP
>
> The series removes DSPP allocation based on encoder type and allows
> the DSPP reservation based on user request via CTM.
>
> The series will release/reserve the dpu resources when ever there is
> a topology change to suit the new requirements.
>
> Kalyan Thota (4):
>   drm/msm/dpu: clear DSPP reservations in rm release
>   drm/msm/dpu: add DSPPs into reservation upon a CTM request
>   drm/msm/dpu: avoid unnecessary check in DPU reservations
>   drm/msm/dpu: reserve the resources on topology change
>
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h|  2 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 58 
> -
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c  |  2 +
>  3 files changed, 37 insertions(+), 25 deletions(-)

I tried out your changes, but unfortunately it seems like there's
something wrong. :( I did this:

1. Picked your 5 patches to the chromeos-5.15 tree (this series plus [1])

2. Put them on herobrine villager.

3. Booted up with no external display plugged in.

4. Tried to enable night light in the ChromeOS UI.

5. Night light didn't turn on for the internal display.


I also tried applying them to the top of msm-next (had to resolve some
small conflicts). Same thing, night light didn't work.


I thought maybe this was because the Chrome browser hasn't been
updated to properly use atomic_check for testing for night light, so I
hacked my herobrine device tree to not mark "mdss_dp" as "okay". Now
there's _only_ an eDP display. Same thing, night light didn't work.


I could only get night light to work for the internal display if I
plugged and unplugged an external display in.


Is the above the behavior that's expected right now?


[1] 
https://lore.kernel.org/all/1674814487-2112-1-git-send-email-quic_kaly...@quicinc.com/

Re: [Freedreno] [RFT PATCH v2 3/3] drm/msm/dsi: More properly handle errors in regards to dsi_mgr_bridge_power_on()

[Doug Anderson]

Hi,

On Thu, Feb 2, 2023 at 2:37 PM Abhinav Kumar  wrote:
>
> Hi Doug
>
> On 1/31/2023 2:18 PM, Douglas Anderson wrote:
> > In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> > time") the error handling with regards to dsi_mgr_bridge_power_on()
> > got a bit worse. Specifically if we failed to power the bridge on then
> > nothing would really notice. The modeset function couldn't return an
> > error and thus we'd blindly go forward and try to do the pre-enable.
> >
> > In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time
> > for parade-ps8640") we added a special case to move the powerup back
> > to pre-enable time for ps8640. When we did that, we didn't try to
> > recover the old/better error handling just for ps8640.
> >
> > In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts
> > at modeset") we've now moved the powering up back to exclusively being
> > during pre-enable. That means we can add the better error handling
> > back in, so let's do it. To do so we'll add a new function
> > dsi_mgr_bridge_power_off() that's matches how errors were handled
> > prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to
> > modeset time").
> >
> > NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we
> > should be calling it in dsi_mgr_bridge_post_disable(). That would make
> > some sense, but doing so would change the current behavior and thus
> > should be a separate patch. Specifically:
> > * dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable()
> >even in the slave-DSI case of bonded DSI. We'd need to add special
> >handling for this if it's truly needed.
> > * dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state()
> >midway through the poweroff.
> > * dsi_mgr_bridge_post_disable() has a different order of some of the
> >poweroffs / IRQ disables.
> > For now we'll leave dsi_mgr_bridge_post_disable() alone.
> >
> > Signed-off-by: Douglas Anderson 
> > ---
> >
> > Changes in v2:
> > - ("More properly handle errors...") new for v2.
> >
> >   drivers/gpu/drm/msm/dsi/dsi_manager.c | 32 ++-
> >   1 file changed, 26 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
> > b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > index 2197a54b9b96..28b8012a21f2 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > @@ -228,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id)
> >   }
> >   }
> >
> > -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> > +static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> >   {
> >   int id = dsi_mgr_bridge_get_id(bridge);
> >   struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> > @@ -268,14 +268,31 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge 
> > *bridge)
> >   if (is_bonded_dsi && msm_dsi1)
> >   msm_dsi_host_enable_irq(msm_dsi1->host);
> >
> > - return;
> > + return 0;
> >
> >   host1_on_fail:
> >   msm_dsi_host_power_off(host);
> >   host_on_fail:
> >   dsi_mgr_phy_disable(id);
> >   phy_en_fail:
> > - return;
> > + return ret;
> > +}
> > +
> > +static void dsi_mgr_bridge_power_off(struct drm_bridge *bridge)
> > +{
> > + int id = dsi_mgr_bridge_get_id(bridge);
> > + struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> > + struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
> > + struct mipi_dsi_host *host = msm_dsi->host;
> > + bool is_bonded_dsi = IS_BONDED_DSI();
> > +
> > + msm_dsi_host_disable_irq(host);
> > + if (is_bonded_dsi && msm_dsi1) {
> > + msm_dsi_host_disable_irq(msm_dsi1->host);
> > + msm_dsi_host_power_off(msm_dsi1->host);
> > + }
>
> The order of disabling the IRQs should be opposite of how they were enabled.
>
> So while enabling it was DSI0 and then DSI1.
>
> Hence while disabling it should be DSI1 and then DSI0.
>
> So the order here should be
>
> DSI1 irq disable
> DSI0 irq disable
> DSI1 host power off
> DSI0 host power off

Right. Normally you want to go opposite. I guess a few points, though:

1. As talked about in the commit message, the order I have matches the
order we had prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host
powerup to modeset time").

2. I'd be curious if it matters. The order you request means we need
to check for `(is_bonded_dsi && msm_dsi1)` twice. While that's not a
big deal if it's important, it's nice not to have to do so.

3. As talked about in the commit message, eventually we should
probably resolve this order with the order of things in
dsi_mgr_bridge_post_disable(), which is yet a different ordering.
Ideally this resolution would be done by someone who actually has
proper documentation of the hardware and how it's supposed to work
(AKA not me).

So my preference would be to either land or drop ${SUBJECT} patch
(either is fine with me) and then someone at 

Re: [Freedreno] [RFT PATCH v2 2/3] drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset

[Doug Anderson]

Hi,

On Tue, Jan 31, 2023 at 3:32 PM Abhinav Kumar  wrote:
>
> On 1/31/2023 2:18 PM, Douglas Anderson wrote:
> > In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> > time"), we moved powering up DSI hosts to modeset time. This wasn't
> > because it was an elegant design, but there were no better options.
> >
> > That commit actually ended up breaking ps8640, and thus was born
> > commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for
> > parade-ps8640") as a temporary hack to un-break ps8640 by moving it to
> > the old way of doing things. It turns out that ps8640 _really_ doesn't
> > like its pre_enable() function to be called after
> > dsi_mgr_bridge_power_on(). Specifically (from experimentation, not
> > because I have any inside knowledge), it looks like the assertion of
> > "RST#" in the ps8640 runtime resume handler seems like it's not
> > allowed to happen after dsi_mgr_bridge_power_on()
> >
> > Recently, Dave Stevenson's series landed allowing bridges some control
> > over pre_enable ordering. The meaty commit for our purposes is commit
> > 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
> > bridge init order"). As documented by that series, if a bridge doesn't
> > set "pre_enable_prev_first" then we should use the old ordering.
> >
> > Now that we have the commit ("drm/bridge: tc358762: Set
> > pre_enable_prev_first") we can go back to the old ordering, which also
> > allows us to remove the ps8640 special case.
> >
> > One last note is that even without reverting commit 7d8e9a90509f
> > ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_
> > revert the ps8640 special case and try it out then it doesn't seem to
> > fail anymore. I spent time bisecting / debugging this and it turns out
> > to be mostly luck, so we still want this patch to make sure it's
> > solid. Specifically the reason it sorta works these days is because
> > we implemented wait_hpd_asserted() in ps8640 now, plus the magic of
> > "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted()
> > implemented means that we actually power the bridge chip up just a wee
> > bit earlier and then the bridge happens to stay on because of
> > autosuspend and thus ends up powered before dsi_mgr_bridge_power_on().
> >
> > Cc: Dave Stevenson 
> > Cc: Dmitry Baryshkov 
> > Cc: Abhinav Kumar 
> > Signed-off-by: Douglas Anderson 
> > ---
> >
> > Changes in v2:
> > - Don't fold dsi_mgr_bridge_power_on() back into dsi_mgr_bridge_pre_enable()
> >
> >   drivers/gpu/drm/msm/dsi/dsi_manager.c | 38 +--
> >   1 file changed, 1 insertion(+), 37 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
> > b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > index 1bbac72dad35..2197a54b9b96 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > @@ -34,32 +34,6 @@ static struct msm_dsi_manager msm_dsim_glb;
> >   #define IS_SYNC_NEEDED()(msm_dsim_glb.is_sync_needed)
> >   #define IS_MASTER_DSI_LINK(id)  (msm_dsim_glb.master_dsi_link_id == 
> > id)
> >
> > -#ifdef CONFIG_OF
> > -static bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
> > -{
> > - struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge);
> > -
> > - /*
> > -  * If the next bridge in the chain is the Parade ps8640 bridge chip
> > -  * then don't power on early since it seems to violate the 
> > expectations
> > -  * of the firmware that the bridge chip is running.
> > -  *
> > -  * NOTE: this is expected to be a temporary special case. It's 
> > expected
> > -  * that we'll eventually have a framework that allows the next level
> > -  * bridge to indicate whether it needs us to power on before it or
> > -  * after it. When that framework is in place then we'll use it and
> > -  * remove this special case.
> > -  */
> > - return !(next_bridge && next_bridge->of_node &&
> > -  of_device_is_compatible(next_bridge->of_node, 
> > "parade,ps8640"));
> > -}
> > -#else
> > -static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
> > -{
> > - return true;
> > -}
> > -#endif
> > -
> >   static inline struct msm_dsi *dsi_mgr_get_dsi(int id)
> >   {
> >   return msm_dsim_glb.dsi[id];
> > @@ -265,12 +239,6 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge 
> > *bridge)
> >   int ret;
> >
> >   DBG("id=%d", id);
> > - if (!msm_dsi_device_connected(msm_dsi))
> > - return;
> > -
> > - /* Do nothing with the host if it is slave-DSI in case of bonded DSI 
> > */
> > - if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
> > - return;
> >
>
> Why are these two checks removed?

After this patch there is now one caller to this function and the one
caller does those exact same two checks immediately before calling
this function. Thus, they no longer do anything useful.

-Doug

Re: [Freedreno] [RFT PATCH 2/2] drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset

[Doug Anderson]

Hi,

On Fri, Jan 27, 2023 at 10:54 AM Abhinav Kumar
 wrote:
>
> On 1/13/2023 3:56 PM, Douglas Anderson wrote:
> > In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> > time"), we moved powering up DSI hosts to modeset time. This wasn't
> > because it was an elegant design, but there were no better options.
> >
> > That commit actually ended up breaking ps8640, and thus was born
> > commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for
> > parade-ps8640") as a temporary hack to un-break ps8640 by moving it to
> > the old way of doing things. It turns out that ps8640 _really_ doesn't
> > like its pre_enable() function to be called after
> > dsi_mgr_bridge_power_on(). Specifically (from experimentation, not
> > because I have any inside knowledge), it looks like the assertion of
> > "RST#" in the ps8640 runtime resume handler seems like it's not
> > allowed to happen after dsi_mgr_bridge_power_on()
> >
> > Recently, Dave Stevenson's series landed allowing bridges some control
> > over pre_enable ordering. The meaty commit for our purposes is commit
> > 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
> > bridge init order"). As documented by that series, if a bridge doesn't
> > set "pre_enable_prev_first" then we should use the old ordering.
> >
> > Now that we have the commit ("drm/bridge: tc358762: Set
> > pre_enable_prev_first") we can go back to the old ordering, which also
> > allows us to remove the ps8640 special case.
> >
> > One last note is that even without reverting commit 7d8e9a90509f
> > ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_
> > revert the ps8640 special case and try it out then it doesn't seem to
> > fail anymore. I spent time bisecting / debugging this and it turns out
> > to be mostly luck, so we still want this patch to make sure it's
> > solid. Specifically the reason it sorta works these days is because
> > we implemented wait_hpd_asserted() in ps8640 now, plus the magic of
> > "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted()
> > implemented means that we actually power the bridge chip up just a wee
> > bit earlier and then the bridge happens to stay on because of
> > autosuspend and thus ends up powered before dsi_mgr_bridge_power_on().
> >
> > Cc: Dave Stevenson 
> > Cc: Dmitry Baryshkov 
> > Cc: Abhinav Kumar 
> > Signed-off-by: Douglas Anderson 
>
> Why is the patch title showing 2/2? I am not seeing any 1/2 here.

Is it a problem with your mail filters? You can see it at:

https://lore.kernel.org/r/20230113155547.RFT.1.I723a3761d57ea60c5dd754c144aed6c3b2ea6f5a@changeid/

You are listed on the "To:" line. ;-)


> > @@ -349,7 +297,16 @@ static void dsi_mgr_bridge_pre_enable(struct 
> > drm_bridge *bridge)
> >   host1_en_fail:
> >   msm_dsi_host_disable(host);
> >   host_en_fail:
> > -
> > + msm_dsi_host_disable_irq(host);
> > + if (is_bonded_dsi && msm_dsi1) {
> > + msm_dsi_host_disable_irq(msm_dsi1->host);
> > + msm_dsi_host_power_off(msm_dsi1->host);
> > + }
>
> In addition to Dmitry's comment of keeping the bridge_power_on() name,
>
> this part of the change seems independent of the patch. This was missing
> cleanup for DSI1 (esp the disable_irq part).
>
> So can we break it up into two parts.
>
> 1) Add missing cleanup for DSI1
> 2) Just get rid of dsi_mgr_power_on_early() and keep the call
> dsi_mgr_bridge_power_on() in dsi_mgr_bridge_pre_enable() unconditionally.

I didn't intentionally fix any bug in my patch--I just reverted it all
back to how it was before. ;-)

So looking more closely, it looks like overall the current code (AKA
what's landed today and without ${SUBJECT} patch) doesn't really
handle errors with dsi_mgr_bridge_power_on() very well. The normal
case of calling dsi_mgr_bridge_power_on() from modeset is totally
ignored because modeset returns no error. Then the special workaround
for ps8640 just followed the same pattern and assumed that
dsi_mgr_bridge_power_on() succeeded. It also assumed that if the rest
of dsi_mgr_bridge_pre_enable() failed that it didn't need to undo
dsi_mgr_bridge_power_on() because it wouldn't have undone it in the
modeset case.

While the current code isn't the best, it's not like the pre_enable()
call could have returned errors anyway. It probably wasn't truly the
end of the world to behave the way it did.

With all that, I guess my plan would be to do as Dmitry says and just
always call dsi_mgr_bridge_power_on() from
dsi_mgr_bridge_pre_enable(). In the first patch I'll just do that and
remove the ps8640 workaround. Then I can add a 2nd patch that improves
the error handling by having dsi_mgr_bridge_power_on() return an error
code and then adding a matching dsi_mgr_bridge_power_off() that will
undo it and include the extra cleanup.

-Doug

Re: [Freedreno] [RFT PATCH 2/2] drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset

[Doug Anderson]

Hi,

On Thu, Jan 26, 2023 at 9:49 PM Dmitry Baryshkov
 wrote:
>
> Hi,
>
> On Sat, 14 Jan 2023 at 01:56, Douglas Anderson  wrote:
> >
> > In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> > time"), we moved powering up DSI hosts to modeset time. This wasn't
> > because it was an elegant design, but there were no better options.
> >
> > That commit actually ended up breaking ps8640, and thus was born
> > commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for
> > parade-ps8640") as a temporary hack to un-break ps8640 by moving it to
> > the old way of doing things. It turns out that ps8640 _really_ doesn't
> > like its pre_enable() function to be called after
> > dsi_mgr_bridge_power_on(). Specifically (from experimentation, not
> > because I have any inside knowledge), it looks like the assertion of
> > "RST#" in the ps8640 runtime resume handler seems like it's not
> > allowed to happen after dsi_mgr_bridge_power_on()
> >
> > Recently, Dave Stevenson's series landed allowing bridges some control
> > over pre_enable ordering. The meaty commit for our purposes is commit
> > 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
> > bridge init order"). As documented by that series, if a bridge doesn't
> > set "pre_enable_prev_first" then we should use the old ordering.
> >
> > Now that we have the commit ("drm/bridge: tc358762: Set
> > pre_enable_prev_first") we can go back to the old ordering, which also
> > allows us to remove the ps8640 special case.
> >
> > One last note is that even without reverting commit 7d8e9a90509f
> > ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_
> > revert the ps8640 special case and try it out then it doesn't seem to
> > fail anymore. I spent time bisecting / debugging this and it turns out
> > to be mostly luck, so we still want this patch to make sure it's
> > solid. Specifically the reason it sorta works these days is because
> > we implemented wait_hpd_asserted() in ps8640 now, plus the magic of
> > "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted()
> > implemented means that we actually power the bridge chip up just a wee
> > bit earlier and then the bridge happens to stay on because of
> > autosuspend and thus ends up powered before dsi_mgr_bridge_power_on().
>
> I had a small comment on your patch, but then I was distracted and
> forgot to send it. See below.
>
> >
> > Cc: Dave Stevenson 
> > Cc: Dmitry Baryshkov 
> > Cc: Abhinav Kumar 
> > Signed-off-by: Douglas Anderson 
> > ---
> >
> >  drivers/gpu/drm/msm/dsi/dsi_manager.c | 68 +--
> >  1 file changed, 11 insertions(+), 57 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
> > b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > index 3a1417397283..5e6b8d423b96 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > @@ -34,32 +34,6 @@ static struct msm_dsi_manager msm_dsim_glb;
> >  #define IS_SYNC_NEEDED()   (msm_dsim_glb.is_sync_needed)
> >  #define IS_MASTER_DSI_LINK(id) (msm_dsim_glb.master_dsi_link_id == id)
> >
> > -#ifdef CONFIG_OF
> > -static bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
> > -{
> > -   struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge);
> > -
> > -   /*
> > -* If the next bridge in the chain is the Parade ps8640 bridge chip
> > -* then don't power on early since it seems to violate the 
> > expectations
> > -* of the firmware that the bridge chip is running.
> > -*
> > -* NOTE: this is expected to be a temporary special case. It's 
> > expected
> > -* that we'll eventually have a framework that allows the next level
> > -* bridge to indicate whether it needs us to power on before it or
> > -* after it. When that framework is in place then we'll use it and
> > -* remove this special case.
> > -*/
> > -   return !(next_bridge && next_bridge->of_node &&
> > -of_device_is_compatible(next_bridge->of_node, 
> > "parade,ps8640"));
> > -}
> > -#else
> > -static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
> > -{
> > -   return true;
> > -}
> > -#endif
> > -
> >  static inline struct msm_dsi *dsi_mgr_get_dsi(int id)
> >  {
> > return msm_dsim_glb.dsi[id];
> > @@ -254,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id)
> > }
> >  }
> >
> > -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> > +static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
>
> Can you please keep the dsi_mgr_bridge_power_on() as is and just
> remove the now-legacy dsi_mgr_power_on_early().

By this, I assume you mean keep the function separate but still remove
the call to it from "modeset" and unconditionally call it from
dsi_mgr_bridge_pre_enable(), right?


> >  {
> > int id = dsi_mgr_bridge_get_id(bridge);
> > struct msm_dsi 

Re: [Freedreno] [RFT PATCH 2/2] drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset

[Doug Anderson]

Hi,

On Fri, Jan 13, 2023 at 3:56 PM Douglas Anderson  wrote:
>
> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> time"), we moved powering up DSI hosts to modeset time. This wasn't
> because it was an elegant design, but there were no better options.
>
> That commit actually ended up breaking ps8640, and thus was born
> commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for
> parade-ps8640") as a temporary hack to un-break ps8640 by moving it to
> the old way of doing things. It turns out that ps8640 _really_ doesn't
> like its pre_enable() function to be called after
> dsi_mgr_bridge_power_on(). Specifically (from experimentation, not
> because I have any inside knowledge), it looks like the assertion of
> "RST#" in the ps8640 runtime resume handler seems like it's not
> allowed to happen after dsi_mgr_bridge_power_on()
>
> Recently, Dave Stevenson's series landed allowing bridges some control
> over pre_enable ordering. The meaty commit for our purposes is commit
> 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
> bridge init order"). As documented by that series, if a bridge doesn't
> set "pre_enable_prev_first" then we should use the old ordering.
>
> Now that we have the commit ("drm/bridge: tc358762: Set
> pre_enable_prev_first") we can go back to the old ordering, which also
> allows us to remove the ps8640 special case.
>
> One last note is that even without reverting commit 7d8e9a90509f
> ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_
> revert the ps8640 special case and try it out then it doesn't seem to
> fail anymore. I spent time bisecting / debugging this and it turns out
> to be mostly luck, so we still want this patch to make sure it's
> solid. Specifically the reason it sorta works these days is because
> we implemented wait_hpd_asserted() in ps8640 now, plus the magic of
> "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted()
> implemented means that we actually power the bridge chip up just a wee
> bit earlier and then the bridge happens to stay on because of
> autosuspend and thus ends up powered before dsi_mgr_bridge_power_on().
>
> Cc: Dave Stevenson 
> Cc: Dmitry Baryshkov 
> Cc: Abhinav Kumar 
> Signed-off-by: Douglas Anderson 
> ---
>
>  drivers/gpu/drm/msm/dsi/dsi_manager.c | 68 +--
>  1 file changed, 11 insertions(+), 57 deletions(-)

Does anyone have any comments on this patch series? It would probably
make sense to wait to land until early in a kernel's release cycle, so
perhaps there is no hurry. That being said, it would still be good to
know what the plan is.

Abhinav: I think you're the one that was most concerned with removing
the special case for ps8640. Does that mean you'd be willing to review
this patch?

For whether or not the "tc358762" panel works with the MSM display
driver after this series, are the correct people on this thread?

Thanks!

-Doug

Re: [Freedreno] [PATCH 1/2] drm/msm/dp: Clean up handling of DP AUX interrupts

[Doug Anderson]

Hi,

On Wed, Jan 25, 2023 at 9:13 AM Kuogee Hsieh  wrote:
>
>
> On 1/19/2023 2:53 PM, Douglas Anderson wrote:
> > The DP AUX interrupt handling was a bit of a mess.
> > * There were two functions (one for "native" transfers and one for
> >"i2c" transfers) that were quite similar. It was hard to say how
> >many of the differences between the two functions were on purpose
> >and how many of them were just an accident of how they were coded.
> > * Each function sometimes used "else if" to test for error bits and
> >sometimes didn't and again it was hard to say if this was on purpose
> >or just an accident.
> > * The two functions wouldn't notice whether "unknown" bits were
> >set. For instance, there seems to be a bit "DP_INTR_PLL_UNLOCKED"
> >and if it was set there would be no indication.
> > * The two functions wouldn't notice if more than one error was set.
> >
> > Let's fix this by being more consistent / explicit about what we're
> > doing.
> >
> > By design this could cause different handling for AUX transfers,
> > though I'm not actually aware of any bug fixed as a result of
> > this patch (this patch was created because we simply noticed how odd
> > the old code was by code inspection). Specific notes here:
> > 1. In the old native transfer case if we got "done + wrong address"
> > we'd ignore the "wrong address" (because of the "else if"). Now we
> > won't.
> > 2. In the old native transfer case if we got "done + timeout" we'd
> > ignore the "timeout" (because of the "else if"). Now we won't.
> > 3. In the old native transfer case we'd see "nack_defer" and translate
> > it to the error number for "nack". This differed from the i2c
> > transfer case where "nack_defer" was given the error number for
> > "nack_defer". This 100% can't matter because the only user of this
> > error number treats "nack defer" the same as "nack", so it's clear
> > that the difference between the "native" and "i2c" was pointless
> > here.
> > 4. In the old i2c transfer case if we got "done" plus any error
> > besides "nack" or "defer" then we'd ignore the error. Now we don't.
> > 5. If there is more than one error signaled by the hardware it's
> > possible that we'll report a different one than we used to. I don't
> > know if this matters. If someone is aware of a case this matters we
> > should document it and change the code to make it explicit.
> > 6. One quirk we keep (I don't know if this is important) is that in
> > the i2c transfer case if we see "done + defer" we report that as a
> > "nack". That seemed too intentional in the old code to just drop.
> >
> > After this change we will add extra logging, including:
> > * A warning if we see more than one error bit set.
> > * A warning if we see an unexpected interrupt.
> > * A warning if we get an AUX transfer interrupt when shouldn't.
> >
> > It actually turns out that as a result of this change then at boot we
> > sometimes see an error:
> >[drm:dp_aux_isr] *ERROR* Unexpected DP AUX IRQ 0x0100 when not busy
> > That means that, during init, we are seeing DP_INTR_PLL_UNLOCKED. For
> > now I'm going to say that leaving this error reported in the logs is
> > OK-ish and hopefully it will encourage someone to track down what's
> > going on at init time.
> >
> > One last note here is that this change renames one of the interrupt
> > bits. The bit named "i2c done" clearly was used for native transfers
> > being done too, so I renamed it to indicate this.
> >
> > Signed-off-by: Douglas Anderson 
> > ---
> > I don't have good test coverage for this change and it does have the
> > potential to change behavior. I confirmed that eDP and DP still
> > continue to work OK on one machine. Hopefully folks can test it more.
> >
> >   drivers/gpu/drm/msm/dp/dp_aux.c | 80 -
> >   drivers/gpu/drm/msm/dp/dp_catalog.c |  2 +-
> >   drivers/gpu/drm/msm/dp/dp_catalog.h |  2 +-
> >   3 files changed, 36 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
> > b/drivers/gpu/drm/msm/dp/dp_aux.c
> > index cc3efed593aa..34ad08ae6eb9 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> > @@ -162,47 +162,6 @@ static ssize_t dp_aux_cmd_fifo_rx(struct 
> > dp_aux_private *aux,
> >   return i;
> >   }
> >
> > -static void dp_aux_native_handler(struct dp_aux_private *aux, u32 isr)
> > -{
> > - if (isr & DP_INTR_AUX_I2C_DONE)
> > - aux->aux_error_num = DP_AUX_ERR_NONE;
> > - else if (isr & DP_INTR_WRONG_ADDR)
> > - aux->aux_error_num = DP_AUX_ERR_ADDR;
> > - else if (isr & DP_INTR_TIMEOUT)
> > - aux->aux_error_num = DP_AUX_ERR_TOUT;
> > - if (isr & DP_INTR_NACK_DEFER)
> > - aux->aux_error_num = DP_AUX_ERR_NACK;
> > - if (isr & DP_INTR_AUX_ERROR) {
> > - aux->aux_error_num = DP_AUX_ERR_PHY;
> > - 

Re: [Freedreno] [PATCH 2/2] drm/msm/dp: Return IRQ_NONE for unhandled interrupts

[Doug Anderson]

Hi,

On Wed, Jan 25, 2023 at 9:22 AM Kuogee Hsieh  wrote:
>
> > -void dp_ctrl_isr(struct dp_ctrl *dp_ctrl)
> > +irqreturn_t dp_ctrl_isr(struct dp_ctrl *dp_ctrl)
> >   {
> >   struct dp_ctrl_private *ctrl;
> >   u32 isr;
> > + irqreturn_t ret = IRQ_NONE;
> >
> >   if (!dp_ctrl)
> > - return;
> > + return IRQ_NONE;
> >
> >   ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> >
> >   isr = dp_catalog_ctrl_get_interrupt(ctrl->catalog);
> can you add (!isr) check and return IRQ_NONE here to be consistent with
> dp_aux_isr()?

I could, though it doesn't really buy us a whole lot in this case and
just adds an extra test that's not needed. Here it should be easy for
someone reading the function to see that if "isr == 0" that neither of
the two "if" statements below will fire and we'll return "IRQ_NONE"
anyway.

...that actually made me go back and wonder whether we still needed
the "if" test in dp_aux_isr() or if it too was also redundant. It
turns out that it's not! The previous patch made dp_aux_irq() detect
unexpected interrupts. Thus the "if (!isr)" test earlier is important
because otherwise we'd end up WARNing "Unexpected interrupt:
0x" which would be confusing.

So unless you or others feel strongly that I should add the redundant
test here, I'd rather keep it off. Let me know.

-Doug

Re: [Freedreno] [PATCH v6 2/2] drm/msm/dp: enhance dp controller isr

[Doug Anderson]

Hi,

On Wed, Jan 18, 2023 at 2:34 PM Stephen Boyd  wrote:
>
> Quoting Doug Anderson (2023-01-18 10:29:59)
> > Hi,
> >
> > On Tue, Dec 27, 2022 at 6:16 PM Kuogee Hsieh  
> > wrote:
> > > +
> > > if (isr & DP_INTR_AUX_ERROR) {
> > > aux->aux_error_num = DP_AUX_ERR_PHY;
> > > dp_catalog_aux_clear_hw_interrupts(aux->catalog);
> > > +   ret = IRQ_HANDLED;
> > > }
> >
> > The end result of the above is a weird mix of "if" and "else if" for
> > no apparent reason. All except one of them just updates the exact same
> > variable so doing more than one is mostly useless. If you made it
> > consistently with "else" then the whole thing could be much easier,
> > like this (untested):
>
> Totally agreed. I even asked that when I posted the RFC[1]!
>
> "Can we also simplify the aux handlers to be a big pile of
> if-else-if conditions that don't overwrite the 'aux_error_num'? That
> would simplify the patch below."
>
> > > @@ -425,17 +464,15 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)
> > >
> > > /* no interrupts pending, return immediately */
> > > if (!isr)
> > > -   return;
> > > +   return IRQ_NONE;
> > >
> > > if (!aux->cmd_busy)
> > > -   return;
> > > +   return IRQ_NONE;
> > >
> > > if (aux->native)
> > > -   dp_aux_native_handler(aux, isr);
> > > +   return dp_aux_native_handler(aux, isr);
> > > else
> > > -   dp_aux_i2c_handler(aux, isr);
> > > -
> > > -   complete(>comp);
> > > +   return dp_aux_i2c_handler(aux, isr);
> >
> > Personally, I wouldn't have done it this way. I guess that means I
> > disagree with Stephen. I'm not dead-set against this way and it's fine
> > if you want to continue with it. If I were doing it, though, then I
> > would always return IRQ_HANDLED IF dp_catalog_aux_get_irq() returned
> > anything non-zero. Why? Officially if dp_catalog_aux_get_irq() returns
> > something non-zero then you know for sure that there was an interrupt
> > for this device and officially you have "handled" it by acking it,
> > since dp_catalog_aux_get_irq() acks all the bits that it returns. That
> > means that even if dp_aux_native_handler() or dp_aux_i2c_handler()
> > didn't do anything with the interrupt you at least know that it was
> > for us (so if the IRQ is shared we properly report back to the IRQ
> > subsystem) and that it won't keep firing over and over (because we
> > acked it).
>
> I'm primarily concerned with irq storms taking down the system. Can that
> happen here? If not, then returning IRQ_NONE is not really useful. The
> overall IRQ for DP looks to be level, because the driver requests the
> IRQ that way. The aux interrupt status bits look to be edge style
> interrupts though, because the driver acks them in the handler. I guess
> that means the edges come in and latch into the interrupt status
> register so the driver has to ack all of them to drop the IRQ level for
> the overall DP interrupt? If the driver only acked the bits it looked at
> instead of all interrupt bits in the register, then the level would
> never go down for the IRQ if an unhandled interrupt bit was present like
> 'DP_INTR_PLL_UNLOCKED'. That would mean we would hit spurious IRQ
> handling very quickly if that interrupt bit was ever seen.
>
> But the driver is acking all interrupts, so probably trying to work
> IRQ_NONE into this code is not very useful? The only thing it would
> catch is DP_INTR_PLL_UNLOCKED being set over and over again, which seems
> unlikely. Of course, why is this driver unmasking interrupt bits it
> doesn't care about? That may be leading to useless interrupt handling in
> this driver if some interrupt bit is unmasked but never looked at. Can
> that be fixed in another patch?
>
> >
> > NOTE: I still like having the complete() call in
> > dp_aux_native_handler() and dp_aux_i2c_handler() and, to me, that part
> > of this patch is worthwhile. That makes it more obvious that the code
> > is truly expecting that complete to be called for all error cases as
> > well as transfer finished.
> >
>
> I think it may be required. We don't want to allow DP_INTR_PLL_UNLOCKED
> to complete() the transfer.

OK, I've tried to code up what I think is the right solution. I'd
appreciate review and testing.

https://lore.kernel.org/r/20230119145248.1.I90ffed3ddd21e818ae534f820cb4d6d8638859ab@changeid

-Doug

Re: [Freedreno] [PATCH v6 2/2] drm/msm/dp: enhance dp controller isr

[Doug Anderson]

Hi,

On Tue, Dec 27, 2022 at 6:16 PM Kuogee Hsieh  wrote:
>
> dp_display_irq_handler() is the main isr handler with the helps
> of two sub isr, dp_aux_isr and dp_ctrl_isr, to service all DP
> interrupts on every irq triggered. Current all three isr does
> not return IRQ_HANDLED if there are any interrupts it had
> serviced. This patch fix this ambiguity by having all isr
> return IRQ_HANDLED if there are interrupts had been serviced
> or IRQ_NONE otherwise.
>
> Changes in v5:
> -- move complete into dp_aux_native_handler()
> -- move complete into dp_aux_i2c_handler()
> -- restore null ctrl check at isr
> -- return IRQ_NODE directly
>
> Signed-off-by: Kuogee Hsieh 
> Suggested-by: Stephen Boyd 
> ---
>  drivers/gpu/drm/msm/dp/dp_aux.c | 95 
> ++---
>  drivers/gpu/drm/msm/dp/dp_aux.h |  2 +-
>  drivers/gpu/drm/msm/dp/dp_ctrl.c| 12 -
>  drivers/gpu/drm/msm/dp/dp_ctrl.h|  2 +-
>  drivers/gpu/drm/msm/dp/dp_display.c | 16 +--
>  5 files changed, 89 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index cc3efed..d01ff45 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -162,45 +162,84 @@ static ssize_t dp_aux_cmd_fifo_rx(struct dp_aux_private 
> *aux,
> return i;
>  }
>
> -static void dp_aux_native_handler(struct dp_aux_private *aux, u32 isr)
> +static irqreturn_t dp_aux_native_handler(struct dp_aux_private *aux, u32 isr)
>  {
> -   if (isr & DP_INTR_AUX_I2C_DONE)
> +   irqreturn_t ret = IRQ_NONE;
> +
> +   if (isr & DP_INTR_AUX_I2C_DONE) {
> aux->aux_error_num = DP_AUX_ERR_NONE;
> -   else if (isr & DP_INTR_WRONG_ADDR)
> +   ret = IRQ_HANDLED;
> +   } else if (isr & DP_INTR_WRONG_ADDR) {
> aux->aux_error_num = DP_AUX_ERR_ADDR;
> -   else if (isr & DP_INTR_TIMEOUT)
> +   ret = IRQ_HANDLED;
> +   } else if (isr & DP_INTR_TIMEOUT) {
> aux->aux_error_num = DP_AUX_ERR_TOUT;
> -   if (isr & DP_INTR_NACK_DEFER)
> +   ret = IRQ_HANDLED;
> +   }
> +
> +   if (isr & DP_INTR_NACK_DEFER) {
> aux->aux_error_num = DP_AUX_ERR_NACK;
> +   ret = IRQ_HANDLED;
> +   }
> +
> if (isr & DP_INTR_AUX_ERROR) {
> aux->aux_error_num = DP_AUX_ERR_PHY;
> dp_catalog_aux_clear_hw_interrupts(aux->catalog);
> +   ret = IRQ_HANDLED;
> }

The end result of the above is a weird mix of "if" and "else if" for
no apparent reason. All except one of them just updates the exact same
variable so doing more than one is mostly useless. If you made it
consistently with "else" then the whole thing could be much easier,
like this (untested):

static irqreturn_t dp_aux_native_handler(struct dp_aux_private *aux, u32 isr)
{
if (isr & DP_INTR_AUX_ERROR) {
aux->aux_error_num = DP_AUX_ERR_PHY;
dp_catalog_aux_clear_hw_interrupts(aux->catalog);
} else if (isr & DP_INTR_NACK_DEFER) {
aux->aux_error_num = DP_AUX_ERR_NACK;
} else if (isr & DP_INTR_AUX_I2C_DONE) {
aux->aux_error_num = DP_AUX_ERR_NONE;
} else if (isr & DP_INTR_WRONG_ADDR) {
aux->aux_error_num = DP_AUX_ERR_ADDR;
} else if (isr & DP_INTR_TIMEOUT) {
aux->aux_error_num = DP_AUX_ERR_TOUT;
} else {
return IRQ_NONE;
}

complete(>comp);

return IRQ_HANDLED;
}

Note that I changed the order to make sure that the behavior was
exactly the same (previously later tests without the "if" would
override "aux_error_num" so I moved them first. Also previously
dp_catalog_aux_clear_hw_interrupts() would always be called for the
PHY error even if other errors were present so my new proposal
preserves this behavior.


> +
> +   if (ret == IRQ_HANDLED)
> +   complete(>comp);
> +
> +   return ret;
>  }
>
> -static void dp_aux_i2c_handler(struct dp_aux_private *aux, u32 isr)
> +static irqreturn_t dp_aux_i2c_handler(struct dp_aux_private *aux, u32 isr)
>  {
> +   irqreturn_t ret = IRQ_NONE;
> +
> if (isr & DP_INTR_AUX_I2C_DONE) {
> if (isr & (DP_INTR_I2C_NACK | DP_INTR_I2C_DEFER))
> aux->aux_error_num = DP_AUX_ERR_NACK;
> else
> aux->aux_error_num = DP_AUX_ERR_NONE;
> -   } else {
> -   if (isr & DP_INTR_WRONG_ADDR)
> -   aux->aux_error_num = DP_AUX_ERR_ADDR;
> -   else if (isr & DP_INTR_TIMEOUT)
> -   aux->aux_error_num = DP_AUX_ERR_TOUT;
> -   if (isr & DP_INTR_NACK_DEFER)
> -   aux->aux_error_num = DP_AUX_ERR_NACK_DEFER;
> -   if (isr & DP_INTR_I2C_NACK)
> -   aux->aux_error_num = DP_AUX_ERR_NACK;
> -   if (isr & DP_INTR_I2C_DEFER)
> -   aux->aux_error_num = DP_AUX_ERR_DEFER;
> -  

Re: [Freedreno] [PATCH] drm/msm/dsi: Drop the redundant fail label

[Doug Anderson]

Hi,

On Mon, Jan 9, 2023 at 6:50 PM Jiasheng Jiang  wrote:
>
> @@ -1954,9 +1949,8 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
>
> msm_host->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> if (msm_host->irq < 0) {
> -   ret = msm_host->irq;
> dev_err(>dev, "failed to get irq: %d\n", ret);
> -   return ret;
> +   return msm_host->irq;

The dev_err() is no longer printing the right value of "ret" above.

Other than that this looks reasonable to me. Feel free to add my
Reviewed-by tag once the above bug is fixed.

-Doug

Re: [Freedreno] [PATCH v12 2/5] dt-bindings: msm/dp: add data-lanes and link-frequencies property

[Doug Anderson]

Hi,

On Thu, Dec 15, 2022 at 1:12 PM Dmitry Baryshkov
 wrote:
>
> On 15/12/2022 02:38, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2022-12-14 14:56:23)
> >>
> >> On 12/13/2022 3:06 PM, Stephen Boyd wrote:
> >>> Quoting Kuogee Hsieh (2022-12-13 13:44:05)
>  Add both data-lanes and link-frequencies property into endpoint
> >>> Why do we care? Please tell us why it's important.
> >
> > Any response?
> >
>  @@ -193,6 +217,8 @@ examples:
> reg = <1>;
> endpoint {
> remote-endpoint = <>;
>  +data-lanes = <0 1>;
>  +link-frequencies = /bits/ 64 <162000 27 
>  54 81>;
> };
> >>> So far we haven't used the output port on the DP controller in DT.
> >>>
> >>> I'm still not clear on what we should do in general for DP because
> >>> there's a PHY that actually controls a lane count and lane mapping. In
> >>> my mental model of the SoC, this DP controller's output port is
> >>> connected to the DP PHY, which then sends the DP lanes out of the SoC to
> >>> the next downstream device (i.e. a DP connector or type-c muxer). Having
> >>> a remote-endpoint property with a phandle to typec doesn't fit my mental
> >>> model. I'd expect it to be the typec PHY.
> >> ack
> >>>
> >>> That brings up the question: when we have 2 lanes vs. 4 lanes will we
> >>> duplicate the data-lanes property in the PHY binding? I suspect we'll
> >>> have to. Hopefully that sort of duplication is OK?
> >> Current we have limitation by reserve 2 data lanes for usb2, i am not
> >> sure duplication to 4 lanes will work automatically.
> >>>
> >>> Similarly, we may have a redriver that limits the link-frequencies
> >>> property further (e.g. only support <= 2.7GHz). Having multiple
> >>> link-frequencies along the graph is OK, right? And isn't the
> >>> link-frequencies property known here by fact that the DP controller
> >>> tells us which SoC this controller is for, and thus we already know the
> >>> supported link frequencies?
> >>>
> >>> Finally, I wonder if we should put any of this in the DP controller's
> >>> output endpoint, or if we can put these sorts of properties in the DP
> >>> PHY binding directly? Can't we do that and then when the DP controller
> >>> tries to set 4 lanes, the PHY immediately fails the call and the link
> >>> training algorithm does its thing and tries fewer lanes? And similarly,
> >>> if link-frequencies were in the PHY's binding, the PHY could fail to set
> >>> those frequencies during link training, returning an error to the DP
> >>> controller, letting the training move on to a lower frequency. If we did
> >>> that this patch series would largely be about modifying the PHY binding,
> >>> updating the PHY driver to enforce constraints, and handling errors
> >>> during link training in the DP controller (which may already be done? I
> >>> didn't check).
> >>
> >>
> >> phy/pll have different configuration base on link lanes and rate.
> >>
> >> it has to be set up before link training can start.
> >>
> >> Once link training start, then there are no any interactions between
> >> controller and phy during link training session.
> >
> > What do you mean? The DP controller calls phy_configure() and changes
> > the link rate. The return value from phy_configure() should be checked
> > and link training should skip link rates that aren't supported and/or
> > number of lanes that aren't supported.
>
> I'd toss another coin into the argument. We have previously discussed
> using the link-frequencies property in the context of limiting link
> speeds for the DSI. There we have both hardware (SoC) limitations and
> the board limitations as in some cases the DSI lanes can not sustain
> some high rate. I still hope for these patches to materialize at some point.
>
> For the DP this is more or less the same story. We have the hardware
> (SoC, PHY, etc) limitations, but also we have the board/device
> limitations. For example some of the board might not be able to support
> HBR3 e.g. because of the PCB design. And while it might be logical to
> also add the 'max bit rate' support to the eDP & combo PHYs, it
> definitely makes sense to be able to limit the rate on the DP <->
> `something' link.
>
> Now, for all the practical purposes this `something' for the DP is the
> DP connector, the eDP panel or the USB-C mux (with the possible
> redrivers in the middle).
>
> Thus I'd support Kuogee's proposal to have link-frequencies in the DP's
> outbound endpoint. This is the link which will be driven by the data
> stream from the Linux point of view. The PHY is linked through the
> 'phys' property, but it doesn't participate in the USB-C (or in the
> connector/panel) graph.
>
> Now let's discuss the data lanes. Currently we have them in the DP
> property itself. Please correct me if I'm wrong, but I think that we can
> drop it for all the practical purposes. 

Re: [Freedreno] [PATCH] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer

[Doug Anderson]

Hi,

On Wed, Dec 14, 2022 at 3:46 PM Abhinav Kumar  wrote:
>
> Hi Doug
>
> On 12/14/2022 2:29 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Dec 14, 2022 at 1:21 PM Kuogee Hsieh  
> > wrote:
> >>
> >> There are 3 possible interrupt sources are handled by DP controller,
> >> HPDstatus, Controller state changes and Aux read/write transaction.
> >> At every irq, DP controller have to check isr status of every interrupt
> >> sources and service the interrupt if its isr status bits shows interrupts
> >> are pending. There is potential race condition may happen at current aux
> >> isr handler implementation since it is always complete dp_aux_cmd_fifo_tx()
> >> even irq is not for aux read or write transaction. This may cause aux read
> >> transaction return premature if host aux data read is in the middle of
> >> waiting for sink to complete transferring data to host while irq happen.
> >> This will cause host's receiving buffer contains unexpected data. This
> >> patch fixes this problem by checking aux isr and return immediately at
> >> aux isr handler if there are no any isr status bits set.
> >>
> >> Follows are the signature at kernel logs when problem happen,
> >> EDID has corrupt header
> >> panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID
> >> panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor 
> >> find a fallback
> >>
> >> Signed-off-by: Kuogee Hsieh 
> >> ---
> >>   drivers/gpu/drm/msm/dp/dp_aux.c | 7 +++
> >>   1 file changed, 7 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
> >> b/drivers/gpu/drm/msm/dp/dp_aux.c
> >> index d030a93..8f8b12a 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> >> @@ -423,6 +423,13 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)
> >>
> >>  isr = dp_catalog_aux_get_irq(aux->catalog);
> >>
> >> +   /*
> >> +* if this irq is not for aux transfer,
> >> +* then return immediately
> >> +*/
> >
> > Why do you need 4 lines for a comment that fits on one line?
> Yes, we can fit this to one line.
> >
> >> +   if (!isr)
> >> +   return;
> >
> > I can confirm that this works for me. I could reproduce the EDID
> > problems in the past and I can't after this patch. ...so I could give
> > a:
> >
> > Tested-by: Douglas Anderson 
> >
> > I'm not an expert on this part of the code, so feel free to ignore my
> > other comments if everyone else thinks this patch is fine as-is, but
> > to me something here feels a little fragile. It feels a little weird
> > that we'll "complete" for _any_ interrupt that comes through now
> > rather than relying on dp_aux_native_handler() / dp_aux_i2c_handler()
> > to specifically identify interrupts that caused the end of the
> > transfer. I guess that idea is that every possible interrupt we get
> > causes the end of the transfer?
> >
> > -Doug
>
> So this turned out to be more tricky and was a good finding from kuogee.
>
> In the bad EDID case, it was technically not bad EDID.
>
> What was happening was, the VIDEO_READY interrupt was continuously
> firing. Ideally, this should fire only once but due to some error
> condition it kept firing. We dont exactly know why yet what was the
> error condition making it continuously fire.
>
> In the DP ISR, the dp_aux_isr() gets called even if it was not an aux
> interrupt which fired (so the call flow in this case was
> dp_display_irq_handler (triggered for VIDEO_READY) ---> dp_aux_isr()
> So we should certainly have some protection to return early from this
> routine if there was no aux interrupt which fired.
>
> Which is what this fix is doing.
>
> Its not completing any interrupt, its just returning early if no aux
> interrupt fired.

...but the whole problem was that it was doing the complete() at the
end, right? Kuogee even mentioned that in the commit message.
Specifically, I checked dp_aux_native_handler() and
dp_aux_i2c_handler(), both of which are passed the "isr". Unless I
messed up, both functions already were no-ops if the ISR was 0, even
before Kuogee's patch. That means that the only thing Kuogee's patch
does is to prevent the call to "complete(>comp)" at the end of
"dp_aux_isr()".

...and it makes sense not to call the complete() if no "isr" is 0.
...but what I'm saying is that _any_ non-zero value of ISR will still
cause the complete() to be called after Kuogee's patch. That means
that if any of the 32-bits in the "isr" variable are set, that we will
call complete(). I'm asking if you're sure that every single bit of
the "isr" means that we're ready to call complete(). It feels like it
would be less fragile if dp_aux_native_handler() and
dp_aux_i2c_handler() (which both already look at the ISR) returned
some value saying whether the "isr" contained a bit that meant that
complete() should be called.

-Doug

Re: [Freedreno] [PATCH] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer

[Doug Anderson]

Hi,

On Wed, Dec 14, 2022 at 1:21 PM Kuogee Hsieh  wrote:
>
> There are 3 possible interrupt sources are handled by DP controller,
> HPDstatus, Controller state changes and Aux read/write transaction.
> At every irq, DP controller have to check isr status of every interrupt
> sources and service the interrupt if its isr status bits shows interrupts
> are pending. There is potential race condition may happen at current aux
> isr handler implementation since it is always complete dp_aux_cmd_fifo_tx()
> even irq is not for aux read or write transaction. This may cause aux read
> transaction return premature if host aux data read is in the middle of
> waiting for sink to complete transferring data to host while irq happen.
> This will cause host's receiving buffer contains unexpected data. This
> patch fixes this problem by checking aux isr and return immediately at
> aux isr handler if there are no any isr status bits set.
>
> Follows are the signature at kernel logs when problem happen,
> EDID has corrupt header
> panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID
> panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor 
> find a fallback
>
> Signed-off-by: Kuogee Hsieh 
> ---
>  drivers/gpu/drm/msm/dp/dp_aux.c | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index d030a93..8f8b12a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -423,6 +423,13 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)
>
> isr = dp_catalog_aux_get_irq(aux->catalog);
>
> +   /*
> +* if this irq is not for aux transfer,
> +* then return immediately
> +*/

Why do you need 4 lines for a comment that fits on one line?

> +   if (!isr)
> +   return;

I can confirm that this works for me. I could reproduce the EDID
problems in the past and I can't after this patch. ...so I could give
a:

Tested-by: Douglas Anderson 

I'm not an expert on this part of the code, so feel free to ignore my
other comments if everyone else thinks this patch is fine as-is, but
to me something here feels a little fragile. It feels a little weird
that we'll "complete" for _any_ interrupt that comes through now
rather than relying on dp_aux_native_handler() / dp_aux_i2c_handler()
to specifically identify interrupts that caused the end of the
transfer. I guess that idea is that every possible interrupt we get
causes the end of the transfer?

-Doug

Re: [Freedreno] [PATCH v2] drm/msm/dpu: Fix memory leak in msm_mdss_parse_data_bus_icc_path

[Doug Anderson]

Hi,

On Tue, Dec 6, 2022 at 10:59 PM Miaoqian Lin  wrote:
>
> of_icc_get() alloc resources for path1, we should release it when not
> need anymore. Early return when IS_ERR_OR_NULL(path0) may leak path1.
> Defer getting path1 to fix this.
>
> Fixes: b9364eed9232 ("drm/msm/dpu: Move min BW request and full BW disable 
> back to mdss")
> Signed-off-by: Miaoqian Lin 
> ---
> changes in v2:
> - move getting path1 after error check for path0.
> ---
>  drivers/gpu/drm/msm/msm_mdss.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Douglas Anderson 

Re: [Freedreno] [PATCH] drm/msm/dpu: Fix memory leak in msm_mdss_parse_data_bus_icc_path

[Doug Anderson]

Hi,

On Mon, Dec 5, 2022 at 11:55 PM Miaoqian Lin  wrote:
>
> of_icc_get() alloc resources for path1, we should release it when not
> need anymore. Early return when IS_ERR_OR_NULL(path0) may leak path1.
> Add icc_put(path1) in the error path to fix this.
>
> Fixes: b9364eed9232 ("drm/msm/dpu: Move min BW request and full BW disable 
> back to mdss")
> Signed-off-by: Miaoqian Lin 
> ---
>  drivers/gpu/drm/msm/msm_mdss.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> index e13c5c12b775..a38fa9a9a3d6 100644
> --- a/drivers/gpu/drm/msm/msm_mdss.c
> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> @@ -49,8 +49,10 @@ static int msm_mdss_parse_data_bus_icc_path(struct device 
> *dev,
> struct icc_path *path0 = of_icc_get(dev, "mdp0-mem");
> struct icc_path *path1 = of_icc_get(dev, "mdp1-mem");
>
> -   if (IS_ERR_OR_NULL(path0))
> +   if (IS_ERR_OR_NULL(path0)) {
> +   icc_put(path1);
> return PTR_ERR_OR_ZERO(path0);
> +   }
>
> msm_mdss->path[0] = path0;
> msm_mdss->num_paths = 1;

Hmmm. I guess the original author of the code (which wasn't me--I just
restored the code that was deleted by a previous change) was assuming
that if mdp0-mem had a problem that mdp1-mem would also have a
problem. That would mean that you wouldn't need to call icc_put() on
it.

...and, in fact, your patch doesn't handle that case, does it? If
_both_ of the two are error or NULL then you'll be calling icc_put()
on something invalid. I guess icc_put() handles those cases without
crashing but it will give a WARN_ON() splat if it happens to be an
error...

Really, there's a better solution anyway. Instead, you should do:

path0 = of_icc_get(dev, "mdp0-mem");
if (IS_ERR_OR_NULL(path0))
  return PTR_ERR_OR_ZERO(path0);

msm_mdss->path[0] = path0;
msm_mdss->num_paths = 1;

path1 = of_icc_get(dev, "mdp1-mem");
if (!IS_ERR_OR_NULL(path1)) {
 ...
}

In other words just defer getting path1 until after you've checked
path0 for an error.

-Doug

Re: [Freedreno] [PATCH] drm/msm: Enable clamp_to_idle for 7c3

[Doug Anderson]

Hi,

On Tue, Nov 15, 2022 at 7:55 AM Rob Clark  wrote:
>
> From: Rob Clark 
>
> This was overlooked.
>
> Signed-off-by: Rob Clark 
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)

Reviewed-by: Douglas Anderson 

Re: [Freedreno] [PATCH v2] drm/msm/a6xx: Fix speed-bin detection vs probe-defer

[Doug Anderson]

Hi,

On Mon, Nov 14, 2022 at 12:50 PM Rob Clark  wrote:
>
> From: Rob Clark 
>
> If we get an error (other than -ENOENT) we need to propagate that up the
> stack.  Otherwise if the nvmem driver hasn't probed yet, we'll end up
> end up claiming that we support all the OPPs which is not likely to be
> true (and on some generations impossible to be true, ie. if there are
> conflicting OPPs).
>
> v2: Update commit msg, gc unused label, etc
>
> Fixed: fe7952c629da ("drm/msm: Add speed-bin support to a618 gpu")
> Signed-off-by: Rob Clark 
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 10 --
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c 
> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 7fe60c65a1eb..6ae77e88060f 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -1941,7 +1941,7 @@ static u32 fuse_to_supp_hw(struct device *dev, struct 
> adreno_rev rev, u32 fuse)
>
>  static int a6xx_set_supported_hw(struct device *dev, struct adreno_rev rev)
>  {
> -   u32 supp_hw = UINT_MAX;
> +   u32 supp_hw;
> u32 speedbin;
> int ret;
>
> @@ -1953,15 +1953,13 @@ static int a6xx_set_supported_hw(struct device *dev, 
> struct adreno_rev rev)
> if (ret == -ENOENT) {
> return 0;
> } else if (ret) {
> -   DRM_DEV_ERROR(dev,
> - "failed to read speed-bin (%d). Some OPPs may 
> not be supported by hardware",
> - ret);
> -   goto done;
> +   dev_err_probe(dev, ret,
> + "failed to read speed-bin. Some OPPs may not be 
> supported by hardware");
> +   return ret;

Both before and after this change, I think you're missing a "\n" at
the end of your error string?

If you want to get even fancier, dev_err_probe is designed to run
"braceless" and returns "ret" as its return value. This you could do:

if (ret == -ENOENT)
  return ret;
else if (ret)
  return dev_err_probe(dev, ret, ...)

After adding the "\n" then either with the extra fanciness or as you have it:

Reviewed-by: Douglas Anderson 

-Doug

Re: [Freedreno] [PATCH] drm/msm/a6xx: Fix speed-bin detection vs probe-defer

[Doug Anderson]

Hi,

On Mon, Nov 14, 2022 at 11:41 AM Rob Clark  wrote:
>
> From: Rob Clark 
>
> If we get an error (other than -ENOENT) we need to propagate that up the
> stack.  Otherwise if the nvmem driver hasn't probed yet, we'll end up with
> whatever OPP(s) are represented by bit zero.

Can you explain the "whatever OPP(s) are represented by bit zero"
part? This doesn't seem to be true because `supp_hw` is initiated to
UINT_MAX. If I'm remembering how this all works, doesn't that mean
that if we get an error we'll assume all OPPs are OK?

I'm not saying that I'm against your change, but I think maybe you're
misdescribing the old behavior.

Speaking of the initialization of supp_hw, if we want to change the
behavior like your patch does then we should be able to remove that
initialization, right?

I would also suspect that your patch will result in a compiler
warning, at least on some compilers. The goto label `done` is no
longer needed, right?

-Doug

Re: [Freedreno] [PATCH] drm/msm/dp: remove limitation of link rate at 5.4G to support HBR3

[Doug Anderson]

Hi,

On Wed, Nov 2, 2022 at 10:23 AM Dmitry Baryshkov
 wrote:
>
> > 1. Someone figures out how to model this with the bridge chain and
> > then we only allow HBR3 if we detect we've got a TCPC that supports
> > it. This seems like the cleanest / best but feels like a long pole.
> > Not only have we been trying to get the TCPC-modeled-as-a-bridge stuff
> > landed for a long time but even when we do it we still don't have a
> > solution for how to communicate the number of lanes and other stuff
> > between the TCPC and the DP controller so we have to enrich the bridge
> > interface.
>
> I think we'd need some OOB interface. For example for DSI interfaces we
> have mipi_dsi_device struct to communicate such OOB data.
>
> Also take a note regarding data-lanes from my previous email.

Right, we can somehow communicate the max link rate through the bridge
chain to the DP controller in an OOB manner that would work.


> > 2. We add in a DT property to the display controller node that says
> > the max link rate for use on this board. This feels like a hack, but
> > maybe it's not too bad. Certainly it would be incredibly simple to
> > implement. Actually... ...one could argue that even if we later model
> > the TCPC as a bridge that this property would still be valid / useful!
> > You could certainly imagine that the SoC supports HBR3 and the TCPC
> > supports HBR3 but that the board routing between the SoC and the TCPC
> > is bad and only supports HBR2. In this case the only way out is
> > essentially a "board constraint" AKA a DT property in the DP
> > controller.
>
> We have been discussing similar topics with Abhinav. Krzysztof suggested
> using link-frequencies property to provide max and min values.

This sounds good to me and seems worth doing even if we eventually do #1.

Re: [Freedreno] [PATCH] drm/msm/dp: remove limitation of link rate at 5.4G to support HBR3

[Doug Anderson]

Hi,

On Wed, Nov 2, 2022 at 10:15 AM Dmitry Baryshkov
 wrote:
>
> On 01/11/2022 17:37, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Oct 31, 2022 at 5:15 PM Dmitry Baryshkov
> >  wrote:
> >>
> >> On 01/11/2022 03:08, Doug Anderson wrote:
> >>> Hi,
> >>>
> >>> On Mon, Oct 31, 2022 at 2:11 PM Kuogee Hsieh  
> >>> wrote:
> >>>>
> >>>> Hi Dmitry,
> >>>>
> >>>>
> >>>> Link rate is advertised by sink, but adjusted (reduced the link rate)
> >>>> by host during link training.
> >>>>
> >>>> Therefore should be fine if host did not support HBR3 rate.
> >>>>
> >>>> It will reduce to lower link rate during link training procedures.
> >>>>
> >>>> kuogee
> >>>>
> >>>> On 10/31/2022 11:46 AM, Dmitry Baryshkov wrote:
> >>>>> On 31/10/2022 20:27, Kuogee Hsieh wrote:
> >>>>>> An HBR3-capable device shall also support TPS4. Since TPS4 feature
> >>>>>> had been implemented already, it is not necessary to limit link
> >>>>>> rate at HBR2 (5.4G). This patch remove this limitation to support
> >>>>>> HBR3 (8.1G) link rate.
> >>>>>
> >>>>> The DP driver supports several platforms including sdm845 and can
> >>>>> support, if I'm not mistaken, platforms up to msm8998/sdm630/660.
> >>>>> Could you please confirm that all these SoCs have support for HBR3?
> >>>>>
> >>>>> With that fact being confirmed:
> >>>>>
> >>>>> Reviewed-by: Dmitry Baryshkov 
> >>>>>
> >>>>>
> >>>>>>
> >>>>>> Signed-off-by: Kuogee Hsieh 
> >>>>>> ---
> >>>>>> drivers/gpu/drm/msm/dp/dp_panel.c | 4 
> >>>>>> 1 file changed, 4 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
> >>>>>> b/drivers/gpu/drm/msm/dp/dp_panel.c
> >>>>>> index 5149ceb..3344f5a 100644
> >>>>>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> >>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> >>>>>> @@ -78,10 +78,6 @@ static int dp_panel_read_dpcd(struct dp_panel
> >>>>>> *dp_panel)
> >>>>>> if (link_info->num_lanes > dp_panel->max_dp_lanes)
> >>>>>> link_info->num_lanes = dp_panel->max_dp_lanes;
> >>>>>> -/* Limit support upto HBR2 until HBR3 support is added */
> >>>>>> -if (link_info->rate >=
> >>>>>> (drm_dp_bw_code_to_link_rate(DP_LINK_BW_5_4)))
> >>>>>> -link_info->rate = drm_dp_bw_code_to_link_rate(DP_LINK_BW_5_4);
> >>>>>> -
> >>>>>> drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", major, minor);
> >>>>>> drm_dbg_dp(panel->drm_dev, "link_rate=%d\n", link_info->rate);
> >>>>>> drm_dbg_dp(panel->drm_dev, "lane_count=%d\n",
> >>>>>> link_info->num_lanes);
> >>>
> >>> Stephen might remember better, but I could have sworn that the problem
> >>> was that there might be something in the middle that couldn't support
> >>> the higher link rate. In other words, I think we have:
> >>>
> >>> SoC <--> TypeC Port Controller <--> Display
> >>>
> >>> The SoC might support HBR3 and the display might support HBR3, but the
> >>> TCPC (Type C Port Controller) might not. I think that the TCPC is a
> >>> silent/passive component so it can't really let anyone know about its
> >>> limitations.
> >>>
> >>> In theory I guess you could rely on link training to just happen to
> >>> fail if you drive the link too fast for the TCPC to handle. Does this
> >>> actually work reliably?
> >>>
> >>> I think the other option that was discussed in the past was to add
> >>> something in the device tree for this. Either you could somehow model
> >>> the TCPC in DRM and thus know that a given model of TCPC limits the
> >>> link rate or you could hack in a property in the DP controller to
> >>> limit it.
> >>
> >>

Re: [Freedreno] [PATCH] drm/msm/dp: remove limitation of link rate at 5.4G to support HBR3

[Doug Anderson]

Hi,

On Tue, Nov 1, 2022 at 7:37 AM Doug Anderson  wrote:
>
> Hi,
>
> On Mon, Oct 31, 2022 at 5:15 PM Dmitry Baryshkov
>  wrote:
> >
> > On 01/11/2022 03:08, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Mon, Oct 31, 2022 at 2:11 PM Kuogee Hsieh  
> > > wrote:
> > >>
> > >> Hi Dmitry,
> > >>
> > >>
> > >> Link rate is advertised by sink, but adjusted (reduced the link rate)
> > >> by host during link training.
> > >>
> > >> Therefore should be fine if host did not support HBR3 rate.
> > >>
> > >> It will reduce to lower link rate during link training procedures.
> > >>
> > >> kuogee
> > >>
> > >> On 10/31/2022 11:46 AM, Dmitry Baryshkov wrote:
> > >>> On 31/10/2022 20:27, Kuogee Hsieh wrote:
> > >>>> An HBR3-capable device shall also support TPS4. Since TPS4 feature
> > >>>> had been implemented already, it is not necessary to limit link
> > >>>> rate at HBR2 (5.4G). This patch remove this limitation to support
> > >>>> HBR3 (8.1G) link rate.
> > >>>
> > >>> The DP driver supports several platforms including sdm845 and can
> > >>> support, if I'm not mistaken, platforms up to msm8998/sdm630/660.
> > >>> Could you please confirm that all these SoCs have support for HBR3?
> > >>>
> > >>> With that fact being confirmed:
> > >>>
> > >>> Reviewed-by: Dmitry Baryshkov 
> > >>>
> > >>>
> > >>>>
> > >>>> Signed-off-by: Kuogee Hsieh 
> > >>>> ---
> > >>>>drivers/gpu/drm/msm/dp/dp_panel.c | 4 
> > >>>>1 file changed, 4 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
> > >>>> b/drivers/gpu/drm/msm/dp/dp_panel.c
> > >>>> index 5149ceb..3344f5a 100644
> > >>>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> > >>>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> > >>>> @@ -78,10 +78,6 @@ static int dp_panel_read_dpcd(struct dp_panel
> > >>>> *dp_panel)
> > >>>>if (link_info->num_lanes > dp_panel->max_dp_lanes)
> > >>>>link_info->num_lanes = dp_panel->max_dp_lanes;
> > >>>>-/* Limit support upto HBR2 until HBR3 support is added */
> > >>>> -if (link_info->rate >=
> > >>>> (drm_dp_bw_code_to_link_rate(DP_LINK_BW_5_4)))
> > >>>> -link_info->rate = drm_dp_bw_code_to_link_rate(DP_LINK_BW_5_4);
> > >>>> -
> > >>>>drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", major, minor);
> > >>>>drm_dbg_dp(panel->drm_dev, "link_rate=%d\n", link_info->rate);
> > >>>>drm_dbg_dp(panel->drm_dev, "lane_count=%d\n",
> > >>>> link_info->num_lanes);
> > >
> > > Stephen might remember better, but I could have sworn that the problem
> > > was that there might be something in the middle that couldn't support
> > > the higher link rate. In other words, I think we have:
> > >
> > > SoC <--> TypeC Port Controller <--> Display
> > >
> > > The SoC might support HBR3 and the display might support HBR3, but the
> > > TCPC (Type C Port Controller) might not. I think that the TCPC is a
> > > silent/passive component so it can't really let anyone know about its
> > > limitations.
> > >
> > > In theory I guess you could rely on link training to just happen to
> > > fail if you drive the link too fast for the TCPC to handle. Does this
> > > actually work reliably?
> > >
> > > I think the other option that was discussed in the past was to add
> > > something in the device tree for this. Either you could somehow model
> > > the TCPC in DRM and thus know that a given model of TCPC limits the
> > > link rate or you could hack in a property in the DP controller to
> > > limit it.
> >
> > Latest pmic_glink proposal from Bjorn include adding the drm_bridge for
> > the TCPC. Such bridge can in theory limit supported modes and rates.
>
> Excellent! Even so, I think this isn't totally a solved problem,
> right? Even though a bridge seems like a good place for this, last I
> remember checking the brid

Re: [Freedreno] [PATCH] drm/msm/dp: remove limitation of link rate at 5.4G to support HBR3

[Doug Anderson]

Hi,

On Mon, Oct 31, 2022 at 5:15 PM Dmitry Baryshkov
 wrote:
>
> On 01/11/2022 03:08, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Oct 31, 2022 at 2:11 PM Kuogee Hsieh  
> > wrote:
> >>
> >> Hi Dmitry,
> >>
> >>
> >> Link rate is advertised by sink, but adjusted (reduced the link rate)
> >> by host during link training.
> >>
> >> Therefore should be fine if host did not support HBR3 rate.
> >>
> >> It will reduce to lower link rate during link training procedures.
> >>
> >> kuogee
> >>
> >> On 10/31/2022 11:46 AM, Dmitry Baryshkov wrote:
> >>> On 31/10/2022 20:27, Kuogee Hsieh wrote:
> >>>> An HBR3-capable device shall also support TPS4. Since TPS4 feature
> >>>> had been implemented already, it is not necessary to limit link
> >>>> rate at HBR2 (5.4G). This patch remove this limitation to support
> >>>> HBR3 (8.1G) link rate.
> >>>
> >>> The DP driver supports several platforms including sdm845 and can
> >>> support, if I'm not mistaken, platforms up to msm8998/sdm630/660.
> >>> Could you please confirm that all these SoCs have support for HBR3?
> >>>
> >>> With that fact being confirmed:
> >>>
> >>> Reviewed-by: Dmitry Baryshkov 
> >>>
> >>>
> >>>>
> >>>> Signed-off-by: Kuogee Hsieh 
> >>>> ---
> >>>>drivers/gpu/drm/msm/dp/dp_panel.c | 4 
> >>>>1 file changed, 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
> >>>> b/drivers/gpu/drm/msm/dp/dp_panel.c
> >>>> index 5149ceb..3344f5a 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> >>>> @@ -78,10 +78,6 @@ static int dp_panel_read_dpcd(struct dp_panel
> >>>> *dp_panel)
> >>>>if (link_info->num_lanes > dp_panel->max_dp_lanes)
> >>>>link_info->num_lanes = dp_panel->max_dp_lanes;
> >>>>-/* Limit support upto HBR2 until HBR3 support is added */
> >>>> -if (link_info->rate >=
> >>>> (drm_dp_bw_code_to_link_rate(DP_LINK_BW_5_4)))
> >>>> -link_info->rate = drm_dp_bw_code_to_link_rate(DP_LINK_BW_5_4);
> >>>> -
> >>>>drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", major, minor);
> >>>>drm_dbg_dp(panel->drm_dev, "link_rate=%d\n", link_info->rate);
> >>>>drm_dbg_dp(panel->drm_dev, "lane_count=%d\n",
> >>>> link_info->num_lanes);
> >
> > Stephen might remember better, but I could have sworn that the problem
> > was that there might be something in the middle that couldn't support
> > the higher link rate. In other words, I think we have:
> >
> > SoC <--> TypeC Port Controller <--> Display
> >
> > The SoC might support HBR3 and the display might support HBR3, but the
> > TCPC (Type C Port Controller) might not. I think that the TCPC is a
> > silent/passive component so it can't really let anyone know about its
> > limitations.
> >
> > In theory I guess you could rely on link training to just happen to
> > fail if you drive the link too fast for the TCPC to handle. Does this
> > actually work reliably?
> >
> > I think the other option that was discussed in the past was to add
> > something in the device tree for this. Either you could somehow model
> > the TCPC in DRM and thus know that a given model of TCPC limits the
> > link rate or you could hack in a property in the DP controller to
> > limit it.
>
> Latest pmic_glink proposal from Bjorn include adding the drm_bridge for
> the TCPC. Such bridge can in theory limit supported modes and rates.

Excellent! Even so, I think this isn't totally a solved problem,
right? Even though a bridge seems like a good place for this, last I
remember checking the bridge API wasn't expressive enough to solve
this problem. A bridge could limit pixel clocks just fine, but here we
need to take into account other considerations to know if a given
pixel clock can work at 5.4 GHz or not. For instance, if we're at 4
lanes we could maybe make a given pixel clock at 5.4 GHz but not if we
only have 2 lanes. I don't think that the DP controller passes the
number of lanes to other parts of the bridge chain, though maybe
there's some trick for it?

...I guess the other problem is that all existing users aren't
currently modeling their TCPC in this way. What happens to them?

-Doug

Re: [Freedreno] [PATCH] drm/msm/dp: remove limitation of link rate at 5.4G to support HBR3

[Doug Anderson]

Hi,

On Mon, Oct 31, 2022 at 2:11 PM Kuogee Hsieh  wrote:
>
> Hi Dmitry,
>
>
> Link rate is advertised by sink, but adjusted (reduced the link rate)
> by host during link training.
>
> Therefore should be fine if host did not support HBR3 rate.
>
> It will reduce to lower link rate during link training procedures.
>
> kuogee
>
> On 10/31/2022 11:46 AM, Dmitry Baryshkov wrote:
> > On 31/10/2022 20:27, Kuogee Hsieh wrote:
> >> An HBR3-capable device shall also support TPS4. Since TPS4 feature
> >> had been implemented already, it is not necessary to limit link
> >> rate at HBR2 (5.4G). This patch remove this limitation to support
> >> HBR3 (8.1G) link rate.
> >
> > The DP driver supports several platforms including sdm845 and can
> > support, if I'm not mistaken, platforms up to msm8998/sdm630/660.
> > Could you please confirm that all these SoCs have support for HBR3?
> >
> > With that fact being confirmed:
> >
> > Reviewed-by: Dmitry Baryshkov 
> >
> >
> >>
> >> Signed-off-by: Kuogee Hsieh 
> >> ---
> >>   drivers/gpu/drm/msm/dp/dp_panel.c | 4 
> >>   1 file changed, 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
> >> b/drivers/gpu/drm/msm/dp/dp_panel.c
> >> index 5149ceb..3344f5a 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> >> @@ -78,10 +78,6 @@ static int dp_panel_read_dpcd(struct dp_panel
> >> *dp_panel)
> >>   if (link_info->num_lanes > dp_panel->max_dp_lanes)
> >>   link_info->num_lanes = dp_panel->max_dp_lanes;
> >>   -/* Limit support upto HBR2 until HBR3 support is added */
> >> -if (link_info->rate >=
> >> (drm_dp_bw_code_to_link_rate(DP_LINK_BW_5_4)))
> >> -link_info->rate = drm_dp_bw_code_to_link_rate(DP_LINK_BW_5_4);
> >> -
> >>   drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", major, minor);
> >>   drm_dbg_dp(panel->drm_dev, "link_rate=%d\n", link_info->rate);
> >>   drm_dbg_dp(panel->drm_dev, "lane_count=%d\n",
> >> link_info->num_lanes);

Stephen might remember better, but I could have sworn that the problem
was that there might be something in the middle that couldn't support
the higher link rate. In other words, I think we have:

SoC <--> TypeC Port Controller <--> Display

The SoC might support HBR3 and the display might support HBR3, but the
TCPC (Type C Port Controller) might not. I think that the TCPC is a
silent/passive component so it can't really let anyone know about its
limitations.

In theory I guess you could rely on link training to just happen to
fail if you drive the link too fast for the TCPC to handle. Does this
actually work reliably?

I think the other option that was discussed in the past was to add
something in the device tree for this. Either you could somehow model
the TCPC in DRM and thus know that a given model of TCPC limits the
link rate or you could hack in a property in the DP controller to
limit it.

-Doug

Re: [Freedreno] [v5] drm/msm/disp/dpu1: add support for dspp sub block flush in sc7280

[Doug Anderson]

Hi,

On Wed, Sep 14, 2022 at 5:16 AM Kalyan Thota  wrote:
>
> Flush mechanism for DSPP blocks has changed in sc7280 family, it
> allows individual sub blocks to be flushed in coordination with
> master flush control.
>
> Representation: master_flush && (PCC_flush | IGC_flush .. etc )
>
> This change adds necessary support for the above design.
>
> Changes in v1:
> - Few nits (Doug, Dmitry)
> - Restrict sub-block flush programming to dpu_hw_ctl file (Dmitry)
>
> Changes in v2:
> - Move the address offset to flush macro (Dmitry)
> - Seperate ops for the sub block flush (Dmitry)
>
> Changes in v3:
> - Reuse the DPU_DSPP_xx enum instead of a new one (Dmitry)
>
> Changes in v4:
> - Use shorter version for unsigned int (Stephen)
>
> Reviewed-by: Dmitry Baryshkov 
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c   |  2 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c |  5 +++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |  4 +++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 35 
> --
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 10 ++--
>  5 files changed, 50 insertions(+), 6 deletions(-)

Breadcrumbs: though this is tagged in the subject as v5 I think the
newest version is actually "resend v4" [1] which just fixes the
Signed-off-by.

[1] 
https://lore.kernel.org/r/1663825463-6715-1-git-send-email-quic_kaly...@quicinc.com

Re: [Freedreno] [PATCH v2 06/10] drm/msm/dp: fix aux-bus EP lifetime

[Doug Anderson]

Hi,

On Tue, Sep 13, 2022 at 9:58 AM Johan Hovold  wrote:
>
> Device-managed resources allocated post component bind must be tied to
> the lifetime of the aggregate DRM device or they will not necessarily be
> released when binding of the aggregate device is deferred.
>
> This can lead resource leaks or failure to bind the aggregate device
> when binding is later retried and a second attempt to allocate the
> resources is made.
>
> For the DP aux-bus, an attempt to populate the bus a second time will
> simply fail ("DP AUX EP device already populated").
>
> Fix this by tying the lifetime of the EP device to the DRM device rather
> than DP controller platform device.
>
> Fixes: c3bf8e21b38a ("drm/msm/dp: Add eDP support via aux_bus")
> Cc: sta...@vger.kernel.org  # 5.19
> Signed-off-by: Johan Hovold 
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 13 -
>  1 file changed, 12 insertions(+), 1 deletion(-)

This seems fine to me as a short term fix until we get the DP AUX
populating moved to probe.

Reviewed-by: Douglas Anderson 

Re: [Freedreno] [PATCH 4/7] drm/msm/dp: fix aux-bus EP lifetime

[Doug Anderson]

Hi,

On Mon, Sep 12, 2022 at 7:10 PM Dmitry Baryshkov
 wrote:
>
> On 12/09/2022 18:40, Johan Hovold wrote:
> > Device-managed resources allocated post component bind must be tied to
> > the lifetime of the aggregate DRM device or they will not necessarily be
> > released when binding of the aggregate device is deferred.
> >
> > This can lead resource leaks or failure to bind the aggregate device
> > when binding is later retried and a second attempt to allocate the
> > resources is made.
> >
> > For the DP aux-bus, an attempt to populate the bus a second time will
> > simply fail ("DP AUX EP device already populated").
> >
> > Fix this by amending the DP aux interface and tying the lifetime of the
> > EP device to the DRM device rather than DP controller platform device.
>
> Doug, could you please take a look?
>
> For me this is another reminder/pressure point that we should populate
> the AUX BUS from the probe(), before binding the components together.

Aside from the kernel robot complaints, I'm not necessarily convinced.
I think we know that the AUX DP stuff in MSM-DP is fragile right now
and Qualcomm has promised to clean it up. This really feels like a
band-aid and is really a sign that we're populating the AUX DP bus in
the wrong place in Qualcomm's code. As you said, if we moved this to
probe(), which is the plan in the promised cleanup, then it wouldn't
be a problem.

As far as I know Qualcomm has queued this cleanup behind their current
PSR work (though it's never been clear why both can't be worked on at
the same time) and the PSR work was stalled because they couldn't
figure out what caused the glitching I reported. It's still on my nag
list that I bring up with them every week...

In any case, if a band-aid is urgent, maybe you could just call
of_dp_aux_populate_bus() directly in Qualcomm code and you could add
your own devm_add_action_or_reset() on the DRM device.

Re: [Freedreno] [PATCH v3] drm/msm/dp: correct 1.62G link rate at dp_catalog_ctrl_config_msa()

[Doug Anderson]

Hi,

On Wed, Aug 24, 2022 at 1:16 PM Kuogee Hsieh  wrote:
>
> At current implementation there is an extra 0 at 1.62G link rate which cause
> no correct pixel_div selected for 1.62G link rate to calculate mvid and nvid.
> This patch delete the extra 0 to have mvid and nvid be calculated correctly.
>
> Changes in v2:
> -- fix Fixes tag's text
>
> Changes in v3:
> -- fix misspelling of "Reviewed-by"
>
> Fixes: 937f941ca06f  ("drm/msm/dp: Use qmp phy for DP PLL and PHY")
> Signed-off-by: Kuogee Hsieh 
>
> Reviewed-by: Stephen Boyd 
> Reviewed-by: Abhinav Kumar 
> ---
>  drivers/gpu/drm/msm/dp/dp_catalog.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Douglas Anderson 

Re: [Freedreno] [PATCH v2 4/7] regulator: core: Allow specifying an initial load w/ the bulk API

[Doug Anderson]

Hi,

On Mon, Aug 22, 2022 at 11:23 PM Yongqin Liu  wrote:
>
> Hi, Douglas
>
> Just an update on the fix you pointed out previously here:
> > > [1] 
> > > https://lore.kernel.org/r/20220809142738.1.I91625242f137c707bb345c51c80c5ecee02eeff3@changeid
>
> With it I could boot the hikey960 build to the home screen if it does
> not use the GKI kernel.
> but the problem will be reproduced if it uses the GKI kernel.
>
> And if this change is reverted, then it could boot with the GKI kernel as 
> well.
>
> I am not sure what's the reason there, but there seems to be some
> difference with the fix above and the workaround of revert.
> Not sure if you have any idea about that.
>
> Regarding the GKI kernel(Android Generic Kernel Image)[2], it's built
> from the android-mainline tree(f51334eac4de) without any workaround.
> (Neither the revert, nor the fix applied), and the regulator modules
> used for the hikey960 build are hi6421v530-regulator.ko and
> hi655x-regulator.ko
>
> I am still not sure if it would work with the GKI kernel that has the
> fix that you pointed out in. the case that both the GKI kernel and
> vendor tree have the fix.
> Will update here when I have some results.
>
> [2]: 
> https://source.android.com/docs/core/architecture/kernel/generic-kernel-image?hl=en

That's not too surprising. The broken patch is in the core kernel so
you need the fix in the core kernel. I think that means you'll have to
wait until `android-mainline` gets the fix. I don't work on Android,
so if there's some other route to get an expedited fix into
android-mainline I'm not aware of it.

-Doug

-Doug

Re: [Freedreno] [RFC PATCH] drm/edid: Make 144 Hz not preferred on Sharp LQ140M1JW46

[Doug Anderson]

Hi,

On Sun, Aug 14, 2022 at 11:46 PM Maxime Ripard  wrote:
>
> On Fri, Jul 29, 2022 at 12:57:40PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Jul 29, 2022 at 9:41 AM Maxime Ripard  wrote:
> > >
> > > On Fri, Jul 29, 2022 at 07:50:20AM -0700, Doug Anderson wrote:
> > > > On Fri, Jul 29, 2022 at 12:51 AM Maxime Ripard  
> > > > wrote:
> > > > >
> > > > > On Thu, Jul 28, 2022 at 02:18:38PM -0700, Doug Anderson wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Thu, Jul 28, 2022 at 10:34 AM Abhinav Kumar
> > > > > >  wrote:
> > > > > > >
> > > > > > > Hi Rob and Doug
> > > > > > >
> > > > > > > On 7/22/2022 10:36 AM, Rob Clark wrote:
> > > > > > > > On Fri, Jul 22, 2022 at 9:48 AM Doug Anderson 
> > > > > > > >  wrote:
> > > > > > > >>
> > > > > > > >> Hi,
> > > > > > > >>
> > > > > > > >> On Fri, Jul 22, 2022 at 9:37 AM Abhinav Kumar 
> > > > > > > >>  wrote:
> > > > > > > >>>
> > > > > > > >>> + sankeerth
> > > > > > > >>>
> > > > > > > >>> Hi Doug
> > > > > > > >>>
> > > > > > > >>> On 7/21/2022 3:23 PM, Douglas Anderson wrote:
> > > > > > > >>>> The Sharp LQ140M1JW46 panel is on the Qualcomm sc7280 CRD 
> > > > > > > >>>> reference
> > > > > > > >>>> board. This panel supports 144 Hz and 60 Hz. In the EDID, 
> > > > > > > >>>> the 144 Hz
> > > > > > > >>>> mode is listed first and thus is marked preferred. The EDID 
> > > > > > > >>>> decode I
> > > > > > > >>>> ran says:
> > > > > > > >>>>
> > > > > > > >>>> First detailed timing includes the native pixel format 
> > > > > > > >>>> and preferred
> > > > > > > >>>> refresh rate.
> > > > > > > >>>>
> > > > > > > >>>> ...
> > > > > > > >>>>
> > > > > > > >>>> Detailed Timing Descriptors:
> > > > > > > >>>>   DTD 1:  1920x1080  143.981 Hz  16:9   166.587 kHz  
> > > > > > > >>>> 346.500 MHz
> > > > > > > >>>>Hfront   48 Hsync  32 Hback  80 Hpol N
> > > > > > > >>>>Vfront3 Vsync   5 Vback  69 Vpol N
> > > > > > > >>>>   DTD 2:  1920x1080   59.990 Hz  16:969.409 kHz  
> > > > > > > >>>> 144.370 MHz
> > > > > > > >>>>Hfront   48 Hsync  32 Hback  80 Hpol N
> > > > > > > >>>>Vfront3 Vsync   5 Vback  69 Vpol N
> > > > > > > >>>>
> > > > > > > >>>> I'm proposing here that the above is actually a bug and that 
> > > > > > > >>>> the 60 Hz
> > > > > > > >>>> mode really should be considered preferred by Linux.
> > > > > > >
> > > > > > > Its a bit tricky to say that this is a bug but I think we can 
> > > > > > > certainly
> > > > > > > add here that for an internal display we would have ideally had 
> > > > > > > the
> > > > > > > lower resolution first to indicate it as default.
> > > > > >
> > > > > > Yeah, it gets into the vagueness of the EDID spec in general. As far
> > > > > > as I can find it's really up to the monitor to decide by what means 
> > > > > > it
> > > > > > chooses the "preferred" refresh rate if the monitor can support 
> > > > > > many.
> > > > > > Some displays may decide that the normal rate is "preferred" and 
> > > > > > some
> > > > > > may decide that the high refresh rate is "preferred". Neither 
> > > > > >

Re: [Freedreno] [PATCH] drm/msm/dsi: Set panel orientation when directly connected

[Doug Anderson]

Hi,

On Wed, Jul 20, 2022 at 3:42 PM Doug Anderson  wrote:
>
> Hi,
>
> On Wed, Jul 20, 2022 at 1:46 PM Rob Clark  wrote:
> >
> > On Fri, Jul 8, 2022 at 8:25 AM Doug Anderson  wrote:
> > >
> > > Hi,
> > >
> > > On Wed, Jul 6, 2022 at 12:14 PM Stephen Boyd  wrote:
> > > >
> > > > Set the panel orientation in drm when the panel is directly connected,
> > > > i.e. we're not using an external bridge. The external bridge case is
> > > > already handled by the panel bridge code, so we only update the path we
> > > > take when the panel is directly connected/internal. This silences a
> > > > warning splat coming from __drm_mode_object_add() on Wormdingler boards.
> > > >
> > > > Cc: Hsin-Yi Wang 
> > > > Cc: Douglas Anderson 
> > > > Signed-off-by: Stephen Boyd 
> > > > ---
> > > >
> > > > This relies on commit 5e41b01a7808 ("drm/panel: Add an API to allow drm
> > > > to set orientation from panel") which is in drm-misc
> > > >
> > > >  drivers/gpu/drm/msm/dsi/dsi_manager.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > >
> > > I don't personally have objections to this, but (to my understanding)
> > > "the future" is that everyone should use panel_bridge. If we made the
> > > move to panel_bridge today then we wouldn't need to do this. In
> > > general I think panel_bridge would end up letting us delete a bunch of
> > > code...
> > >
> > > See commit 4e5763f03e10 ("drm/bridge: ti-sn65dsi86: Wrap panel with
> > > panel-bridge") for when this was done by ti-sn65dsi86.
> > >
> > > Then again, I spent a small amount of time looking into this and it's
> > > definitely non-trivial. Still likely worthwhile, but not worth
> > > blocking a tiny fix like this. It also should be fairly obvious that
> > > we should delete this when we switch to panel_bridge.
> > >
> > > Thus:
> > >
> > > Reviewed-by: Douglas Anderson 
> > >
> > > I'll assume that we'll just snooze this commit until drm-misc-next
> > > merges into a tree that msm-next is based on, which will probably be
> > > the next -rc1. If desired and Acked I could land this in
> > > drm-misc-next, but it's probably not worth it?
> >
> > if you want to land this patch via drm-misc, which might be the
> > easier/faster route, then:
> >
> > Acked-by: Rob Clark 
>
> As per discussion on IRC, I'm not going to apply this to drm-misc-next.
>
> Given where we are in the cycle landing in drm-misc-next means it
> won't be in mainline for a couple versions and I suspect that'll cause
> merge conflicts with Dmitry's series [1]. ...and, of course, if
> Dmitry's series lands then we don't even need ${SUBJECT} patch...
>
> So I think the plan is:
>
> 1. Snooze waiting for the next -rc1 since
> drm_connector_set_orientation_from_panel() won't be in mainline until
> then.
>
> 2. If Dmitry's series looks like a long way off, we could land
> ${SUBJECT} patch in msm-next as a stopgap fix.
>
>
> [1] 
> https://lore.kernel.org/r/20220711094320.368062-5-dmitry.barysh...@linaro.org/

Just checking up. What's the latest thinking here? Do we want to land
Stephen's change as a stopgap?
drm_connector_set_orientation_from_panel() is available in v6.0-rc1.

-Doug

Re: [Freedreno] [PATCH v2 4/7] regulator: core: Allow specifying an initial load w/ the bulk API

[Doug Anderson]

Hi,

On Tue, Aug 16, 2022 at 5:58 AM Yongqin Liu  wrote:
>
> HI, Douglas
>
> With this change, I get one kernel panic with my hikey960
> android-mainline based Android build,
> if it's reverted, then the build could boot to the home screen successfully.
> From the log information I shared here, not sure if you have any idea
> what I could do to have the hikey960
> build work with this change,
>
> The kernel panic is something like this, for details, please check the
> log here: http://ix.io/47Lq
>
> [   10.738042][  T193] adv7511 1-0039: error : Failed
> to get supply 'v1p2'
> [   10.748457][  T194] apexd: Found pre-installed APEX
> /system/apex/com.android.os.statsd.apex
> [   10.752908][   T67] Unable to handle kernel read from unreadable
> memory at virtual address 004c
> [   10.753116][T8] Unable to handle kernel read from unreadable
> memory at virtual address 0078
> [   10.753130][T8] Mem abort info:
> [   10.753135][T8]   ESR = 0x9605
> [   10.753142][T8]   EC = 0x25: DABT (current EL), IL = 32 bits
> [   10.753152][T8]   SET = 0, FnV = 0
> [   10.753159][T8]   EA = 0, S1PTW = 0
> [   10.753166][T8]   FSC = 0x05: level 1 translation fault
> [   10.753174][T8] Data abort info:
> [   10.753179][T8]   ISV = 0, ISS = 0x0005
> [   10.753184][T8]   CM = 0, WnR = 0
> [   10.753192][T8] user pgtable: 4k pages, 39-bit VAs, 
> pgdp=03098000
> [   10.753204][T8] [0078] pgd=,
> p4d=, pud=
> [   10.753232][T8] Internal error: Oops: 9605 [#1] PREEMPT SMP
> [   10.753245][T8] Modules linked in: adv7511(E+) tcpci_rt1711h(E)
> hci_uart(E) btqca(E) btbcm(E) cpufreq_dt(E) hi3660_i2s(E)
> hisi_hikey_usb(E) hi6421_pmic_core(E) syscon_reboot_mode(E)
> reboot_mode(E) hi3660_mailbox(E) dw_mmc_k3(E) hisi_thermal(E)
> dw_mmc_pltfm(E) dw_mmc(E) kirin_drm(E) snd_soc_simple_card(E)
> snd_soc_simple_card_utils(E) spi_pl022(E) kirin_dsi(E)
> phy_hi3660_usb3(E) i2c_designware_platform(E) drm_cma_helper(E)
> i2c_designware_core(E) mali_kbase(OE) k3dma(E) cma_heap(E)
> system_heap(E)
> [   10.753436][T8] CPU: 2 PID: 8 Comm: kworker/u16:0 Tainted: G
>OE  5.19.0-mainline-03487-g86d047950300-dirty #1
> [   10.753454][T8] Hardware name: HiKey960 (DT)
> [   10.753463][T8] Workqueue: events_unbound async_run_entry_fn
> [   10.753497][T8] pstate: 6045 (nZCv daif +PAN -UAO -TCO -DIT
> -SSBS BTYPE=--)
> [   10.753516][T8] pc : regulator_bulk_enable_async+0x3c/0x98
> [   10.753540][T8] lr : async_run_entry_fn+0x30/0xf8
> [   10.753559][T8] sp : ffc009ed3d20
> [   10.753567][T8] x29: ffc009ed3d40 x28: 0402
> x27: ff801ad99828
> [   10.753592][T8] x26: ff803217b010 x25: 0002
> x24: ff8003385da8
> [   10.753617][T8] x23: ff8003385da0 x22: ff801ad94805
> x21: ff8003385da0
> [   10.753642][T8] x20:  x19: ff8003143d20
> x18: ffc008075028
> [   10.753667][T8] x17: 00040044 x16: 0001
> x15: 0010
> [   10.753691][T8] x14:  x13: 0f58
> x12: 8235
> [   10.753715][T8] x11: 6acfbfa2f400 x10: 0016 x9
> : 00ff
> [   10.753740][T8] x8 : da9ecda1b63b0500 x7 : 8080 x6
> : 
> [   10.753764][T8] x5 : 0001 x4 : 646e756f626e x3
> : ff801ad99828
> [   10.753788][T8] x2 : ff8003385da8 x1 : ffc009ed3d20 x0
> : ff8003143d20
> [   10.753812][T8] Call trace:
> [   10.753818][T8]  regulator_bulk_enable_async+0x3c/0x98
> [   10.753839][T8]  async_run_entry_fn+0x30/0xf8
> [   10.753859][T8]  process_one_work+0x1d0/0x404
> [   10.753879][T8]  worker_thread+0x25c/0x43c
> [   10.753897][T8]  kthread+0xf0/0x2c0
> [   10.753912][T8]  ret_from_fork+0x10/0x20
> [   10.753940][T8] Code: f81f83a8 f9400814 a900 f90003ff (f9403e95)
> [   10.753950][T8] ---[ end trace  ]---
> [   10.760621][  T194] apexd: Found pre-installed APEX
> /system/apex/com.android.permission.capex
> [   10.767672][   T67] Mem abort info:
> [   10.779658][  T194] apexd: Found pre-installed APEX
> /system/apex/com.android.art.capex
> [   10.783690][   T67]   ESR = 0x9605
> [   10.792424][  T194] apexd: Found pre-installed APEX
> /system/apex/com.android.scheduling.capex
> [   10.794713][T8] Kernel panic - not syncing: Oops: Fatal exception
> [   10.794723][T8] SMP: stopping secondary CPUs
> [   10.798141][T8] Kernel Offset: 0x7 from 0xffc00800
> [   10.798150][T8] PHYS_OFFSET: 0x0
> [   10.798156][T8] CPU features: 0x,00649020,1086
> [   10.798167][T8] Memory Limit: none

Are you fixed by the patch ("regulator: core: Fix missing error return
from regulator_bulk_get()") [1]

[1] 

Re: [Freedreno] [v1] drm/msm/disp/dpu1: add support for hierarchical flush for dspp in sc7280

[Doug Anderson]

Hi,

On Mon, Aug 8, 2022 at 3:44 AM Kalyan Thota  wrote:
>
> >I'd like to land at least patches 6-8 from [1] next cycle. They clean up the 
> >CTL
> >interface. Could you please rebase your patch on top of them?
> >
>
> Sure I'll wait for the series to rebase. @Doug can you comment if this is 
> okay and this patch is not needed immediately ?
>
> >[1] https://patchwork.freedesktop.org/series/99909/

I don't personally see a problem basing them atop a cleanup. If the
patches Dmitry points at are targeted for the next cycle then that
seems like a pretty reasonable timeframe to me.

-Doug

Re: [Freedreno] [PATCH v6 00/10] Add PSR support for eDP

[Doug Anderson]

Hi,

On Thu, Aug 4, 2022 at 9:21 AM Robert Foss  wrote:
>
> On Fri, 29 Jul 2022 at 02:22, Doug Anderson  wrote:
> >
> > Hi,
> >
> > On Mon, Jul 11, 2022 at 5:57 AM Vinod Polimera
> >  wrote:
> > >
> > > Changes in v2:
> > >   - Use dp bridge to set psr entry/exit instead of dpu_enocder.
> > >   - Don't modify whitespaces.
> > >   - Set self refresh aware from atomic_check.
> > >   - Set self refresh aware only if psr is supported.
> > >   - Provide a stub for msm_dp_display_set_psr.
> > >   - Move dp functions to bridge code.
> > >
> > > Changes in v3:
> > >   - Change callback names to reflect atomic interfaces.
> > >   - Move bridge callback change to separate patch as suggested by Dmitry.
> > >   - Remove psr function declaration from msm_drv.h.
> > >   - Set self_refresh_aware flag only if psr is supported.
> > >   - Modify the variable names to simpler form.
> > >   - Define bit fields for PSR settings.
> > >   - Add comments explaining the steps to enter/exit psr.
> > >   - Change DRM_INFO to drm_dbg_db.
> > >
> > > Changes in v4:
> > >   - Move the get crtc functions to drm_atomic.
> > >   - Add atomic functions for DP bridge too.
> > >   - Add ternary operator to choose eDP or DP ops.
> > >   - Return true/false instead of 1/0.
> > >   - mode_valid missing in the eDP bridge ops.
> > >   - Move the functions to get crtc into drm_atomic.c.
> > >   - Fix compilation issues.
> > >   - Remove dpu_assign_crtc and get crtc from drm_enc instead of dpu_enc.
> > >   - Check for crtc state enable while reserving resources.
> > >
> > > Changes in v5:
> > >   - Move the mode_valid changes into a different patch.
> > >   - Complete psr_op_comp only when isr is set.
> > >   - Move the DP atomic callback changes to a different patch.
> > >   - Get crtc from drm connector state crtc.
> > >   - Move to separate patch for check for crtc state enable while
> > > reserving resources.
> > >
> > > Changes in v6:
> > >   - Remove crtc from dpu_encoder_virt struct.
> > >   - fix crtc check during vblank toggle crtc.
> > >   - Misc changes.
> > >
> > > Signed-off-by: Sankeerth Billakanti 
> > > Signed-off-by: Kalyan Thota 
> > > Signed-off-by: Vinod Polimera 
> > >
> > > Vinod Polimera (10):
> > >   drm/msm/disp/dpu: clear dpu_assign_crtc and get crtc from connector
> > > state instead of dpu_enc
> > >   drm: add helper functions to retrieve old and new crtc
> > >   drm/msm/dp: use atomic callbacks for DP bridge ops
> > >   drm/msm/dp: Add basic PSR support for eDP
> > >   drm/msm/dp: use the eDP bridge ops to validate eDP modes
> > >   drm/bridge: use atomic enable/disable callbacks for panel bridge
> > >   drm/bridge: add psr support for panel bridge callbacks
> > >   drm/msm/disp/dpu: use atomic enable/disable callbacks for encoder
> > > functions
> > >   drm/msm/disp/dpu: add PSR support for eDP interface in dpu driver
> > >   drm/msm/disp/dpu: check for crtc enable rather than crtc active to
> > > release shared resources
> > >
> > >  drivers/gpu/drm/bridge/panel.c  |  68 --
> > >  drivers/gpu/drm/drm_atomic.c|  60 +
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c|  17 ++-
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  56 +
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |   8 --
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c |   2 +-
> > >  drivers/gpu/drm/msm/dp/dp_catalog.c |  81 
> > >  drivers/gpu/drm/msm/dp/dp_catalog.h |   4 +
> > >  drivers/gpu/drm/msm/dp/dp_ctrl.c|  73 +++
> > >  drivers/gpu/drm/msm/dp/dp_ctrl.h|   3 +
> > >  drivers/gpu/drm/msm/dp/dp_display.c |  31 +++--
> > >  drivers/gpu/drm/msm/dp/dp_display.h |   2 +
> > >  drivers/gpu/drm/msm/dp/dp_drm.c | 184 
> > > ++--
> > >  drivers/gpu/drm/msm/dp/dp_drm.h |   9 +-
> > >  drivers/gpu/drm/msm/dp/dp_link.c|  36 ++
> > >  drivers/gpu/drm/msm/dp/dp_panel.c   |  22 
> > >  drivers/gpu/drm/msm/dp/dp_panel.h   |   6 +
> > >  drivers/gpu/drm/msm/dp/dp_reg.h |  27 
> > >  include/drm/drm_atomic.h|   7 ++
> > >  19 files changed, 631 insertions(+), 65 deletions(-)
> >
>
> Which tree does this series apply to?

It ought to apply to msm-next, but as I mentioned in my reply to patch
#1 it doesn't apply to the top of msm-next. I think I also had to
manually apply a few of the later patches with "patch -p1".

-Doug

Re: [Freedreno] [v1] drm/msm/disp/dpu1: add support for hierarchical flush for dspp in sc7280

[Doug Anderson]

On Thu, Aug 4, 2022 at 3:29 AM Kalyan Thota  wrote:
>
> +static void dpu_hw_ctl_set_dspp_hierarchical_flush(struct dpu_hw_ctl *ctx,
> +   enum dpu_dspp dspp, enum dpu_dspp_sub_blk dspp_sub_blk)
> +{
> +   uint32_t flushbits = 0, active = 0;

nit: don't init to 0 since you just override below.


> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h 
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> index ac15444..8ecab91 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> @@ -160,6 +160,9 @@ struct dpu_hw_ctl_ops {
> uint32_t (*get_bitmask_dspp)(struct dpu_hw_ctl *ctx,
> enum dpu_dspp blk);
>
> +   void (*set_dspp_hierarchical_flush)(struct dpu_hw_ctl *ctx,
> +   enum dpu_dspp blk, enum dpu_dspp_sub_blk dspp_sub_blk);
> +

Given that most of the items in this list have kernel-doc comments
explaining them, probably you should have one for your new one too.

-Doug

Re: [Freedreno] [PATCH v3 5/6] drm/msm/dsi: Take advantage of devm_regulator_bulk_get_const()

[Doug Anderson]

Hi,

On Wed, Aug 3, 2022 at 12:19 AM Dmitry Baryshkov
 wrote:
>
> On 03/08/2022 01:37, Douglas Anderson wrote:
> > As of the commit 1de452a0edda ("regulator: core: Allow drivers to
> > define their init data as const") we no longer need to do copying of
> > regulator bulk data from initdata to something dynamic. Let's take
> > advantage of that.
> >
> > In addition to saving some code, this also moves us to using
> > ARRAY_SIZE() to specify how many regulators we have which is less
> > error prone.
> >
> > This gets rid of some layers of wrappers which makes it obvious that
> > we can get rid of an extra error print.
> > devm_regulator_bulk_get_const() prints errors for you so you don't
> > need an extra layer of printing.
> >
> > In all cases here I have preserved the old settings without any
> > investigation about whether the loads being set are sensible. In the
> > cases of some of the PHYs if several PHYs in the same file used
> > exactly the same settings I had them point to the same data structure.
> >
> > NOTE: Though I haven't done the math, this is likely an overall
> > savings in terms of "static const" data. We previously always
> > allocated space for 8 supplies. Each of these supplies took up 36
> > bytes of data (32 for name, 4 for an int).
> >
> > Signed-off-by: Douglas Anderson 
>
> Ah, so to array conversion is already done. That's great.
>
> Reviewed-by: Dmitry Baryshkov 
>
>
> > ---
> >
> > Changes in v3:
> > - Do all the PHYs too.
>
> It would have been easier if DSI and DSI PHY were split to separate patches.

One of the earlier patches in the series (where we remove the
"disable" load) was harder to split since the DSI and DSI PHY code was
sharing a single data structure. Once I had one patch touching both at
the same time I figured I'd keep them all like that. If you need me to
rework them to be separate patches to make it easier to land then
please yell. Otherwise I'll assume it's OK.

-Doug

Re: [Freedreno] [PATCH v3 6/6] drm/msm/dsi: Improve dsi_phy_driver_probe() probe error handling

[Doug Anderson]

Hi,

On Wed, Aug 3, 2022 at 12:32 AM Dmitry Baryshkov
 wrote:
>
> > @@ -634,88 +631,71 @@ static int dsi_phy_driver_probe(struct 
> > platform_device *pdev)
> >   phy->cphy_mode = (phy_type == PHY_TYPE_CPHY);
> >
> >   phy->base = msm_ioremap_size(pdev, "dsi_phy", >base_size);
> > - if (IS_ERR(phy->base)) {
> > - DRM_DEV_ERROR(dev, "%s: failed to map phy base\n", __func__);
> > - ret = -ENOMEM;
>
> Here (and in a few cases later) this changes the error code from crafted
> -ENOMEM to the proper one returned by msm_ioremap_size(). This should be
> mentioned in the commit message.

Good point. Unless something comes up I'll plan to spin a v4 with this
change to the commit message tomorrow.

-Doug

Re: [Freedreno] [PATCH v3 4/6] drm/msm/dsi: Use the new regulator bulk feature to specify the load

[Doug Anderson]

Hi,

On Wed, Aug 3, 2022 at 12:12 AM Dmitry Baryshkov
 wrote:
>
> On 03/08/2022 01:37, Douglas Anderson wrote:
> > As of commit 6eabfc018e8d ("regulator: core: Allow specifying an
> > initial load w/ the bulk API") we can now specify the initial load in
> > the bulk data rather than having to manually call regulator_set_load()
> > on each regulator. Let's use it.
> >
> > Signed-off-by: Douglas Anderson 
>
> Reviewed-by: Dmitry Baryshkov 
>
> It might have been better, if the previous patch had only removed the
> load_setting on disable and inlined the dsi_host_regulator_disable().
> Then this patch would drop the regulator_set_load() from
> dsi_host_regulator_enable() path and inline it. Then it would have been
> more obvious that after these two changes the time when we set loads is
> not changed.

Seems like I should post a v4 to update the commit message of the
final patch in the series, but I'm going to leave this the way it is
since the end result is the same. Originally when I wrote the series I
didn't know if the new regulator API changes would be accepted, so the
previous patch did the most cleanup it could do with the old API. ;-)

-Doug

Re: [Freedreno] [RFC PATCH] drm/edid: Make 144 Hz not preferred on Sharp LQ140M1JW46

[Doug Anderson]

Hi,

On Fri, Jul 29, 2022 at 9:41 AM Maxime Ripard  wrote:
>
> On Fri, Jul 29, 2022 at 07:50:20AM -0700, Doug Anderson wrote:
> > On Fri, Jul 29, 2022 at 12:51 AM Maxime Ripard  wrote:
> > >
> > > On Thu, Jul 28, 2022 at 02:18:38PM -0700, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Thu, Jul 28, 2022 at 10:34 AM Abhinav Kumar
> > > >  wrote:
> > > > >
> > > > > Hi Rob and Doug
> > > > >
> > > > > On 7/22/2022 10:36 AM, Rob Clark wrote:
> > > > > > On Fri, Jul 22, 2022 at 9:48 AM Doug Anderson 
> > > > > >  wrote:
> > > > > >>
> > > > > >> Hi,
> > > > > >>
> > > > > >> On Fri, Jul 22, 2022 at 9:37 AM Abhinav Kumar 
> > > > > >>  wrote:
> > > > > >>>
> > > > > >>> + sankeerth
> > > > > >>>
> > > > > >>> Hi Doug
> > > > > >>>
> > > > > >>> On 7/21/2022 3:23 PM, Douglas Anderson wrote:
> > > > > >>>> The Sharp LQ140M1JW46 panel is on the Qualcomm sc7280 CRD 
> > > > > >>>> reference
> > > > > >>>> board. This panel supports 144 Hz and 60 Hz. In the EDID, the 
> > > > > >>>> 144 Hz
> > > > > >>>> mode is listed first and thus is marked preferred. The EDID 
> > > > > >>>> decode I
> > > > > >>>> ran says:
> > > > > >>>>
> > > > > >>>> First detailed timing includes the native pixel format and 
> > > > > >>>> preferred
> > > > > >>>> refresh rate.
> > > > > >>>>
> > > > > >>>> ...
> > > > > >>>>
> > > > > >>>> Detailed Timing Descriptors:
> > > > > >>>>   DTD 1:  1920x1080  143.981 Hz  16:9   166.587 kHz  346.500 
> > > > > >>>> MHz
> > > > > >>>>Hfront   48 Hsync  32 Hback  80 Hpol N
> > > > > >>>>Vfront3 Vsync   5 Vback  69 Vpol N
> > > > > >>>>   DTD 2:  1920x1080   59.990 Hz  16:969.409 kHz  144.370 
> > > > > >>>> MHz
> > > > > >>>>Hfront   48 Hsync  32 Hback  80 Hpol N
> > > > > >>>>Vfront3 Vsync   5 Vback  69 Vpol N
> > > > > >>>>
> > > > > >>>> I'm proposing here that the above is actually a bug and that the 
> > > > > >>>> 60 Hz
> > > > > >>>> mode really should be considered preferred by Linux.
> > > > >
> > > > > Its a bit tricky to say that this is a bug but I think we can 
> > > > > certainly
> > > > > add here that for an internal display we would have ideally had the
> > > > > lower resolution first to indicate it as default.
> > > >
> > > > Yeah, it gets into the vagueness of the EDID spec in general. As far
> > > > as I can find it's really up to the monitor to decide by what means it
> > > > chooses the "preferred" refresh rate if the monitor can support many.
> > > > Some displays may decide that the normal rate is "preferred" and some
> > > > may decide that the high refresh rate is "preferred". Neither display
> > > > is "wrong" per say, but it's nice to have some consistency here and to
> > > > make it so that otherwise "dumb" userspace will get something
> > > > reasonable by default. I'll change it to say:
> > > >
> > > > While the EDID spec appears to allow a display to use any criteria for
> > > > picking which refresh mode is "preferred" or "optimal", that vagueness
> > > > is a bit annoying. From Linux's point of view let's choose the 60 Hz
> > > > one as the default.
> > >
> > > And if we start making that decision, it should be for all panels with a
> > > similar constraint, so most likely handled by the core, and the new
> > > policy properly documented.
> > >
> > > Doing that just for a single panel is weird.
> >
> > Yeah, though having a "gen

Re: [Freedreno] [PATCH v6 03/10] drm/msm/dp: use atomic callbacks for DP bridge ops

[Doug Anderson]

Hi,

On Mon, Jul 11, 2022 at 5:57 AM Vinod Polimera
 wrote:
>
> Use atomic variants for DP bridge callback functions so that
> the atomic state can be accessed in the interface drivers.
> The atomic state will help the driver find out if the display
> is in self refresh state.
>
> Signed-off-by: Sankeerth Billakanti 
> Signed-off-by: Vinod Polimera 
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c |  9 ++---
>  drivers/gpu/drm/msm/dp/dp_drm.c | 17 ++---
>  drivers/gpu/drm/msm/dp/dp_drm.h |  9 ++---
>  3 files changed, 22 insertions(+), 13 deletions(-)

Reviewed-by: Douglas Anderson 

Re: [Freedreno] [PATCH v6 00/10] Add PSR support for eDP

[Doug Anderson]

Hi,

On Mon, Jul 11, 2022 at 5:57 AM Vinod Polimera
 wrote:
>
> Changes in v2:
>   - Use dp bridge to set psr entry/exit instead of dpu_enocder.
>   - Don't modify whitespaces.
>   - Set self refresh aware from atomic_check.
>   - Set self refresh aware only if psr is supported.
>   - Provide a stub for msm_dp_display_set_psr.
>   - Move dp functions to bridge code.
>
> Changes in v3:
>   - Change callback names to reflect atomic interfaces.
>   - Move bridge callback change to separate patch as suggested by Dmitry.
>   - Remove psr function declaration from msm_drv.h.
>   - Set self_refresh_aware flag only if psr is supported.
>   - Modify the variable names to simpler form.
>   - Define bit fields for PSR settings.
>   - Add comments explaining the steps to enter/exit psr.
>   - Change DRM_INFO to drm_dbg_db.
>
> Changes in v4:
>   - Move the get crtc functions to drm_atomic.
>   - Add atomic functions for DP bridge too.
>   - Add ternary operator to choose eDP or DP ops.
>   - Return true/false instead of 1/0.
>   - mode_valid missing in the eDP bridge ops.
>   - Move the functions to get crtc into drm_atomic.c.
>   - Fix compilation issues.
>   - Remove dpu_assign_crtc and get crtc from drm_enc instead of dpu_enc.
>   - Check for crtc state enable while reserving resources.
>
> Changes in v5:
>   - Move the mode_valid changes into a different patch.
>   - Complete psr_op_comp only when isr is set.
>   - Move the DP atomic callback changes to a different patch.
>   - Get crtc from drm connector state crtc.
>   - Move to separate patch for check for crtc state enable while
> reserving resources.
>
> Changes in v6:
>   - Remove crtc from dpu_encoder_virt struct.
>   - fix crtc check during vblank toggle crtc.
>   - Misc changes.
>
> Signed-off-by: Sankeerth Billakanti 
> Signed-off-by: Kalyan Thota 
> Signed-off-by: Vinod Polimera 
>
> Vinod Polimera (10):
>   drm/msm/disp/dpu: clear dpu_assign_crtc and get crtc from connector
> state instead of dpu_enc
>   drm: add helper functions to retrieve old and new crtc
>   drm/msm/dp: use atomic callbacks for DP bridge ops
>   drm/msm/dp: Add basic PSR support for eDP
>   drm/msm/dp: use the eDP bridge ops to validate eDP modes
>   drm/bridge: use atomic enable/disable callbacks for panel bridge
>   drm/bridge: add psr support for panel bridge callbacks
>   drm/msm/disp/dpu: use atomic enable/disable callbacks for encoder
> functions
>   drm/msm/disp/dpu: add PSR support for eDP interface in dpu driver
>   drm/msm/disp/dpu: check for crtc enable rather than crtc active to
> release shared resources
>
>  drivers/gpu/drm/bridge/panel.c  |  68 --
>  drivers/gpu/drm/drm_atomic.c|  60 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c|  17 ++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  56 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |   8 --
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c |   2 +-
>  drivers/gpu/drm/msm/dp/dp_catalog.c |  81 
>  drivers/gpu/drm/msm/dp/dp_catalog.h |   4 +
>  drivers/gpu/drm/msm/dp/dp_ctrl.c|  73 +++
>  drivers/gpu/drm/msm/dp/dp_ctrl.h|   3 +
>  drivers/gpu/drm/msm/dp/dp_display.c |  31 +++--
>  drivers/gpu/drm/msm/dp/dp_display.h |   2 +
>  drivers/gpu/drm/msm/dp/dp_drm.c | 184 
> ++--
>  drivers/gpu/drm/msm/dp/dp_drm.h |   9 +-
>  drivers/gpu/drm/msm/dp/dp_link.c|  36 ++
>  drivers/gpu/drm/msm/dp/dp_panel.c   |  22 
>  drivers/gpu/drm/msm/dp/dp_panel.h   |   6 +
>  drivers/gpu/drm/msm/dp/dp_reg.h |  27 
>  include/drm/drm_atomic.h|   7 ++
>  19 files changed, 631 insertions(+), 65 deletions(-)

I spent some time looking at the first few patches. I can try to look
at more later this week, though (as you've noticed) many of my reviews
are more nit-picks because I don't really have experience with PSR and
my overall knowledge of the Qualcomm DP driver is pretty weak.

I tried to at least pick to give a Tested-by, but when I did that it
didn't work flawlessly. I picked this series to the chromeos-5.15
tree, which is pretty close to mainline right now. I left it sitting
at a screen with a blinking cursor which pretty much means it's always
transitioning into and out of PSR. I've seen several glitches on the
screen with the series applied. :( No idea what's wrong--that's just
me black-box testing. I did try to add debug printouts to see if we
were hitting "PSR_UPDATE_ERROR_INT | PSR_WAKE_ERROR_INT" but I didn't
see my printouts...

FWIW: I'm running with KASAN enabled which could affect timings...
Glitches happen every few minutes or so for me and so far I haven't
see any glitches without KASAN, but that could just be chance...

-Doug

Re: [Freedreno] [PATCH v6 04/10] drm/msm/dp: Add basic PSR support for eDP

[Doug Anderson]

Hi,

On Mon, Jul 11, 2022 at 5:57 AM Vinod Polimera
 wrote:
>
> @@ -359,6 +367,24 @@ void dp_catalog_ctrl_lane_mapping(struct dp_catalog 
> *dp_catalog)
> ln_mapping);
>  }
>
> +void dp_catalog_ctrl_psr_mainlink_enable(struct dp_catalog *dp_catalog,
> +   bool enable)
> +{
> +   u32 val;
> +   struct dp_catalog_private *catalog = container_of(dp_catalog,
> +   struct dp_catalog_private, dp_catalog);
> +
> +   val = dp_read_link(catalog, REG_DP_MAINLINK_CTRL);
> +   val &= ~DP_MAINLINK_CTRL_ENABLE;

nit: the line above is useless. Remove. In the case that you're
enabling you're just adding the bit back in. In the case that you're
disabling you're doing the exact same operation below.


> @@ -610,6 +636,47 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog 
> *dp_catalog)
> dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, DP_DP_HPD_CTRL_HPD_EN);
>  }
>
> +static void dp_catalog_enable_sdp(struct dp_catalog_private *catalog)
> +{
> +   /* trigger sdp */
> +   dp_write_link(catalog, MMSS_DP_SDP_CFG3, UPDATE_SDP);
> +   dp_write_link(catalog, MMSS_DP_SDP_CFG3, !UPDATE_SDP);

!UPDATE_SDP is a really counter-intuitive way to say 0x0.


> @@ -645,6 +712,20 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog 
> *dp_catalog)
> return isr & (mask | ~DP_DP_HPD_INT_MASK);
>  }
>
> +int dp_catalog_ctrl_read_psr_interrupt_status(struct dp_catalog *dp_catalog)

Why is the return type "int" and not "u32". It's a hardware register.
It's u32 here. The caller assigns it to a u32.


> +void dp_ctrl_set_psr(struct dp_ctrl *dp_ctrl, bool enter)
> +{
> +   struct dp_ctrl_private *ctrl = container_of(dp_ctrl,
> +   struct dp_ctrl_private, dp_ctrl);
> +
> +   if (!ctrl->panel->psr_cap.version)
> +   return;
> +
> +   /*
> +* When entering PSR,
> +* 1. Send PSR enter SDP and wait for the PSR_UPDATE_INT
> +* 2. Turn off video
> +* 3. Disable the mainlink
> +*
> +* When exiting PSR,
> +* 1. Enable the mainlink
> +* 2. Send the PSR exit SDP
> +*/
> +   if (enter) {
> +   reinit_completion(>psr_op_comp);
> +   dp_catalog_ctrl_set_psr(ctrl->catalog, true);
> +
> +   if (!wait_for_completion_timeout(>psr_op_comp,
> +   PSR_OPERATION_COMPLETION_TIMEOUT_JIFFIES)) {
> +   DRM_ERROR("PSR_ENTRY timedout\n");
> +   dp_catalog_ctrl_set_psr(ctrl->catalog, false);
> +   return;
> +   }
> +
> +   dp_catalog_ctrl_state_ctrl(ctrl->catalog, 0);
> +
> +   dp_catalog_ctrl_psr_mainlink_enable(ctrl->catalog, false);
> +   } else {
> +   dp_catalog_ctrl_psr_mainlink_enable(ctrl->catalog, true);
> +
> +   dp_catalog_ctrl_set_psr(ctrl->catalog, false);

My PSR knowledge is not very strong, but I do remember a recent commit
from Brian Norris fly by for the Analogix controller. See commit
c4c6ef229593 ("drm/bridge: analogix_dp: Make PSR-exit block less").

In that commit it sounds as if we need to wait for _something_ (resync
I guess?) here and not just return instantly.


> @@ -388,6 +388,8 @@ static int dp_display_process_hpd_high(struct 
> dp_display_private *dp)
>
> edid = dp->panel->edid;
>
> +   dp->dp_display.psr_supported = !!dp->panel->psr_cap.version;
> +

nit: remove the "!!". You're already storing this in a "bool" which
will handle this for you.


> +static const struct drm_bridge_funcs edp_bridge_ops = {
> +   .atomic_enable = edp_bridge_atomic_enable,
> +   .atomic_disable = edp_bridge_atomic_disable,
> +   .atomic_post_disable = edp_bridge_atomic_post_disable,
> +   .mode_set = dp_bridge_mode_set,
> +   .mode_valid = dp_bridge_mode_valid,
> +   .atomic_reset = drm_atomic_helper_bridge_reset,
> +   .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> +   .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> +   .atomic_check = edp_bridge_atomic_check,
> +};

nit: the location of your new functions is a little weird. You've got:

1. DP functions
2. eDP functions
3. eDP structure
4. DP structure

I'd expect:

1. DP functions
2. DP structure
3. eDP functions
4. eDP structure

-Doug

Re: [Freedreno] [PATCH v6 02/10] drm: add helper functions to retrieve old and new crtc

[Doug Anderson]

Hi,

On Mon, Jul 11, 2022 at 5:57 AM Vinod Polimera
 wrote:
>
> Add new helper functions, drm_atomic_get_old_crtc_for_encoder
> and drm_atomic_get_new_crtc_for_encoder to retrieve the
> corresponding crtc for the encoder.
>
> Signed-off-by: Sankeerth Billakanti 
> Signed-off-by: Vinod Polimera 
> ---
>  drivers/gpu/drm/drm_atomic.c | 60 
> 
>  include/drm/drm_atomic.h |  7 ++
>  2 files changed, 67 insertions(+)

I don't have a lot of intuition about the code here since I haven't
messed much at this level, but what you have here looks right and
matches other similar helpers. I'm happy enough with:

Reviewed-by: Douglas Anderson 

Re: [Freedreno] [PATCH v6 01/10] drm/msm/disp/dpu: clear dpu_assign_crtc and get crtc from connector state instead of dpu_enc

[Doug Anderson]

Hi,

On Mon, Jul 11, 2022 at 5:57 AM Vinod Polimera
 wrote:
>
> Update crtc retrieval from dpu_enc to dpu_enc connector state,
> since new links get set as part of the dpu enc virt mode set.
> The dpu_enc->crtc cache is no more needed, hence cleaning it as
> part of this change.

I don't know this driver terribly well, but _why_ is it no longer
needed? According to the kernel-doc for the "crtc" variable you're
removing it was because we used to need it after the disable()
callback. Maybe that's no longer the case after commit a796ba2cb3dd
("drm/msm: dpu: Separate crtc assignment from vblank enable")?


> Signed-off-by: Vinod Polimera 
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c|  4 
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 30 
> ++---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  8 
>  3 files changed, 14 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index b56f777..f91e3d1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -972,7 +972,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
>  */
> if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO)
> release_bandwidth = true;
> -   dpu_encoder_assign_crtc(encoder, NULL);
> }
>
> /* wait for frame_event_done completion */
> @@ -1042,9 +1041,6 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
> trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
> dpu_crtc->enabled = true;
>
> -   drm_for_each_encoder_mask(encoder, crtc->dev, 
> crtc->state->encoder_mask)
> -   dpu_encoder_assign_crtc(encoder, crtc);
> -
> /* Enable/restore vblank irq handling */
> drm_crtc_vblank_on(crtc);
>  }
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 52516eb..0fddc9d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -181,7 +181,6 @@ struct dpu_encoder_virt {
>
> bool intfs_swapped;
>
> -   struct drm_crtc *crtc;

This structure is documented by kernel-doc. That means you need to
remove the documentation for "crtc".


> struct drm_connector *connector;
>
> struct dentry *debugfs_root;
> @@ -1245,6 +1244,7 @@ static void dpu_encoder_vblank_callback(struct 
> drm_encoder *drm_enc,
> struct dpu_encoder_phys *phy_enc)
>  {
> struct dpu_encoder_virt *dpu_enc = NULL;
> +   struct drm_crtc *crtc;
> unsigned long lock_flags;
>
> if (!drm_enc || !phy_enc)
> @@ -1253,9 +1253,14 @@ static void dpu_encoder_vblank_callback(struct 
> drm_encoder *drm_enc,
> DPU_ATRACE_BEGIN("encoder_vblank_callback");
> dpu_enc = to_dpu_encoder_virt(drm_enc);
>
> +   if (!dpu_enc->connector || !dpu_enc->connector->state)
> +   return;

FWIW: your patch doesn't apply cleanly to msm-next. It conflicts with
commit c28d76d360f9 ("drm/msm/dpu: Increment vsync_cnt before waking
up userspace").

I suspect that you'll want your changes to come _after_ the increment
(AKA you want to increment even if the connector is NULL), but dunno
for sure.


> +
> +   crtc = dpu_enc->connector->state->crtc;
> +
> spin_lock_irqsave(_enc->enc_spinlock, lock_flags);
> -   if (dpu_enc->crtc)
> -   dpu_crtc_vblank_callback(dpu_enc->crtc);
> +   if (crtc)
> +   dpu_crtc_vblank_callback(crtc);

Effectively you are checking for NULLness at 3 levels:

1. dpu_enc->connector
2. dpu_enc->connector->state
3. dpu_enc->connector->state->crtc

You check two of those things outside of the spinlock and one of those
things inside the spinlock. Why? Should they all be inside the
spinlock, or can they all be outside of the spinlock, or is there some
reason it is the way it is?


>  void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc,
> struct drm_crtc *crtc, bool enable)
>  {
> struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> +   struct drm_crtc *new_crtc;
> unsigned long lock_flags;
> int i;
>
> trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable);
>
> +   if (!dpu_enc->connector || !dpu_enc->connector->state)
> +   return;
> +
> +   new_crtc = dpu_enc->connector->state->crtc;
> spin_lock_irqsave(_enc->enc_spinlock, lock_flags);
> -   if (dpu_enc->crtc != crtc) {
> +   if (!new_crtc || new_crtc != crtc) {
> spin_unlock_irqrestore(_enc->enc_spinlock, lock_flags);

Even if there was some reason for your choice of where you did the
spinlock in the previous case, I'm 95% sure that this one is absurd.
You're locking a spinlock around a test of local variables? I'm pretty
sure nobody 

Re: [Freedreno] [RFC PATCH] drm/edid: Make 144 Hz not preferred on Sharp LQ140M1JW46

[Doug Anderson]

Hi,

On Thu, Jul 28, 2022 at 10:34 AM Abhinav Kumar
 wrote:
>
> Hi Rob and Doug
>
> On 7/22/2022 10:36 AM, Rob Clark wrote:
> > On Fri, Jul 22, 2022 at 9:48 AM Doug Anderson  wrote:
> >>
> >> Hi,
> >>
> >> On Fri, Jul 22, 2022 at 9:37 AM Abhinav Kumar  
> >> wrote:
> >>>
> >>> + sankeerth
> >>>
> >>> Hi Doug
> >>>
> >>> On 7/21/2022 3:23 PM, Douglas Anderson wrote:
> >>>> The Sharp LQ140M1JW46 panel is on the Qualcomm sc7280 CRD reference
> >>>> board. This panel supports 144 Hz and 60 Hz. In the EDID, the 144 Hz
> >>>> mode is listed first and thus is marked preferred. The EDID decode I
> >>>> ran says:
> >>>>
> >>>> First detailed timing includes the native pixel format and preferred
> >>>> refresh rate.
> >>>>
> >>>> ...
> >>>>
> >>>> Detailed Timing Descriptors:
> >>>>   DTD 1:  1920x1080  143.981 Hz  16:9   166.587 kHz  346.500 MHz
> >>>>Hfront   48 Hsync  32 Hback  80 Hpol N
> >>>>Vfront3 Vsync   5 Vback  69 Vpol N
> >>>>   DTD 2:  1920x1080   59.990 Hz  16:969.409 kHz  144.370 MHz
> >>>>Hfront   48 Hsync  32 Hback  80 Hpol N
> >>>>Vfront3 Vsync   5 Vback  69 Vpol N
> >>>>
> >>>> I'm proposing here that the above is actually a bug and that the 60 Hz
> >>>> mode really should be considered preferred by Linux.
>
> Its a bit tricky to say that this is a bug but I think we can certainly
> add here that for an internal display we would have ideally had the
> lower resolution first to indicate it as default.

Yeah, it gets into the vagueness of the EDID spec in general. As far
as I can find it's really up to the monitor to decide by what means it
chooses the "preferred" refresh rate if the monitor can support many.
Some displays may decide that the normal rate is "preferred" and some
may decide that the high refresh rate is "preferred". Neither display
is "wrong" per say, but it's nice to have some consistency here and to
make it so that otherwise "dumb" userspace will get something
reasonable by default. I'll change it to say:

While the EDID spec appears to allow a display to use any criteria for
picking which refresh mode is "preferred" or "optimal", that vagueness
is a bit annoying. From Linux's point of view let's choose the 60 Hz
one as the default.


> >>>> The argument here is that this is a laptop panel and on a laptop we
> >>>> know power will always be a concern. Presumably even if someone using
> >>>> this panel wanted to use 144 Hz for some use cases they would only do
> >>>> so dynamically and would still want the default to be 60 Hz.
> >>>>
> >>>> Let's change the default to 60 Hz using a standard quirk.
> >>>>
> >>>> Signed-off-by: Douglas Anderson 
> >>>
> >>> Yes, we were aware that 144Hz was getting picked. We found that while
> >>> debugging the screen corruption issue.
> >>>
> >>> Well, yes power would be less with 60Hz but so will be the performance.
> >>
> >> What performance specifically will be less with 60 Hz? In general the
> >> sc7280 CPU is a bit memory-bandwidth constrained and the LCD refresh
> >> from memory is a non-trivial part of that. Reducing to 60 Hz will
> >> relieve some of the memory bandwidth pressure and will actually allow
> >> tasks on the CPU to run _faster_. I guess the downside is that some
> >> animations might be a little less smooth...
> >
> > I guess he is referring to something that is vblank sync'd running
> > faster than 60fps.
> >
> > but OTOH it is a bit of a waste for fbcon to be using 144Hz.  And
> > there are enough android games that limit themselves to 30fps to save
> > your "phone" battery.  So it seems a lot more sane to default to 60Hz
> > and let userspace that knows it wants more pick the 144Hz rate when
> > needed.
> >
> > BR,
> > -R
>
> Yes i was referring to vblank synced apps.
>
> >
> >>
> >>
> >>> The test teams have been validating with 144Hz so far so we are checking
> >>> internally with the team whether its OKAY to goto 60Hz now since that
> >>> kind of invalidates the testing they have been doing.
> >>
>

Re: [Freedreno] [PATCH] drm/msm/dsi: Don't set a load before disabling a regulator

[Doug Anderson]

Hi,

On Wed, Jul 27, 2022 at 6:59 AM Dmitry Baryshkov
 wrote:
>
> On Wed, 27 Jul 2022 at 16:57, Doug Anderson  wrote:
> >
> > Hi,
> >
> > On Tue, Jul 26, 2022 at 4:53 PM Abhinav Kumar  
> > wrote:
> > >
> > > On 7/25/2022 5:49 PM, Douglas Anderson wrote:
> > > > As of commit 5451781dadf8 ("regulator: core: Only count load for
> > > > enabled consumers"), a load isn't counted for a disabled
> > > > regulator. That means all the code in the DSI driver to specify and
> > > > set loads before disabling a regulator is not actually doing anything
> > > > useful. Let's remove it.
> > > >
> > > > It should be noted that all of the loads set that were being specified
> > > > were pointless noise anyway. The only use for this number is to pick
> > > > between low power and high power modes of regulators. Regulators
> > > > appear to do this changeover at loads on the order of 1 uA. You
> > > > would a lot of clients of the same rail for that 100 uA number to
> > >
> > > I guess you meant "you would need a lot of clients"
> >
> > Yeah, sorry. :( I'll fix it up if I need a v3.
> >
> >
> > > > @@ -259,15 +259,7 @@ static inline struct msm_dsi_host 
> > > > *to_msm_dsi_host(struct mipi_dsi_host *host)
> > > >   static void dsi_host_regulator_disable(struct msm_dsi_host *msm_host)
> > > >   {
> > > It seems like now we can drop this function dsi_host_regulator_disable()
> > > entirely and just call regulator_bulk_disable() ?
> >
> > Sure, if you want. One could still argue that it provides a tiny bit
> > of abstraction and avoids the caller from having to know where to find
> > the number of regulators and all that, but I can go either way. Is
> > this worth a v3, do you think? If so, I might tack it on at the end of
> > the series.
>
> I'd say, drop it. Having extra single-call wrappers doesn't seem to add a lot.

Sounds good. I'll wait a little bit of time for feedback on the larger
series and then send a v3, probably next week.

-Doug

Re: [Freedreno] [PATCH] drm/msm/dsi: Don't set a load before disabling a regulator

[Doug Anderson]

Hi,

On Tue, Jul 26, 2022 at 4:53 PM Abhinav Kumar  wrote:
>
> On 7/25/2022 5:49 PM, Douglas Anderson wrote:
> > As of commit 5451781dadf8 ("regulator: core: Only count load for
> > enabled consumers"), a load isn't counted for a disabled
> > regulator. That means all the code in the DSI driver to specify and
> > set loads before disabling a regulator is not actually doing anything
> > useful. Let's remove it.
> >
> > It should be noted that all of the loads set that were being specified
> > were pointless noise anyway. The only use for this number is to pick
> > between low power and high power modes of regulators. Regulators
> > appear to do this changeover at loads on the order of 1 uA. You
> > would a lot of clients of the same rail for that 100 uA number to
>
> I guess you meant "you would need a lot of clients"

Yeah, sorry. :( I'll fix it up if I need a v3.


> > @@ -259,15 +259,7 @@ static inline struct msm_dsi_host 
> > *to_msm_dsi_host(struct mipi_dsi_host *host)
> >   static void dsi_host_regulator_disable(struct msm_dsi_host *msm_host)
> >   {
> It seems like now we can drop this function dsi_host_regulator_disable()
> entirely and just call regulator_bulk_disable() ?

Sure, if you want. One could still argue that it provides a tiny bit
of abstraction and avoids the caller from having to know where to find
the number of regulators and all that, but I can go either way. Is
this worth a v3, do you think? If so, I might tack it on at the end of
the series.

Note that I say "v3" because I actually included this patch in a
larger series and called that series "v2" [1].


[1] https://lore.kernel.org/r/20220726173824.1166873-1-diand...@chromium.org

Re: [Freedreno] [RFC PATCH] drm/edid: Make 144 Hz not preferred on Sharp LQ140M1JW46

[Doug Anderson]

Hi,

On Fri, Jul 22, 2022 at 9:37 AM Abhinav Kumar  wrote:
>
> + sankeerth
>
> Hi Doug
>
> On 7/21/2022 3:23 PM, Douglas Anderson wrote:
> > The Sharp LQ140M1JW46 panel is on the Qualcomm sc7280 CRD reference
> > board. This panel supports 144 Hz and 60 Hz. In the EDID, the 144 Hz
> > mode is listed first and thus is marked preferred. The EDID decode I
> > ran says:
> >
> >First detailed timing includes the native pixel format and preferred
> >refresh rate.
> >
> >...
> >
> >Detailed Timing Descriptors:
> >  DTD 1:  1920x1080  143.981 Hz  16:9   166.587 kHz  346.500 MHz
> >   Hfront   48 Hsync  32 Hback  80 Hpol N
> >   Vfront3 Vsync   5 Vback  69 Vpol N
> >  DTD 2:  1920x1080   59.990 Hz  16:969.409 kHz  144.370 MHz
> >   Hfront   48 Hsync  32 Hback  80 Hpol N
> >   Vfront3 Vsync   5 Vback  69 Vpol N
> >
> > I'm proposing here that the above is actually a bug and that the 60 Hz
> > mode really should be considered preferred by Linux.
> >
> > The argument here is that this is a laptop panel and on a laptop we
> > know power will always be a concern. Presumably even if someone using
> > this panel wanted to use 144 Hz for some use cases they would only do
> > so dynamically and would still want the default to be 60 Hz.
> >
> > Let's change the default to 60 Hz using a standard quirk.
> >
> > Signed-off-by: Douglas Anderson 
>
> Yes, we were aware that 144Hz was getting picked. We found that while
> debugging the screen corruption issue.
>
> Well, yes power would be less with 60Hz but so will be the performance.

What performance specifically will be less with 60 Hz? In general the
sc7280 CPU is a bit memory-bandwidth constrained and the LCD refresh
from memory is a non-trivial part of that. Reducing to 60 Hz will
relieve some of the memory bandwidth pressure and will actually allow
tasks on the CPU to run _faster_. I guess the downside is that some
animations might be a little less smooth...


> The test teams have been validating with 144Hz so far so we are checking
> internally with the team whether its OKAY to goto 60Hz now since that
> kind of invalidates the testing they have been doing.

You're worried that the panel itself won't work well at 60 Hz, or
something else about the system won't? The whole system in general
needs to work well with 60 Hz displays and I expect them to be much
more common than 144 Hz displays. Quite honestly if switching to 60 Hz
uncovers a problem that would be a huge benefit of landing this patch
because it would mean we'd find it now rather than down the road when
someone hooks up a different panel.

-Doug

Re: [Freedreno] [PATCH v16 0/3] eDP/DP Phy vdda realted function

[Doug Anderson]

Hi,

On Thu, Jul 21, 2022 at 7:52 AM Doug Anderson  wrote:
>
> Hi,
>
> On Thu, Jul 21, 2022 at 7:39 AM Doug Anderson  wrote:
> >
> > > You could add a way to specify constant base loads in DT on either a per
> > > regulator or per consumer basis.
> >
> > Yes, this please! ...on a per consumer basis. :-) It's been on my
> > wishlist for a while and would eliminate a massive amount of code /
> > tables in the drivers.
> >
> > We could debate syntax, but I guess you'd either do it w/ two cells
> >
> > vdda-phy-supply = <_mipi_dsi0_1p2 21800>;
> >
> > ...or with matching properties:
> >
> > vdda-phy-supply = <_mipi_dsi0_1p2>;
> > vdda-phy-supply-base-load = <21800>;
>
> Ah, sorry to respond to my own thread so quickly, but I just thought
> of a reason for the "matching properties" solution instead of the two
> cells. It would allow the SoC "dtsi" file to specify a base load while
> the board "dts" file can then specify the supply. That feels like it
> could be a nice solution.

This seemed easy, so I whipped up a quick prototype. Forewarned that I
did very little detailed testing. I didn't CC everyone on this thread,
but hopefully folks can find it if they are interested...

https://lore.kernel.org/r/20220721182622.RFC.1.I8a64b707169cfd73d9309c5eaf5d43b8bc4db988@changeid

-Doug

Re: [Freedreno] [PATCH v16 0/3] eDP/DP Phy vdda realted function

[Doug Anderson]

Hi,

On Thu, Jul 21, 2022 at 8:06 AM Mark Brown  wrote:
>
> On Thu, Jul 21, 2022 at 07:49:55AM -0700, Doug Anderson wrote:
>
> > Every single LDO on Qualcomm's PMICs seems to be able to be set in
> > "high power mode" and "low power mode", but I think the majority of
> > clients only really care about two things: on and in high power mode
> > vs. off. I think the amount of stuff peripherals can do in low power
> > mode is super limited, so you have to be _really_ sure that the
> > peripheral won't draw too much current without you having a chance to
> > reconfigure the regulator.
>
> *Generally* a low power mode would be mainly useful for low power
> retention type states, not active use.

Right. Certainly the case I've seen where it is most useful is in S3
where we need to keep a device powered just enough to detect a wakeup,
but it can definitely also be useful for mostly idle devices that we
need to keep powered to retain memory so they can start up again
quickly.

I guess I'd put it this way, though: how many drivers in Linux today
have _two_ calls to regulator_set_load(): one for the "active" state
and one for the retention state. Looks like UFS maybe. Any others? For
most devices the pattern is:

* get all of our regulators.
* for each regulator, set the load to something that will trigger HPM
when we're using the regulator.
* turn regulators on when we need power and off when we don't.

All the extra scaffolding and tables to pass something to
regulator_set_load() is just a lot of noise to add for drivers that
don't have any concept of "retention" mode and don't need it.

-Doug

Re: [Freedreno] [PATCH v16 0/3] eDP/DP Phy vdda realted function

[Doug Anderson]

Hi,

On Thu, Jul 21, 2022 at 6:25 AM Dmitry Baryshkov
 wrote:
>
> > This series breaks USB and PCIe for some SC8280XP and SA540P machines
> > where the DP PHY regulators are shared with other PHYs whose drivers do
> > not request a load.
>
> I'm trying to understand, why does this series cause the regression.
> Previously it would be the DP controller voting on the regulators
> load, now it has been moved to the DP/eDP PHYs.

I think in the past not many device trees actually hooked up the
regulators to the DP/eDP but did hook up the regulators to the PHYs?
That means they didn't used to get a regulator_set_load() on them and
now they do?

It should also be noted that we're now setting the load for a bunch of
USB PHYs that we didn't used to set a load on...


> > It seems quite likely that changes like this one affects other systems
> > too, and the effects may be hard to debug. So a more general solution
> > than playing whack-a-mole using DT would be good to have.
>
> I think enabling a regulator which supports set_load() and without
> load being set should cause a warning, at least with
> CONFIG_REGULATOR_DEBUG. WDYT?

I'm not a total fan. I'd love to see evidence to the contrary, but I'm
a believer that the amount of extra cruft involved with all these
regulator_set_load() calls is overkill for most cases. All the extra
code / per-SoC tables added to drivers isn't ideal.

Every single LDO on Qualcomm's PMICs seems to be able to be set in
"high power mode" and "low power mode", but I think the majority of
clients only really care about two things: on and in high power mode
vs. off. I think the amount of stuff peripherals can do in low power
mode is super limited, so you have to be _really_ sure that the
peripheral won't draw too much current without you having a chance to
reconfigure the regulator.

Of course, if the load could be easily specified in the device tree
and we could get rid of all the cruft in the drivers then maybe that
would be OK.

-Doug

Re: [Freedreno] [PATCH v16 0/3] eDP/DP Phy vdda realted function

[Doug Anderson]

Hi,

On Thu, Jul 21, 2022 at 4:20 AM Mark Brown  wrote:
>
> On Thu, Jul 21, 2022 at 12:31:41PM +0200, Johan Hovold wrote:
>
> If you're copying someone into a thread that's not obviously relevant
> for them it's good practice to put a note about it at the top of the
> mail to reduce the chances that it just gets deleted unread - people get
> copies of all sorts of random stuff for not great reasons (like getting
> pulled in by checkpatch due to once having done a cleanup) and are often
> quicky to delete things.
>
> > This series breaks USB and PCIe for some SC8280XP and SA540P machines
> > where the DP PHY regulators are shared with other PHYs whose drivers do
> > not request a load.
>
> > Specifically, the hard-coded vdda-phy load of 21.8 mA added by this
> > series, causes several RPMh regulators to now be put in low-power mode.
>
> > I found Doug's suggestion to handle situations like this in regulator
> > core:
> >
> >   
> > https://lore.kernel.org/all/20180814170617.100087-1-diand...@chromium.org/
>
> > but since that was rejected, how do we deal with this generally?
>
> > In the above thread Doug mentioned adding support for load requests to
> > further drivers and Bjorn mentioned working around it by adding
> > regulator-system-load properties to DT.
>
> > It seems quite likely that changes like this one affects other systems
> > too, and the effects may be hard to debug. So a more general solution
> > than playing whack-a-mole using DT would be good to have.
>
> You could add a way to specify constant base loads in DT on either a per
> regulator or per consumer basis.

Yes, this please! ...on a per consumer basis. :-) It's been on my
wishlist for a while and would eliminate a massive amount of code /
tables in the drivers.

We could debate syntax, but I guess you'd either do it w/ two cells

vdda-phy-supply = <_mipi_dsi0_1p2 21800>;

...or with matching properties:

vdda-phy-supply = <_mipi_dsi0_1p2>;
vdda-phy-supply-base-load = <21800>;

-Doug

Re: [Freedreno] [PATCH] drm/msm/dsi: Set panel orientation when directly connected

[Doug Anderson]

Hi,

On Wed, Jul 20, 2022 at 1:46 PM Rob Clark  wrote:
>
> On Fri, Jul 8, 2022 at 8:25 AM Doug Anderson  wrote:
> >
> > Hi,
> >
> > On Wed, Jul 6, 2022 at 12:14 PM Stephen Boyd  wrote:
> > >
> > > Set the panel orientation in drm when the panel is directly connected,
> > > i.e. we're not using an external bridge. The external bridge case is
> > > already handled by the panel bridge code, so we only update the path we
> > > take when the panel is directly connected/internal. This silences a
> > > warning splat coming from __drm_mode_object_add() on Wormdingler boards.
> > >
> > > Cc: Hsin-Yi Wang 
> > > Cc: Douglas Anderson 
> > > Signed-off-by: Stephen Boyd 
> > > ---
> > >
> > > This relies on commit 5e41b01a7808 ("drm/panel: Add an API to allow drm
> > > to set orientation from panel") which is in drm-misc
> > >
> > >  drivers/gpu/drm/msm/dsi/dsi_manager.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> >
> > I don't personally have objections to this, but (to my understanding)
> > "the future" is that everyone should use panel_bridge. If we made the
> > move to panel_bridge today then we wouldn't need to do this. In
> > general I think panel_bridge would end up letting us delete a bunch of
> > code...
> >
> > See commit 4e5763f03e10 ("drm/bridge: ti-sn65dsi86: Wrap panel with
> > panel-bridge") for when this was done by ti-sn65dsi86.
> >
> > Then again, I spent a small amount of time looking into this and it's
> > definitely non-trivial. Still likely worthwhile, but not worth
> > blocking a tiny fix like this. It also should be fairly obvious that
> > we should delete this when we switch to panel_bridge.
> >
> > Thus:
> >
> > Reviewed-by: Douglas Anderson 
> >
> > I'll assume that we'll just snooze this commit until drm-misc-next
> > merges into a tree that msm-next is based on, which will probably be
> > the next -rc1. If desired and Acked I could land this in
> > drm-misc-next, but it's probably not worth it?
>
> if you want to land this patch via drm-misc, which might be the
> easier/faster route, then:
>
> Acked-by: Rob Clark 

As per discussion on IRC, I'm not going to apply this to drm-misc-next.

Given where we are in the cycle landing in drm-misc-next means it
won't be in mainline for a couple versions and I suspect that'll cause
merge conflicts with Dmitry's series [1]. ...and, of course, if
Dmitry's series lands then we don't even need ${SUBJECT} patch...

So I think the plan is:

1. Snooze waiting for the next -rc1 since
drm_connector_set_orientation_from_panel() won't be in mainline until
then.

2. If Dmitry's series looks like a long way off, we could land
${SUBJECT} patch in msm-next as a stopgap fix.


[1] 
https://lore.kernel.org/r/20220711094320.368062-5-dmitry.barysh...@linaro.org/

-Doug

Re: [Freedreno] [RFC PATCH v3 2/2] drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR

[Doug Anderson]

Hi,

On Mon, Jul 11, 2022 at 10:23 AM Doug Anderson  wrote:
>
> Hi,
>
> On Mon, Jul 11, 2022 at 2:21 AM Dmitry Baryshkov
>  wrote:
> >
> > Now as the driver does not depend on pdata->connector, add support for
> > attaching the bridge with DRM_BRIDGE_ATTACH_NO_CONNECTOR.
> >
> > Reviewed-by: Sam Ravnborg 
> > Reviewed-by: Laurent Pinchart 
> > Signed-off-by: Dmitry Baryshkov 
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 18 --
> >  1 file changed, 8 insertions(+), 10 deletions(-)
>
>  This has been on my list of annoyances for quite some time now. Most
> excellent to have it fixed, thanks!
>
> Reviewed-by: Douglas Anderson 
>
> Tested together with patch #1.
>
> Tested-by: Douglas Anderson 
>
>
> Unless someone yells that there's a problem or someone beats me to it,
> I'll plan to land in drm-misc-next sometime next week.

Landed on drm-misc-next:

6e2dc7ac7141 drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR

Re: [Freedreno] [RFC PATCH v3 1/2] drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state

[Doug Anderson]

Hi,

On Mon, Jul 11, 2022 at 2:21 AM Dmitry Baryshkov
 wrote:
>
> Rather than reading the pdata->connector directly, fetch the connector
> using drm_atomic_state. This allows us to make pdata->connector optional
> (and thus supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR).
>
> Reviewed-by: Sam Ravnborg 
> Reviewed-by: Laurent Pinchart 
> Signed-off-by: Dmitry Baryshkov 
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 22 --
>  1 file changed, 16 insertions(+), 6 deletions(-)

Landed on drm-misc-next:

2dbeef82d14f drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state

Re: [Freedreno] [PATCH v2 5/7] arm64: dts: qcom: sc7280: Update gpu register list

[Doug Anderson]

Hi,

On Fri, Jul 8, 2022 at 11:00 PM Akhil P Oommen  wrote:
>
> Update gpu register array with gpucc memory region.
>
> Signed-off-by: Akhil P Oommen 
> ---
>
> (no changes since v1)
>
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi 
> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index e66fc67..defdb25 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -2228,10 +2228,12 @@
> compatible = "qcom,adreno-635.0", "qcom,adreno";
> reg = <0 0x03d0 0 0x4>,
>   <0 0x03d9e000 0 0x1000>,
> - <0 0x03d61000 0 0x800>;
> + <0 0x03d61000 0 0x800>,
> + <0 0x03d9 0 0x2000>;
> reg-names = "kgsl_3d0_reg_memory",
> "cx_mem",
> -   "cx_dbgc";
> +   "cx_dbgc",
> +   "gpucc";

This doesn't seem right. Shouldn't you be coordinating with the
existing gpucc instead of reaching into its registers?

-Doug

Re: [Freedreno] [PATCH v2 3/7] drm/msm: Fix cx collapse issue during recovery

[Doug Anderson]

Hi,

On Fri, Jul 8, 2022 at 11:00 PM Akhil P Oommen  wrote:
>
> There are some hardware logic under CX domain. For a successful
> recovery, we should ensure cx headswitch collapses to ensure all the
> stale states are cleard out. This is especially true to for a6xx family
> where we can GMU co-processor.
>
> Currently, cx doesn't collapse due to a devlink between gpu and its
> smmu. So the *struct gpu device* needs to be runtime suspended to ensure
> that the iommu driver removes its vote on cx gdsc.
>
> Signed-off-by: Akhil P Oommen 
> ---
>
> (no changes since v1)
>
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 16 ++--
>  drivers/gpu/drm/msm/msm_gpu.c |  2 --
>  2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c 
> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 4d50110..7ed347c 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -1278,8 +1278,20 @@ static void a6xx_recover(struct msm_gpu *gpu)
>  */
> gmu_write(_gpu->gmu, REG_A6XX_GMU_GMU_PWR_COL_KEEPALIVE, 0);
>
> -   gpu->funcs->pm_suspend(gpu);
> -   gpu->funcs->pm_resume(gpu);
> +   /*
> +* Now drop all the pm_runtime usage count to allow cx gdsc to 
> collapse.
> +* First drop the usage count from all active submits
> +*/
> +   for (i = gpu->active_submits; i > 0; i--)
> +   pm_runtime_put(>pdev->dev);
> +
> +   /* And the final one from recover worker */
> +   pm_runtime_put_sync(>pdev->dev);
> +
> +   for (i = gpu->active_submits; i > 0; i--)
> +   pm_runtime_get(>pdev->dev);
> +
> +   pm_runtime_get_sync(>pdev->dev);

In response to v1, Rob suggested pm_runtime_force_suspend/resume().
Those seem like they would work to me, too. Why not use them?

Re: [Freedreno] [RFC PATCH v3 2/2] drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR

[Doug Anderson]

Hi,

On Mon, Jul 11, 2022 at 2:21 AM Dmitry Baryshkov
 wrote:
>
> Now as the driver does not depend on pdata->connector, add support for
> attaching the bridge with DRM_BRIDGE_ATTACH_NO_CONNECTOR.
>
> Reviewed-by: Sam Ravnborg 
> Reviewed-by: Laurent Pinchart 
> Signed-off-by: Dmitry Baryshkov 
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 18 --
>  1 file changed, 8 insertions(+), 10 deletions(-)

 This has been on my list of annoyances for quite some time now. Most
excellent to have it fixed, thanks!

Reviewed-by: Douglas Anderson 

Tested together with patch #1.

Tested-by: Douglas Anderson 


Unless someone yells that there's a problem or someone beats me to it,
I'll plan to land in drm-misc-next sometime next week.

-Doug

Re: [Freedreno] [RFC PATCH v3 1/2] drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state

[Doug Anderson]

Hi,

On Mon, Jul 11, 2022 at 2:21 AM Dmitry Baryshkov
 wrote:
>
> Rather than reading the pdata->connector directly, fetch the connector
> using drm_atomic_state. This allows us to make pdata->connector optional
> (and thus supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR).
>
> Reviewed-by: Sam Ravnborg 
> Reviewed-by: Laurent Pinchart 
> Signed-off-by: Dmitry Baryshkov 
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 22 --
>  1 file changed, 16 insertions(+), 6 deletions(-)

Reviewed-by: Douglas Anderson 

I tested this on a google,lazor-rev5-sku6 (your code correctly found
bpc as 6) and a google,coachz-rev2-sku0 (your code correctly found bpc
as 8).

As an extra safety net, I also double-checked that the generic
edp-panel would work. I also hacked the first device to use
"edp-panel" as a compatible string, found the panel to be detected,
and found bpc was properly found as 6.

So from a testing perspective this seems good.

Tested-by: Douglas Anderson 

Re: [Freedreno] [PATCH v2] arm64: dta: qcom: sc7280: delete vdda-1p2 and vdda-0p9 from both dp and edp

[Doug Anderson]

Hi,

On Mon, Jul 11, 2022 at 9:23 AM Kuogee Hsieh  wrote:
>
> Both vdda-1p2-supply and vdda-0p9-supply regulators are controlled
> by dp combo phy. Therefore remove them from dp controller.
>
> Signed-off-by: Kuogee Hsieh 
> ---
>  arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 2 --
>  arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi | 3 ---
>  2 files changed, 5 deletions(-)

Reviewed-by: Douglas Anderson 

Re: [Freedreno] [PATCH] arm64: dta: qcom: sc7280: delete vdda-1p2 and vdda-0p9 from mdss_edp

[Doug Anderson]

Hi,

On Mon, Jul 11, 2022 at 9:10 AM Kuogee Hsieh  wrote:
>
>
> On 7/11/2022 9:02 AM, Doug Anderson wrote:
>
> Hi,
>
> On Mon, Jul 11, 2022 at 8:58 AM Kuogee Hsieh  wrote:
>
> Both vdda-1p2-supply and vdda-0p9-supply regulators are controlled
> by dp combo phy. Therefore remove them from dp controller.
>
> Signed-off-by: Kuogee Hsieh 
> ---
>  arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi | 3 ---
>  1 file changed, 3 deletions(-)
>
> You also need to remove it from
> `arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi`.
>
> arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi: vdda-1p2-supply = 
> <_a_usbssdp_0_1p2>;
>
> I did not see above line at sc7280-herobrine.dtsi at latest msm-next tree.

It's in the qcom tree...

https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git/tree/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi?h=for-next#n438

-Doug

Re: [Freedreno] [PATCH] arm64: dta: qcom: sc7280: delete vdda-1p2 and vdda-0p9 from mdss_edp

[Doug Anderson]

Hi,

On Mon, Jul 11, 2022 at 8:58 AM Kuogee Hsieh  wrote:
>
> Both vdda-1p2-supply and vdda-0p9-supply regulators are controlled
> by dp combo phy. Therefore remove them from dp controller.
>
> Signed-off-by: Kuogee Hsieh 
> ---
>  arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi | 3 ---
>  1 file changed, 3 deletions(-)

You also need to remove it from
`arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi`.

-Doug

Re: [Freedreno] [PATCH] drm/msm/dsi: Set panel orientation when directly connected

[Doug Anderson]

Hi,

On Wed, Jul 6, 2022 at 12:14 PM Stephen Boyd  wrote:
>
> Set the panel orientation in drm when the panel is directly connected,
> i.e. we're not using an external bridge. The external bridge case is
> already handled by the panel bridge code, so we only update the path we
> take when the panel is directly connected/internal. This silences a
> warning splat coming from __drm_mode_object_add() on Wormdingler boards.
>
> Cc: Hsin-Yi Wang 
> Cc: Douglas Anderson 
> Signed-off-by: Stephen Boyd 
> ---
>
> This relies on commit 5e41b01a7808 ("drm/panel: Add an API to allow drm
> to set orientation from panel") which is in drm-misc
>
>  drivers/gpu/drm/msm/dsi/dsi_manager.c | 2 ++
>  1 file changed, 2 insertions(+)

I don't personally have objections to this, but (to my understanding)
"the future" is that everyone should use panel_bridge. If we made the
move to panel_bridge today then we wouldn't need to do this. In
general I think panel_bridge would end up letting us delete a bunch of
code...

See commit 4e5763f03e10 ("drm/bridge: ti-sn65dsi86: Wrap panel with
panel-bridge") for when this was done by ti-sn65dsi86.

Then again, I spent a small amount of time looking into this and it's
definitely non-trivial. Still likely worthwhile, but not worth
blocking a tiny fix like this. It also should be fairly obvious that
we should delete this when we switch to panel_bridge.

Thus:

Reviewed-by: Douglas Anderson 

I'll assume that we'll just snooze this commit until drm-misc-next
merges into a tree that msm-next is based on, which will probably be
the next -rc1. If desired and Acked I could land this in
drm-misc-next, but it's probably not worth it?

Re: [Freedreno] [PATCH] arm64: dta: qcom: sc7180: delete vdda-1p2 and vdda-0p9 from mdss_dp

[Doug Anderson]

Hi,

On Fri, Jul 1, 2022 at 8:47 AM Kuogee Hsieh  wrote:
>
> Both vdda-1p2-supply and vdda-0p9-supply regulators are controlled
> by dp combo phy. Therefore remove them from dp controller.
>
> Signed-off-by: Kuogee Hsieh 
> ---
>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> index 732e118..824a98c 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> @@ -813,8 +813,6 @@ hp_i2c:  {
> pinctrl-names = "default";
> pinctrl-0 = <_hot_plug_det>;
> data-lanes = <0 1>;
> -   vdda-1p2-supply = <_usb_ss_dp_1p2>;
> -   vdda-0p9-supply = <_usb_ss_dp_core>;
>  };

Reviewed-by: Douglas Anderson 

NOTE: this is somewhat related to Kuogee's series [1] but it's OK to
take even though his series hasn't landed. On trogdor we always keep
these regulators in HPM mode so we're not truly dependent on the
addition of regulator_set_load in the DP PHY.

[1] 
https://lore.kernel.org/lkml/8b751eb3-2e19-0e03-4c94-b26b3badd...@linaro.org/

-Doug