Re: [Intel-gfx] [PATCH 1/3] drm/i915/skl+: Don't trust cached ddb values

[Mahesh Kumar]

Hi,


On Saturday 27 May 2017 02:53 AM, Rodrigo Vivi wrote:

On Fri, May 26, 2017 at 8:15 AM, Mahesh Kumar  wrote:

Don't trust cached DDB values. Recalculate the ddb value if cached value
is zero.

If i915 is build as a module, there may be a race condition when
cursor_disable call comes even before intel_fbdev_initial_config.
Which may lead to cached value being 0. And further commit will fail
until a modeset.

Signed-off-by: Mahesh Kumar 
---
  drivers/gpu/drm/i915/intel_pm.c | 15 ++-
  1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 936eef1634c7..b67be1355e39 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3721,6 +3721,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 struct drm_i915_private *dev_priv = to_i915(dev);
 struct drm_crtc *for_crtc = cstate->base.crtc;
 unsigned int pipe_size, ddb_size;
+   unsigned int active_crtcs;
 int nth_active_pipe;

 if (WARN_ON(!state) || !cstate->base.active) {
@@ -3731,10 +3732,11 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device 
*dev,
 }

 if (intel_state->active_pipe_changes)
-   *num_active = hweight32(intel_state->active_crtcs);
+   active_crtcs = intel_state->active_crtcs;
 else
-   *num_active = hweight32(dev_priv->active_crtcs);
+   active_crtcs = dev_priv->active_crtcs;

+   *num_active = hweight32(active_crtcs);
 ddb_size = INTEL_INFO(dev_priv)->ddb_size;
 WARN_ON(ddb_size == 0);

@@ -3754,12 +3756,15 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device 
*dev,
  * copy from old state to be sure
  */
 *alloc = to_intel_crtc_state(for_crtc->state)->wm.skl.ddb;
-   return;
+   if (!skl_ddb_entry_size(alloc))
+   DRM_DEBUG_KMS("Cached pipe DDB is 0 recalculate\n");
+   else
+   return;

I see 2 different patches inside this patch here. One is this chunk
here that does what commit message tells.


 }

-   nth_active_pipe = hweight32(intel_state->active_crtcs &
+   nth_active_pipe = hweight32(active_crtcs &
 (drm_crtc_mask(for_crtc) - 1));
-   pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
+   pipe_size = ddb_size / hweight32(active_crtcs);

While this is fixing another bug where we were still using
intel_state->active_crtcs here even when we use dev_priv->active_crtcs
for num_active.

Thanks for review :)

This one is prerequisite for the first part. In earlier scenario, we 
will reach to this point only if (intel_state->active_pipe_changes) is true.
So it was ok to get num of active crtcs from 
"intel_state->active_crtcs". But after above change we may reach to this 
point even if
"intel_state->active_pipe_changes" is not true. So we should get the 
active_crtcs from correct struct.


Thats why I think this should be part of same patch. Please let me know 
if you still have different opinion.


thanks,
-Mahesh


Am I missing or misunderstanding something?


 alloc->start = nth_active_pipe * ddb_size / *num_active;
 alloc->end = alloc->start + pipe_size;
  }
--
2.11.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx





___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH RESEND] drm: i915: Don't try detecting sinks on ports already in use

[Gabriel Krisman Bertazi]

Chris Wilson  writes:

> The key problem here is say a race between DP unplug and HDMI plug, and
> users are evil enough (or common enough) for it to happen.
>
> I thought the idea was reasonable though, and perhaps we could make
> more use of the knowlege of the shared ports to improve detection of
> common DP/HDMI DDIs (i.e. run detection once for a ddi and have it
> decide whether it is hdmi or dp).

Hi Chris and Manasi, thanks for the feedback and sorry for the late
response.

I see your point about being racy.  I think this is not an issue in my
system since it appears that the disconnect of the first connector
triggers another scheduling of the hotplug_work, that further detects
the other connector.

I'm still working on this, but can you provide more details on your
suggestion about using more knowledge of the shared ports to improve the
detection of DDIs?  I still don't see how we could prevent this race
once and for all.

In the mean time, I wrote another patch with a different way to reduce
the overhead of attempting to detect connectors of shared ports already
in use.  My understanding is that this version is no longer racy but it
doesn't catch every condition where we could avoid detection.  Though,
it feels like something we want to get upstream.  Can you please take a
look and let me know?

I think this also addresses the cloned encoders issue raised by Manasi,
if I understood it correctly.

>8
Subject: [PATCH] drm: i915: Stop further detection on shared pins if a sink is
 connected

HPD pins may be shared among connectors but, according to documentation,
only one connector can be enebled at any given time.  This avoids
waisting time with attempting to detect further connectors on the same
pin if the first one was already detected.

Since this reduces the overhead of the i915_hotplug_work_func, it may
address the following vblank misses detected by the CI:

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100215
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100558

Signed-off-by: Gabriel Krisman Bertazi 
---
 drivers/gpu/drm/i915/intel_hotplug.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hotplug.c 
b/drivers/gpu/drm/i915/intel_hotplug.c
index f1200272a699..913e8523b89d 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -319,6 +319,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
struct drm_connector_list_iter conn_iter;
bool changed = false;
u32 hpd_event_bits;
+   u32 enc_hpd_pin_mask;
 
mutex_lock(>mode_config.mutex);
DRM_DEBUG_KMS("running encoder hotplug functions\n");
@@ -339,13 +340,18 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
if (!intel_connector->encoder)
continue;
intel_encoder = intel_connector->encoder;
-   if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
+   enc_hpd_pin_mask = (1 << intel_encoder->hpd_pin);
+   if (hpd_event_bits & enc_hpd_pin_mask) {
DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug 
event.\n",
  connector->name, intel_encoder->hpd_pin);
if (intel_encoder->hot_plug)
intel_encoder->hot_plug(intel_encoder);
-   if (intel_hpd_irq_event(dev, connector))
+   if (intel_hpd_irq_event(dev, connector)) {
changed = true;
+   if (connector->status ==
+   connector_status_connected)
+   hpd_event_bits &= ~(enc_hpd_pin_mask);
+   }
}
}
drm_connector_list_iter_end(_iter);
-- 
2.11.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm: i915: Preserve old FBC status for update without new planes

[Gabriel Krisman Bertazi]

Gabriel Krisman Bertazi  writes:

> If the atomic commit doesn't include any new plane, there is no need to
> choose a new CRTC for FBC, and the intel_fbc_choose_crtc() will bail out
> early.  Although, if the FBC setup failed in the previous commit, if the
> current commit doesn't include new plane update, it tries to overwrite
> no_fbc_reason to "no suitable CRTC for FBC".  For that scenario, it is
> better that we simply keep the old message in-place to make debugging
> easier.
>
> A scenario where this happens is with the
> igt@kms_frontbuffer_tracking@fbc-suspend testcase when executed on a
> Haswell system with not enough stolen memory.  When enabling the CRTC,
> the FBC setup will be correctly initialized to a specific CRTC, but
> won't be enabled, since there is not enough memory.  The testcase will
> then enable CRC checking, which requires a quirk for Haswell, which
> issues a new atomic commit that doesn't update the planes.  Since that
> update doesn't include any new planes (and the FBC wasn't enabled),
> intel_fbc_choose_crtc() will not find any suitable CRTC, but update the
> error message, hiding the lack of memory information, which is the
> actual cause of the initialization failure.  As a result, this causes
> that test, which should skip, to fail on Haswell.
>
> Changes since v1:
>   - Update commit message (Manasi)
>

Hi Manasi,

(Resending this message, cause I think it bounced)

Unless there are other concerns about this one, can you help me get it
pushed? I don't have the commit rights.

Thanks!

-- 
Gabriel Krisman Bertazi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm/i915: Fix false-positive assert_rpm_wakelock_held in i915_pmic_bus_access_notifier

[Hans de Goede]

Hi,

On 19-05-17 15:27, Hans de Goede wrote:

assert_rpm_wakelock_held is triggered from i915_pmic_bus_access_notifier
even though it gets unregistered on (runtime) suspend, this is caused
by a race happening under the following circumstances:

intel_runtime_pm_put does:

atomic_dec(_priv->pm.wakeref_count);

pm_runtime_mark_last_busy(kdev);
pm_runtime_put_autosuspend(kdev);

And pm_runtime_put_autosuspend calls intel_runtime_suspend from
a workqueue, so there is ample of time between the atomic_dec() and
intel_runtime_suspend() unregistering the notifier. If the notifier
gets called in this windowd assert_rpm_wakelock_held falsely triggers
(at this point we're not runtime-suspended yet).

This commit adds disable_rpm_wakeref_asserts and
enable_rpm_wakeref_asserts calls around the
intel_uncore_forcewake_get(FORCEWAKE_ALL) call in
i915_pmic_bus_access_notifier fixing the false-positive WARN_ON.

Reported-by: FKr 
Signed-off-by: Hans de Goede 


Ping, what is the status of this series?

I've been running this series on several Bay and Cherry Trail devices
since I posted this without issues.

As this fixes a couple of corner case wrt the pmic bus access
sharing merged for 4.12, it would be nice to get these
queued up and of possible added to a 4.12-rc#.

The only patch which changes code / functionality which was present
in 4.11 is the 3th patch, which I added because Ville requested the
change. The other 3 can be queued up for 4.12 without the 3th patch.

Regards,

Hans




---
  drivers/gpu/drm/i915/intel_uncore.c | 7 +++
  1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c 
b/drivers/gpu/drm/i915/intel_uncore.c
index 6d1ea26b2493..dc5e4f3e5a43 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1286,8 +1286,15 @@ static int i915_pmic_bus_access_notifier(struct 
notifier_block *nb,
 * bus, which will be busy after this notification, leading to:
 * "render: timed out waiting for forcewake ack request."
 * errors.
+*
+* This notifier may get called between intel_runtime_pm_put()
+* doing atomic_dec(wakeref_count) and intel_runtime_resume()
+* unregistering this notifier, which leads to false-positive
+* assert_rpm_wakelock_held() triggering.
 */
+   disable_rpm_wakeref_asserts(dev_priv);
intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
+   enable_rpm_wakeref_asserts(dev_priv);
break;
case MBI_PMIC_BUS_ACCESS_END:
intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PULL] drm-misc-fixes

[Lukas Wunner]

On Fri, May 26, 2017 at 08:36:45AM +0200, Daniel Vetter wrote:
> On Thu, May 25, 2017 at 7:44 PM, Sean Paul  wrote:
> > The pull is noisy because it includes -rc2.
> 
> dim has you covered for this, in case you've rolled forward but Dave
> hasn't yet, you can regenerate against linus upstream branch for a
> cleaner pull (but still warn Dave ofc):
> 
> $ dim pull-request drm-misc-next origin/master

Is it worth documenting this?  If so, below is a suggestion.
Feel free to rephrase as you see fit.

Thanks,

Lukas

-- >8 --
Subject: [PATCH] drm-misc: Document recommended base for -fixes pulls

Summarize the following discussion on dri-devel:

   "dim has you covered for this, in case you've rolled forward but Dave
hasn't yet, you can regenerate against linus upstream branch for a
cleaner pull (but still warn Dave ofc):
$ dim pull-request drm-misc-next origin/master" (Daniel)

   "FWIW this is what I've always done with drm-intel-fixes." (Jani)

   "As long as I'm warned in the pull request I often fast forward to
the base if my tree is clean, just to avoid the pull request having
noise in it." (Dave)

Cc: Daniel Vetter 
Cc: Jani Nikula 
Cc: Sean Paul 
Cc: Dave Airlie 
Signed-off-by: Lukas Wunner 
---
 drm-misc.rst | 8 
 1 file changed, 8 insertions(+)

diff --git a/drm-misc.rst b/drm-misc.rst
index d56c3c7..c66ac67 100644
--- a/drm-misc.rst
+++ b/drm-misc.rst
@@ -178,6 +178,14 @@ Maintainers mostly provide services to keep drm-misc 
running smoothly:
   keep it current. We try to avoid backmerges for bugfix branches, and rebasing
   isn't an option with multiple committers.
 
+* Pull requests become noisy if `-fixes` has been fast-forwarded to Linus'
+  latest -rc tag but drm-upstream hasn't done the same yet: The shortlog
+  will contain not just the queued fixes but also anything else that has
+  landed in Linus' tree in the meantime. The best practice is then to base
+  the pull request on Linus' master branch (rather than drm-upstream) by
+  setting the `upstream` argument for ``dim pull-request`` accordingly.
+  Upstream should be warned that they haven't fast-forwarded yet.
+
 * During the merge-windo blackout, i.e. from -rc6 on until the merge window
   closes with the release of -rc1, try to track `drm-next` with the
   `-next-fixes` branch. Do not advance past -rc1, otherwise the automagic in
-- 
2.11.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx