[Intel-gfx] Updated drm-intel-testing
Hi all, New -testing cycle with cool stuff: - select igt testing depencies for CONFIG_DRM_I915_DEBUG (Chris) - track outputs in crtc state and clean up all our ad-hoc connector/encoder walking in modest code (Ville) - demidlayer drm_device/drm_i915_private (Chris Wilson) - thundering herd fix from Chris Wilson, with lots of help from Tvrtko Ursulin - piles of assorted clean and fallout from the thundering herd fix - documentation and more tuning for waitboosting (Chris) - pooled EU support on bxt (Arun Siluvery) - bxt support is no longer considered prelimary! - ring/engine vfunc cleanup from Tvrtko - introduce intel_wait_for_register helper (Chris) - opregion updates (Jani Nukla) - tuning and fixes for wait_for macros (Tvrkto&Imre) - more kabylake pci ids (Rodrigo) - pps cleanup and fixes for bxt (Imre) - move sink crc support over to atomic state (Maarten) - fix up async fbdev init ordering (Chris) - fbc fixes from Paulo and Chris Happy testing! Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/17] drm/i915: Decouple GuC log setup from verbosity parameter
On 10/07/16 14:41, akash.g...@intel.com wrote:
From: Sagar Arun Kamble
GuC Log buffer allocation was tied up with verbosity level module param
i915.guc_log_level. User would be given a provision to enable firmware
logging at runtime, through a host2guc action, and not necessarily during
Driver load time. But the address of log buffer can be passed only in
init params, at firmware load time, so GuC has to be reset and firmware
needs to be reloaded to pass the log buffer address at runtime.
To avoid reset of GuC & reload of firmware, allocation of log buffer will
be done always but logging would be enabled initially on GuC side based on
the value of module paramter guc_log_level.
v2: Update commit message to describe the constraint with allocation of
log buffer at runtime. (Tvrtko)
Signed-off-by: Sagar Arun Kamble
Signed-off-by: Akash Goel
---
drivers/gpu/drm/i915/i915_guc_submission.c | 3 ---
drivers/gpu/drm/i915/intel_guc_loader.c| 8 +---
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 2112e02..8a9a0cb 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -832,9 +832,6 @@ static void guc_create_log(struct intel_guc *guc)
unsigned long offset;
uint32_t size, flags;
- if (i915.guc_log_level < GUC_LOG_VERBOSITY_MIN)
- return;
-
if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
b/drivers/gpu/drm/i915/intel_guc_loader.c
index 605c696..b211bd0 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -175,11 +175,13 @@ static void set_guc_init_params(struct drm_i915_private
*dev_priv)
params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
GUC_CTL_VCS2_ENABLED;
- if (i915.guc_log_level >= 0) {
- params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
+ params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
guc->log_flags will be zero when logging is not configured because guc
is a part of dev_priv. So it looks safe - although I reckon it would be
clearer to set this (GUC_CTL_LOG_PARAMS) explicitly inside the if-else
below?
+
+ if (i915.guc_log_level >= 0)
params[GUC_CTL_DEBUG] =
i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
- }
+ else
+ params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
I also wonder how come GUC_LOG_DISABLED isn't set today when
i915.guc_log_level == -1, given that:
#define GUC_LOG_DISABLED (1 << 6)
Is that bit set by default somehow if i915 does not program it?
if (guc->ads_obj) {
u32 ads = (u32)i915_gem_obj_ggtt_offset(guc->ads_obj)
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/17] drm/i915: Add low level set of routines for programming PM IER/IIR/IMR register set
On 10/07/16 14:41, akash.g...@intel.com wrote:
From: Akash Goel
So far PM IER/IIR/IMR registers were being used only for Turbo related
interrupts. But interrupts coming from GuC also use the same set.
As a precursor to supporting GuC interrupts, added new low level routines
so as to allow sharing the programming of PM IER/IIR/IMR registers between
Turbo & GuC.
Also similar to PM IMR, maintaining a bitmask for PM IER register, to allow
easy sharing of it between Turbo & GuC without involving a rmw operation.
v2:
- For appropriateness & avoid any ambiguity, rename old functions
enable/disable pm_irq to mask/unmask pm_irq and rename new functions
enable/disable pm_interrupts to enable/disable pm_irq. (Tvrtko)
- Use u32 in place of uint32_t. (Tvrtko)
v3:
- Rename the fields pm_irq_mask & pm_ier_mask and do some cleanup. (Chris)
- Rebase.
Suggested-by: Chris Wilson
Signed-off-by: Akash Goel
---
drivers/gpu/drm/i915/i915_drv.h | 3 +-
drivers/gpu/drm/i915/i915_irq.c | 71 ++---
drivers/gpu/drm/i915/intel_drv.h| 3 ++
drivers/gpu/drm/i915/intel_ringbuffer.c | 4 +-
4 files changed, 54 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4478cc8..c3a579f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1791,7 +1791,8 @@ struct drm_i915_private {
u32 de_irq_mask[I915_MAX_PIPES];
};
u32 gt_irq_mask;
- u32 pm_irq_mask;
+ u32 pm_imr;
+ u32 pm_ier;
u32 pm_rps_events;
u32 pipestat_irq_mask[I915_MAX_PIPES];
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1c2aec3..24bbaf7 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -303,18 +303,18 @@ static void snb_update_pm_irq(struct drm_i915_private
*dev_priv,
assert_spin_locked(&dev_priv->irq_lock);
- new_val = dev_priv->pm_irq_mask;
+ new_val = dev_priv->pm_imr;
new_val &= ~interrupt_mask;
new_val |= (~enabled_irq_mask & interrupt_mask);
- if (new_val != dev_priv->pm_irq_mask) {
- dev_priv->pm_irq_mask = new_val;
- I915_WRITE(gen6_pm_imr(dev_priv), dev_priv->pm_irq_mask);
+ if (new_val != dev_priv->pm_imr) {
+ dev_priv->pm_imr = new_val;
+ I915_WRITE(gen6_pm_imr(dev_priv), dev_priv->pm_imr);
POSTING_READ(gen6_pm_imr(dev_priv));
}
}
-void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
+void gen6_unmask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
{
if (WARN_ON(!intel_irqs_enabled(dev_priv)))
return;
@@ -322,28 +322,54 @@ void gen6_enable_pm_irq(struct drm_i915_private
*dev_priv, uint32_t mask)
snb_update_pm_irq(dev_priv, mask, mask);
}
-static void __gen6_disable_pm_irq(struct drm_i915_private *dev_priv,
- uint32_t mask)
+static void __gen6_mask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
{
snb_update_pm_irq(dev_priv, mask, 0);
}
-void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
+void gen6_mask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
{
if (WARN_ON(!intel_irqs_enabled(dev_priv)))
return;
- __gen6_disable_pm_irq(dev_priv, mask);
+ __gen6_mask_pm_irq(dev_priv, mask);
}
-void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
+void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32 reset_mask)
{
i915_reg_t reg = gen6_pm_iir(dev_priv);
- spin_lock_irq(&dev_priv->irq_lock);
- I915_WRITE(reg, dev_priv->pm_rps_events);
- I915_WRITE(reg, dev_priv->pm_rps_events);
+ assert_spin_locked(&dev_priv->irq_lock);
+
+ I915_WRITE(reg, reset_mask);
+ I915_WRITE(reg, reset_mask);
POSTING_READ(reg);
+}
+
+void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, u32 enable_mask)
+{
+ assert_spin_locked(&dev_priv->irq_lock);
+
+ dev_priv->pm_ier |= enable_mask;
+ I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier);
+ gen6_unmask_pm_irq(dev_priv, enable_mask);
+ /* unmask_pm_irq provides an implicit barrier (POSTING_READ) */
+}
+
+void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, u32 disable_mask)
+{
+ assert_spin_locked(&dev_priv->irq_lock);
+
+ dev_priv->pm_ier &= ~disable_mask;
+ __gen6_mask_pm_irq(dev_priv, disable_mask);
+ I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier);
+ /* though a barrier is missing here, but don't really need a one */
+}
+
+void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
+{
+ spin_lock_irq(&dev_priv->irq_lock);
+ gen6_reset_pm_iir(dev_priv, dev_priv->pm_rps_events);
dev_priv->rps.pm_iir = 0;
spin_unlock_irq(&dev_priv->irq_lock);
}
@@ -354,8 +3
Re: [Intel-gfx] [PATCH 05/17] drm/i915: Support for GuC interrupts
On 10/07/16 14:41, akash.g...@intel.com wrote:
From: Sagar Arun Kamble
There are certain types of interrupts which Host can recieve from GuC.
GuC ukernel sends an interrupt to Host for certain events, like for
example retrieve/consume the logs generated by ukernel.
This patch adds support to receive interrupts from GuC but currently
enables & partially handles only the interrupt sent by GuC ukernel.
Future patches will add support for handling other interrupt types.
v2:
- Use common low level routines for PM IER/IIR programming (Chris)
- Rename interrupt functions to gen9_xxx from gen8_xxx (Chris)
- Replace disabling of wake ref asserts with rpm get/put (Chris)
v3:
- Update comments for more clarity. (Tvrtko)
- Remove the masking of GuC interrupt, which was kept masked till the
start of bottom half, its not really needed as there is only a
single instance of work item & wq is ordered. (Tvrtko)
v4:
- Rebase.
- Rename guc_events to pm_guc_events so as to be indicative of the
register/control block it is associated with. (Chris)
- Add handling for back to back log buffer flush interrupts.
Signed-off-by: Sagar Arun Kamble
Signed-off-by: Akash Goel
---
drivers/gpu/drm/i915/i915_drv.h| 1 +
drivers/gpu/drm/i915/i915_guc_submission.c | 5 ++
drivers/gpu/drm/i915/i915_irq.c| 98 --
drivers/gpu/drm/i915/i915_reg.h| 11
drivers/gpu/drm/i915/intel_drv.h | 3 +
drivers/gpu/drm/i915/intel_guc.h | 4 ++
drivers/gpu/drm/i915/intel_guc_loader.c| 4 ++
7 files changed, 122 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c3a579f..6e2ddfa 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1794,6 +1794,7 @@ struct drm_i915_private {
u32 pm_imr;
u32 pm_ier;
u32 pm_rps_events;
+ u32 pm_guc_events;
u32 pipestat_irq_mask[I915_MAX_PIPES];
struct i915_hotplug hotplug;
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 0fb00ab..0bac172 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1044,6 +1044,8 @@ int intel_guc_suspend(struct drm_device *dev)
if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
return 0;
+ gen9_disable_guc_interrupts(dev_priv);
+
ctx = dev_priv->kernel_context;
data[0] = HOST2GUC_ACTION_ENTER_S_STATE;
@@ -1070,6 +1072,9 @@ int intel_guc_resume(struct drm_device *dev)
if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
return 0;
+ if (i915.guc_log_level >= 0)
+ gen9_enable_guc_interrupts(dev_priv);
+
ctx = dev_priv->kernel_context;
data[0] = HOST2GUC_ACTION_EXIT_S_STATE;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 24bbaf7..fd73c94 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -170,6 +170,7 @@ static void gen5_assert_iir_is_zero(struct drm_i915_private
*dev_priv,
} while (0)
static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32
pm_iir);
+static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32
pm_iir);
/* For display hotplug interrupt */
static inline void
@@ -411,6 +412,39 @@ void gen6_disable_rps_interrupts(struct drm_i915_private
*dev_priv)
gen6_reset_rps_interrupts(dev_priv);
}
+void gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv)
+{
+ spin_lock_irq(&dev_priv->irq_lock);
+ gen6_reset_pm_iir(dev_priv, dev_priv->pm_guc_events);
+ spin_unlock_irq(&dev_priv->irq_lock);
+}
+
+void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv)
+{
+ spin_lock_irq(&dev_priv->irq_lock);
+ if (!dev_priv->guc.interrupts_enabled) {
+ WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
+ dev_priv->pm_guc_events);
+ dev_priv->guc.interrupts_enabled = true;
+ gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
+ }
+ spin_unlock_irq(&dev_priv->irq_lock);
+}
+
+void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv)
+{
+ spin_lock_irq(&dev_priv->irq_lock);
+ dev_priv->guc.interrupts_enabled = false;
+
+ gen6_disable_pm_irq(dev_priv, dev_priv->pm_guc_events);
+
+ spin_unlock_irq(&dev_priv->irq_lock);
+ synchronize_irq(dev_priv->drm.irq);
+
+ cancel_work_sync(&dev_priv->guc.events_work);
+ gen9_reset_guc_interrupts(dev_priv);
+}
+
/**
* bdw_update_port_irq - update DE port interrupt
* @dev_priv: driver private
@@ -1174,6 +1208,21 @@ static void gen6_pm_rps_work(struct work_struct *work)
mutex_unlock(&dev_priv->rps.hw_lock);
}
+static void gen9_guc2host_events_work(struct w
Re: [Intel-gfx] [PATCH] drm/i915/dmc: Accept symbolic link in firmware name
On to, 2016-07-07 at 17:57 +0300, Mika Kuoppala wrote:
> "Vivi, Rodrigo" writes:
>
> > Nak.
> >
> > I don't intend to update the symbolic links on linux-firmware.git
> > repository anymore so if we receive a new minor version update we
> > are
> > not going to load.
> >
> > I was the one advocating in the favor for the symbolic link
> > flexibility
> > but I lost the discussions for the stability and validation etc.
> >
>
> And I was one advocating in favor of getting rid of symlink. But the
> filename versioning is superfluous as the contents has the version
> info
> which we can solely rely to not run something we dont want.
>
> So I am not sure what we lose in stability and validation front
> with the strict version check.
Bisection is more cumbersome with a symlink.
--Imre
> -Mika
>
> > So let's just move away from symbolic link for good.
> >
> >
> > On Tue, 2016-07-05 at 12:25 +0300, Mika Kuoppala wrote:
> > > We need the ability to explicitly load only a specified firmware
> > > version. As the firmware blob contains the version, we use
> > > that to filter out the ones we don't want. The version encoded
> > > into the firmware name is superfluous and we should allow user
> > > to point into specific firmware through a symlink, and only do
> > > filtering based on the version stamp included in the blob.
> > > This allows user to conveniently point to a firmware blob and
> > > still gives us the control of what we decided to run on.
> > >
> > > This is partial revert of
> > > 4aa7fb9c3c4f ("drm/i915/dmc: Step away from symbolic links")
> > >
> > > Fixes: 4aa7fb9c3c4f ("drm/i915/dmc: Step away from symbolic
> > > links")
> > > References: https://bugs.freedesktop.org/show_bug.cgi?id=96800
> > > Reported-by: Mike Lothian
> > > Cc: Rodrigo Vivi
> > > Cc: Imre Deak
> > > Cc: Mika Kuoppala
> > > Cc: Patrik Jakobsson
> > > Cc: Chris Wilson
> > > Signed-off-by: Mika Kuoppala
> > > ---
> > > drivers/gpu/drm/i915/intel_csr.c | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_csr.c
> > > b/drivers/gpu/drm/i915/intel_csr.c
> > > index ea047cd46b71..77d01a0b64b4 100644
> > > --- a/drivers/gpu/drm/i915/intel_csr.c
> > > +++ b/drivers/gpu/drm/i915/intel_csr.c
> > > @@ -41,15 +41,15 @@
> > > * be moved to FW_FAILED.
> > > */
> > >
> > > -#define I915_CSR_KBL "i915/kbl_dmc_ver1_01.bin"
> > > +#define I915_CSR_KBL "i915/kbl_dmc_ver1.bin"
> > > MODULE_FIRMWARE(I915_CSR_KBL);
> > > #define KBL_CSR_VERSION_REQUIRED CSR_VERSION(1, 1)
> > >
> > > -#define I915_CSR_SKL "i915/skl_dmc_ver1_26.bin"
> > > +#define I915_CSR_SKL "i915/skl_dmc_ver1.bin"
> > > MODULE_FIRMWARE(I915_CSR_SKL);
> > > #define SKL_CSR_VERSION_REQUIRED CSR_VERSION(1, 26)
> > >
> > > -#define I915_CSR_BXT "i915/bxt_dmc_ver1_07.bin"
> > > +#define I915_CSR_BXT "i915/bxt_dmc_ver1.bin"
> > > MODULE_FIRMWARE(I915_CSR_BXT);
> > > #define BXT_CSR_VERSION_REQUIRED CSR_VERSION(1, 7)
> > >
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/10] drm/i915: Define a separate variable and control for RPS waitboost frequency
Chris Wilson writes:
> To allow the user finer control over waitboosting, allow them to set the
> frequency we request for the boost. This also them allows to effectively
> disable the boosting by setting the boost request to a low frequency.
>
> Signed-off-by: Chris Wilson
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 2 ++
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_irq.c | 9 +
> drivers/gpu/drm/i915/i915_sysfs.c | 38
> +
> drivers/gpu/drm/i915/intel_pm.c | 5 -
> 5 files changed, 50 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 844fea795bae..d1ff4cb9b90e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1381,6 +1381,8 @@ static int i915_frequency_info(struct seq_file *m, void
> *unused)
> intel_gpu_freq(dev_priv, dev_priv->rps.idle_freq));
> seq_printf(m, "Min freq: %d MHz\n",
> intel_gpu_freq(dev_priv, dev_priv->rps.min_freq));
> + seq_printf(m, "Boost freq: %d MHz\n",
> +intel_gpu_freq(dev_priv, dev_priv->rps.boost_freq));
> seq_printf(m, "Max freq: %d MHz\n",
> intel_gpu_freq(dev_priv, dev_priv->rps.max_freq));
> seq_printf(m,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4478cc852489..bb59bc637f7b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1170,6 +1170,7 @@ struct intel_gen6_power_mgmt {
> u8 max_freq_softlimit; /* Max frequency permitted by the driver */
> u8 max_freq;/* Maximum frequency, RP0 if not overclocking */
> u8 min_freq;/* AKA RPn. Minimum frequency */
> + u8 boost_freq; /* Frequency to request when wait boosting */
> u8 idle_freq; /* Frequency to request when we are idle */
> u8 efficient_freq; /* AKA RPe. Pre-determined balanced frequency */
> u8 rp1_freq;/* "less than" RP0 power/freqency */
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 1c2aec392412..c8ed36765301 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1105,9 +1105,10 @@ static void gen6_pm_rps_work(struct work_struct *work)
> new_delay = dev_priv->rps.cur_freq;
> min = dev_priv->rps.min_freq_softlimit;
> max = dev_priv->rps.max_freq_softlimit;
> -
> - if (client_boost) {
> - new_delay = dev_priv->rps.max_freq_softlimit;
> + if (client_boost || any_waiters(dev_priv))
> + max = dev_priv->rps.max_freq;
> + if (client_boost && new_delay < dev_priv->rps.boost_freq) {
> + new_delay = dev_priv->rps.boost_freq;
What should be the rules for boost_freq? With this change it can
be over max_freq_softlimit and thus will break pm_rps.
-Mika
> adj = 0;
> } else if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
> if (adj > 0)
> @@ -1122,7 +1123,7 @@ static void gen6_pm_rps_work(struct work_struct *work)
> new_delay = dev_priv->rps.efficient_freq;
> adj = 0;
> }
> - } else if (any_waiters(dev_priv)) {
> + } else if (client_boost || any_waiters(dev_priv)) {
> adj = 0;
> } else if (pm_iir & GEN6_PM_RP_DOWN_TIMEOUT) {
> if (dev_priv->rps.cur_freq > dev_priv->rps.efficient_freq)
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c
> b/drivers/gpu/drm/i915/i915_sysfs.c
> index d61829e54f93..8c045ff47f0e 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -318,6 +318,41 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
> return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> }
>
> +static ssize_t gt_boost_freq_mhz_show(struct device *kdev, struct
> device_attribute *attr, char *buf)
> +{
> + struct drm_minor *minor = dev_to_drm_minor(kdev);
> + struct drm_i915_private *dev_priv = to_i915(minor->dev);
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n",
> + intel_gpu_freq(dev_priv, dev_priv->rps.boost_freq));
> +}
> +
> +static ssize_t gt_boost_freq_mhz_store(struct device *kdev,
> +struct device_attribute *attr,
> +const char *buf, size_t count)
> +{
> + struct drm_minor *minor = dev_to_drm_minor(kdev);
> + struct drm_device *dev = minor->dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + u32 val;
> + ssize_t ret;
> +
> + ret = kstrtou32(buf, 0, &val);
> + if (ret)
> + return ret;
> +
> + /* Validate against (static) hardware limits */
> + val = intel_freq_opcode(dev_priv, val);
> + if
Re: [Intel-gfx] [PATCH 01/17] drm/i915: Decouple GuC log setup from verbosity parameter
On 7/11/2016 3:07 PM, Tvrtko Ursulin wrote:
On 10/07/16 14:41, akash.g...@intel.com wrote:
From: Sagar Arun Kamble
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 2112e02..8a9a0cb 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -832,9 +832,6 @@ static void guc_create_log(struct intel_guc *guc)
unsigned long offset;
uint32_t size, flags;
-if (i915.guc_log_level < GUC_LOG_VERBOSITY_MIN)
-return;
-
if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
b/drivers/gpu/drm/i915/intel_guc_loader.c
index 605c696..b211bd0 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -175,11 +175,13 @@ static void set_guc_init_params(struct
drm_i915_private *dev_priv)
params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
GUC_CTL_VCS2_ENABLED;
-if (i915.guc_log_level >= 0) {
-params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
+params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
guc->log_flags will be zero when logging is not configured because guc
is a part of dev_priv. So it looks safe - although I reckon it would be
clearer to set this (GUC_CTL_LOG_PARAMS) explicitly inside the if-else
below?
If logging is not enabled at (due to guc_log_level < 0), then also
log_flags needs to be setup & passed to GuC firmware.
log_flags shall not be zero even when logging is not be enabled (at boot
time).
Actually log_flags will also contain the address of the log buffer.
+
+if (i915.guc_log_level >= 0)
params[GUC_CTL_DEBUG] =
i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
-}
+else
+params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
I also wonder how come GUC_LOG_DISABLED isn't set today when
i915.guc_log_level == -1, given that:
#define GUC_LOG_DISABLED (1 << 6)
Is that bit set by default somehow if i915 does not program it?
Yes currently GUC_LOG_DISABLED won't get set for guc_log_level = -1.
But then log buffer address will go as NULL and GUC_LOG_VALID flag will
go as 0, for guc_log_level = -1. So this way logging on GuC side will
not get enabled.
I hope I understood your concern correctly.
Best regards
Akash
if (guc->ads_obj) {
u32 ads = (u32)i915_gem_obj_ggtt_offset(guc->ads_obj)
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/10] drm/i915/breadcrumbs: Queue hangcheck before sleeping
Chris Wilson writes:
> Never go to sleep waiting on the GPU without first ensuring that we will
> get woken up.
>
> We have a choice of queuing the hangcheck before every schedule() or the
> first time we wakeup. In order to simply accommodate both the signaler
> and the ordinary waiter, move the queuing to the common point of
> enabling the irq. We lose the paranoid safety of ensuring that the
> hangcheck is active before the sleep, but avoid code duplication (and
> redundant hangcheck queuing).
>
> Testcase: igt/prime_busy
> Fixes: c81d46138da6 ("drm/i915: Convert trace-irq to the breadcrumb waiter")
> Signed-off-by: Chris Wilson
> Cc: Tvrtko Ursulin
Reviewed-by: Mika Kuoppala
> ---
> drivers/gpu/drm/i915/i915_gem.c | 9 -
> drivers/gpu/drm/i915/intel_breadcrumbs.c | 9 +
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 8f50919ba9b4..7fd44980798f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1501,15 +1501,6 @@ int __i915_wait_request(struct drm_i915_gem_request
> *req,
> break;
> }
>
> - /* Ensure that even if the GPU hangs, we get woken up.
> - *
> - * However, note that if no one is waiting, we never notice
> - * a gpu hang. Eventually, we will have to wait for a resource
> - * held by the GPU and so trigger a hangcheck. In the most
> - * pathological case, this will be upon memory starvation!
> - */
> - i915_queue_hangcheck(req->i915);
> -
> timeout_remain = io_schedule_timeout(timeout_remain);
> if (timeout_remain == 0) {
> ret = -ETIME;
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index d89b2c963618..b074f3d6d127 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -93,6 +93,15 @@ static void __intel_breadcrumbs_enable_irq(struct
> intel_breadcrumbs *b)
> if (!b->irq_enabled ||
> test_bit(engine->id, &i915->gpu_error.missed_irq_rings))
> mod_timer(&b->fake_irq, jiffies + 1);
> +
> + /* Ensure that even if the GPU hangs, we get woken up.
> + *
> + * However, note that if no one is waiting, we never notice
> + * a gpu hang. Eventually, we will have to wait for a resource
> + * held by the GPU and so trigger a hangcheck. In the most
> + * pathological case, this will be upon memory starvation!
> + */
> + i915_queue_hangcheck(i915);
> }
>
> static void __intel_breadcrumbs_disable_irq(struct intel_breadcrumbs *b)
> --
> 2.8.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/10] drm/i915: Define a separate variable and control for RPS waitboost frequency
On Mon, Jul 11, 2016 at 02:39:19PM +0300, Mika Kuoppala wrote: > What should be the rules for boost_freq? With this change it can > be over max_freq_softlimit and thus will break pm_rps. The point is to be an independent variable that the user can set in addition to max freq, just like the cpu turbo. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/17] drm/i915: Decouple GuC log setup from verbosity parameter
On 11/07/16 12:41, Goel, Akash wrote:
On 7/11/2016 3:07 PM, Tvrtko Ursulin wrote:
On 10/07/16 14:41, akash.g...@intel.com wrote:
From: Sagar Arun Kamble
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 2112e02..8a9a0cb 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -832,9 +832,6 @@ static void guc_create_log(struct intel_guc *guc)
unsigned long offset;
uint32_t size, flags;
-if (i915.guc_log_level < GUC_LOG_VERBOSITY_MIN)
-return;
-
if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
b/drivers/gpu/drm/i915/intel_guc_loader.c
index 605c696..b211bd0 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -175,11 +175,13 @@ static void set_guc_init_params(struct
drm_i915_private *dev_priv)
params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
GUC_CTL_VCS2_ENABLED;
-if (i915.guc_log_level >= 0) {
-params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
+params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
guc->log_flags will be zero when logging is not configured because guc
is a part of dev_priv. So it looks safe - although I reckon it would be
clearer to set this (GUC_CTL_LOG_PARAMS) explicitly inside the if-else
below?
If logging is not enabled at (due to guc_log_level < 0), then also
log_flags needs to be setup & passed to GuC firmware.
log_flags shall not be zero even when logging is not be enabled (at boot
time).
Actually log_flags will also contain the address of the log buffer.
Ah yes, I got confused by jumping between one file with your patch
applied and one without it.
+
+if (i915.guc_log_level >= 0)
params[GUC_CTL_DEBUG] =
i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
-}
+else
+params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
I also wonder how come GUC_LOG_DISABLED isn't set today when
i915.guc_log_level == -1, given that:
#define GUC_LOG_DISABLED (1 << 6)
Is that bit set by default somehow if i915 does not program it?
Yes currently GUC_LOG_DISABLED won't get set for guc_log_level = -1.
But then log buffer address will go as NULL and GUC_LOG_VALID flag will
go as 0, for guc_log_level = -1. So this way logging on GuC side will
not get enabled.
I hope I understood your concern correctly.
Yes, this clarifies it. Although I do have one more question then - what
happens if at boot i915.guc_log_level == -1 and then with later patches
logging gets enabled via debugfs - who and how sets
params[GUC_CTL_DEBUG]? Host2GuC overrides this parameter?
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/17] drm/i915: Decouple GuC log setup from verbosity parameter
On 7/11/2016 5:20 PM, Tvrtko Ursulin wrote:
On 11/07/16 12:41, Goel, Akash wrote:
On 7/11/2016 3:07 PM, Tvrtko Ursulin wrote:
On 10/07/16 14:41, akash.g...@intel.com wrote:
From: Sagar Arun Kamble
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 2112e02..8a9a0cb 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
b/drivers/gpu/drm/i915/intel_guc_loader.c
index 605c696..b211bd0 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -175,11 +175,13 @@ static void set_guc_init_params(struct
drm_i915_private *dev_priv)
params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
GUC_CTL_VCS2_ENABLED;
-if (i915.guc_log_level >= 0) {
-params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
+params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
guc->log_flags will be zero when logging is not configured because guc
is a part of dev_priv. So it looks safe - although I reckon it would be
clearer to set this (GUC_CTL_LOG_PARAMS) explicitly inside the if-else
below?
If logging is not enabled at (due to guc_log_level < 0), then also
log_flags needs to be setup & passed to GuC firmware.
log_flags shall not be zero even when logging is not be enabled (at boot
time).
Actually log_flags will also contain the address of the log buffer.
Ah yes, I got confused by jumping between one file with your patch
applied and one without it.
+
+if (i915.guc_log_level >= 0)
params[GUC_CTL_DEBUG] =
i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
-}
+else
+params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
I also wonder how come GUC_LOG_DISABLED isn't set today when
i915.guc_log_level == -1, given that:
#define GUC_LOG_DISABLED (1 << 6)
Is that bit set by default somehow if i915 does not program it?
Yes currently GUC_LOG_DISABLED won't get set for guc_log_level = -1.
But then log buffer address will go as NULL and GUC_LOG_VALID flag will
go as 0, for guc_log_level = -1. So this way logging on GuC side will
not get enabled.
I hope I understood your concern correctly.
Yes, this clarifies it. Although I do have one more question then - what
happens if at boot i915.guc_log_level == -1 and then with later patches
logging gets enabled via debugfs - who and how sets
params[GUC_CTL_DEBUG]? Host2GuC overrides this parameter?
Yes through Host2GuC action type, UK_LOG_ENABLE_LOGGING, Host will
request GuC firmware to enable/disable logging and alter the verbosity
level.
The params[GUC_CTL_DEBUG] is just part of the firmware initialization
parameters and is not used after that.
Best regards
Akash
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/10] drm/i915: Kick hangcheck from retire worker
Chris Wilson writes:
> Let's ensure that we cannot run indefinitely without the hangcheck
> worker being queued. We removed it from being kicked on every request
> because we were kicking it a few millions times in every hangcheck
> interval and only once is necessary! However, that leaves us with the
> issue of what if userspace never waits for a request, or runs out of
> resources, what if userspace just issues a request then spins on
> BUSY_IOCTL?
>
> Testcase: igt/gem_busy
> Signed-off-by: Chris Wilson
> Cc: Tvrtko Ursulin
Reviewed-by: Mika Kuoppala
> ---
> drivers/gpu/drm/i915/i915_gem.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 7fd44980798f..adeca0ec4cfb 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3281,10 +3281,12 @@ i915_gem_retire_work_handler(struct work_struct *work)
>* We do not need to do this test under locking as in the worst-case
>* we queue the retire worker once too often.
>*/
> - if (READ_ONCE(dev_priv->gt.awake))
> + if (READ_ONCE(dev_priv->gt.awake)) {
> + i915_queue_hangcheck(dev_priv);
> queue_delayed_work(dev_priv->wq,
> &dev_priv->gt.retire_work,
> round_jiffies_up_relative(HZ));
> + }
> }
>
> static void
> --
> 2.8.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/dmc: Accept symbolic link in firmware name
On Mon, Jul 11, 2016 at 02:23:48PM +0300, Imre Deak wrote: > On to, 2016-07-07 at 17:57 +0300, Mika Kuoppala wrote: > > "Vivi, Rodrigo" writes: > > > > > Nak. > > > > > > I don't intend to update the symbolic links on linux-firmware.git > > > repository anymore so if we receive a new minor version update we > > > are > > > not going to load. > > > > > > I was the one advocating in the favor for the symbolic link > > > flexibility > > > but I lost the discussions for the stability and validation etc. > > > > > > > And I was one advocating in favor of getting rid of symlink. But the > > filename versioning is superfluous as the contents has the version > > info > > which we can solely rely to not run something we dont want. > > > > So I am not sure what we lose in stability and validation front > > with the strict version check. > > Bisection is more cumbersome with a symlink. Did you miss a without there? Because when bisecting the kernel it's harder without the symlink as the build breaks otherwise and the runtime is not bisectable either. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/dmc: Accept symbolic link in firmware name
On ma, 2016-07-11 at 13:39 +0100, ch...@chris-wilson.co.uk wrote: > On Mon, Jul 11, 2016 at 02:23:48PM +0300, Imre Deak wrote: > > On to, 2016-07-07 at 17:57 +0300, Mika Kuoppala wrote: > > > "Vivi, Rodrigo" writes: > > > > > > > Nak. > > > > > > > > I don't intend to update the symbolic links on linux- > > > > firmware.git > > > > repository anymore so if we receive a new minor version update > > > > we > > > > are > > > > not going to load. > > > > > > > > I was the one advocating in the favor for the symbolic link > > > > flexibility > > > > but I lost the discussions for the stability and validation > > > > etc. > > > > > > > > > > And I was one advocating in favor of getting rid of symlink. But > > > the > > > filename versioning is superfluous as the contents has the > > > version > > > info > > > which we can solely rely to not run something we dont want. > > > > > > So I am not sure what we lose in stability and validation front > > > with the strict version check. > > > > Bisection is more cumbersome with a symlink. > > Did you miss a without there? Because when bisecting the kernel it's > harder without the symlink as the build breaks otherwise and the > runtime > is not bisectable either. No, I meant when bisecting we also want to load the proper fw version for each bisect commit point. So using exact filenames we'll automatically load the proper firmware file for a given commit, whereas with symlinks we have to adjust the symlink at each bisect step. You need to have all the required FW versions for the bisect range to be present on the filesystem of course. --Imre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/10] drm/i915: Kick hangcheck from retire worker
On Mon, Jul 11, 2016 at 03:21:37PM +0300, Mika Kuoppala wrote: > Chris Wilson writes: > > > Let's ensure that we cannot run indefinitely without the hangcheck > > worker being queued. We removed it from being kicked on every request > > because we were kicking it a few millions times in every hangcheck > > interval and only once is necessary! However, that leaves us with the > > issue of what if userspace never waits for a request, or runs out of > > resources, what if userspace just issues a request then spins on > > BUSY_IOCTL? > > > > Testcase: igt/gem_busy > > Signed-off-by: Chris Wilson > > Cc: Tvrtko Ursulin > > Reviewed-by: Mika Kuoppala Ta, I'm going to push this pair as they fix a couple of (new) igt. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/10] drm/i915: Define a separate variable and control for RPS waitboost frequency
Chris Wilson writes: > On Mon, Jul 11, 2016 at 02:39:19PM +0300, Mika Kuoppala wrote: >> What should be the rules for boost_freq? With this change it can >> be over max_freq_softlimit and thus will break pm_rps. > > The point is to be an independent variable that the user can set in > addition to max freq, just like the cpu turbo. But can it be over max_freq? If so, pm_rps needs tweaking. Also, noticed that this is not set up in gen6_init_rps_frequencies() -Mika > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/dmc: Accept symbolic link in firmware name
On Mon, Jul 11, 2016 at 03:45:21PM +0300, Imre Deak wrote: > On ma, 2016-07-11 at 13:39 +0100, ch...@chris-wilson.co.uk wrote: > > On Mon, Jul 11, 2016 at 02:23:48PM +0300, Imre Deak wrote: > > > On to, 2016-07-07 at 17:57 +0300, Mika Kuoppala wrote: > > > > "Vivi, Rodrigo" writes: > > > > > > > > > Nak. > > > > > > > > > > I don't intend to update the symbolic links on linux- > > > > > firmware.git > > > > > repository anymore so if we receive a new minor version update > > > > > we > > > > > are > > > > > not going to load. > > > > > > > > > > I was the one advocating in the favor for the symbolic link > > > > > flexibility > > > > > but I lost the discussions for the stability and validation > > > > > etc. > > > > > > > > > > > > > And I was one advocating in favor of getting rid of symlink. But > > > > the > > > > filename versioning is superfluous as the contents has the > > > > version > > > > info > > > > which we can solely rely to not run something we dont want. > > > > > > > > So I am not sure what we lose in stability and validation front > > > > with the strict version check. > > > > > > Bisection is more cumbersome with a symlink. > > > > Did you miss a without there? Because when bisecting the kernel it's > > harder without the symlink as the build breaks otherwise and the > > runtime > > is not bisectable either. > > No, I meant when bisecting we also want to load the proper fw version > for each bisect commit point. So using exact filenames we'll > automatically load the proper firmware file for a given commit, whereas > with symlinks we have to adjust the symlink at each bisect step. You mean you have to adjust the kernel build at each point. That is the root of the problem as we do not have deferred firmware loading. And then you get random changes in the firmare whilst bisecting the kernel. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/17] drm/i915: Decouple GuC log setup from verbosity parameter
On 11/07/16 13:11, Goel, Akash wrote:
On 7/11/2016 5:20 PM, Tvrtko Ursulin wrote:
On 11/07/16 12:41, Goel, Akash wrote:
On 7/11/2016 3:07 PM, Tvrtko Ursulin wrote:
On 10/07/16 14:41, akash.g...@intel.com wrote:
From: Sagar Arun Kamble
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 2112e02..8a9a0cb 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
b/drivers/gpu/drm/i915/intel_guc_loader.c
index 605c696..b211bd0 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -175,11 +175,13 @@ static void set_guc_init_params(struct
drm_i915_private *dev_priv)
params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
GUC_CTL_VCS2_ENABLED;
-if (i915.guc_log_level >= 0) {
-params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
+params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
guc->log_flags will be zero when logging is not configured because guc
is a part of dev_priv. So it looks safe - although I reckon it would be
clearer to set this (GUC_CTL_LOG_PARAMS) explicitly inside the if-else
below?
If logging is not enabled at (due to guc_log_level < 0), then also
log_flags needs to be setup & passed to GuC firmware.
log_flags shall not be zero even when logging is not be enabled (at boot
time).
Actually log_flags will also contain the address of the log buffer.
Ah yes, I got confused by jumping between one file with your patch
applied and one without it.
+
+if (i915.guc_log_level >= 0)
params[GUC_CTL_DEBUG] =
i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
-}
+else
+params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
I also wonder how come GUC_LOG_DISABLED isn't set today when
i915.guc_log_level == -1, given that:
#define GUC_LOG_DISABLED (1 << 6)
Is that bit set by default somehow if i915 does not program it?
Yes currently GUC_LOG_DISABLED won't get set for guc_log_level = -1.
But then log buffer address will go as NULL and GUC_LOG_VALID flag will
go as 0, for guc_log_level = -1. So this way logging on GuC side will
not get enabled.
I hope I understood your concern correctly.
Yes, this clarifies it. Although I do have one more question then - what
happens if at boot i915.guc_log_level == -1 and then with later patches
logging gets enabled via debugfs - who and how sets
params[GUC_CTL_DEBUG]? Host2GuC overrides this parameter?
Yes through Host2GuC action type, UK_LOG_ENABLE_LOGGING, Host will
request GuC firmware to enable/disable logging and alter the verbosity
level.
The params[GUC_CTL_DEBUG] is just part of the firmware initialization
parameters and is not used after that.
Got it now, in that case:
Reviewed-by: Tvrtko Ursulin
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/3] drm/vgem: Fix mmaping
The vGEM mmap code has bitrotted slightly and now immediately BUGs.
Since vGEM was last updated, there are new core GEM facilities to
provide more common functions, so let's use those here.
v2: drm_gem_free_mmap_offset() is performed from
drm_gem_object_release() so we can remove the redundant call.
Testcase: igt/vgem_basic/mmap
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96603
Signed-off-by: Chris Wilson
Cc: Sean Paul
Cc: Zach Reizner
Cc: Matthew Auld
Tested-by: Humberto Israel Perez Rodriguez
Reviewed-by: Matthew Auld
Acked-by: Zach Reizner
---
drivers/gpu/drm/vgem/vgem_drv.c | 164 +++-
drivers/gpu/drm/vgem/vgem_drv.h | 6 --
2 files changed, 61 insertions(+), 109 deletions(-)
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index 35ea5d02a827..c161b6d7e427 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -42,81 +42,38 @@
#define DRIVER_MAJOR 1
#define DRIVER_MINOR 0
-void vgem_gem_put_pages(struct drm_vgem_gem_object *obj)
-{
- drm_gem_put_pages(&obj->base, obj->pages, false, false);
- obj->pages = NULL;
-}
-
static void vgem_gem_free_object(struct drm_gem_object *obj)
{
struct drm_vgem_gem_object *vgem_obj = to_vgem_bo(obj);
- drm_gem_free_mmap_offset(obj);
-
- if (vgem_obj->use_dma_buf && obj->dma_buf) {
- dma_buf_put(obj->dma_buf);
- obj->dma_buf = NULL;
- }
-
drm_gem_object_release(obj);
-
- if (vgem_obj->pages)
- vgem_gem_put_pages(vgem_obj);
-
- vgem_obj->pages = NULL;
-
kfree(vgem_obj);
}
-int vgem_gem_get_pages(struct drm_vgem_gem_object *obj)
-{
- struct page **pages;
-
- if (obj->pages || obj->use_dma_buf)
- return 0;
-
- pages = drm_gem_get_pages(&obj->base);
- if (IS_ERR(pages)) {
- return PTR_ERR(pages);
- }
-
- obj->pages = pages;
-
- return 0;
-}
-
static int vgem_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
{
struct drm_vgem_gem_object *obj = vma->vm_private_data;
- loff_t num_pages;
- pgoff_t page_offset;
- int ret;
-
/* We don't use vmf->pgoff since that has the fake offset */
- page_offset = ((unsigned long)vmf->virtual_address - vma->vm_start) >>
- PAGE_SHIFT;
-
- num_pages = DIV_ROUND_UP(obj->base.size, PAGE_SIZE);
-
- if (page_offset > num_pages)
- return VM_FAULT_SIGBUS;
-
- ret = vm_insert_page(vma, (unsigned long)vmf->virtual_address,
-obj->pages[page_offset]);
- switch (ret) {
- case 0:
- return VM_FAULT_NOPAGE;
- case -ENOMEM:
- return VM_FAULT_OOM;
- case -EBUSY:
- return VM_FAULT_RETRY;
- case -EFAULT:
- case -EINVAL:
- return VM_FAULT_SIGBUS;
- default:
- WARN_ON(1);
- return VM_FAULT_SIGBUS;
+ unsigned long vaddr = (unsigned long)vmf->virtual_address;
+ struct page *page;
+
+ page = shmem_read_mapping_page(file_inode(obj->base.filp)->i_mapping,
+ (vaddr - vma->vm_start) >> PAGE_SHIFT);
+ if (!IS_ERR(page)) {
+ vmf->page = page;
+ return 0;
+ } else switch (PTR_ERR(page)) {
+ case -ENOSPC:
+ case -ENOMEM:
+ return VM_FAULT_OOM;
+ case -EBUSY:
+ return VM_FAULT_RETRY;
+ case -EFAULT:
+ case -EINVAL:
+ return VM_FAULT_SIGBUS;
+ default:
+ WARN_ON_ONCE(PTR_ERR(page));
+ return VM_FAULT_SIGBUS;
}
}
@@ -134,57 +91,43 @@ static struct drm_gem_object *vgem_gem_create(struct
drm_device *dev,
unsigned long size)
{
struct drm_vgem_gem_object *obj;
- struct drm_gem_object *gem_object;
- int err;
-
- size = roundup(size, PAGE_SIZE);
+ int ret;
obj = kzalloc(sizeof(*obj), GFP_KERNEL);
if (!obj)
return ERR_PTR(-ENOMEM);
- gem_object = &obj->base;
-
- err = drm_gem_object_init(dev, gem_object, size);
- if (err)
- goto out;
-
- err = vgem_gem_get_pages(obj);
- if (err)
- goto out;
-
- err = drm_gem_handle_create(file, gem_object, handle);
- if (err)
- goto handle_out;
+ ret = drm_gem_object_init(dev, &obj->base, roundup(size, PAGE_SIZE));
+ if (ret)
+ goto err_free;
- drm_gem_object_unreference_unlocked(gem_object);
+ ret = drm_gem_handle_create(file, &obj->base, handle);
+ drm_gem_object_unreference_unlocked(&obj->base);
+ if (ret)
+ goto err;
- return gem_o
[Intel-gfx] drm/vgem fixes and new ioctl for testing prime
Just a quick resend of the existing vgem patches, all 3 have been acked, but only the first 2 have reviews. The third involves both new ioctl and dma-buf/fences, so perhaps people have been reluctant... But now is a good time! These patches are exercised by intel-gpu-tools (or will be once the new ioctls are ratified). -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/3] drm/vgem: Enable dmabuf interface for export
Enable the standard GEM dma-buf interface provided by the DRM core, but
only for exporting the VGEM object. This allows passing around the VGEM
objects created from the dumb interface and using them as sources
elsewhere. Creating a VGEM object for a foriegn handle is not supported.
v2: With additional completeness.
v3: Need to clear the CPU cache upon exporting the dma-addresses.
v4: Use drm_gem_put_pages() as well.
v5: Use drm_prime_pages_to_sg()
Testcase: igt/vgem_basic/dmabuf-*
Testcase: igt/prime_vgem
Signed-off-by: Chris Wilson
Cc: Sean Paul
Cc: Zach Reizner
Acked-by: Zach Reizner
Reviewed-by: Matthew Auld
---
drivers/gpu/drm/vgem/vgem_drv.c | 89 -
1 file changed, 88 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index c161b6d7e427..b5fb968d2d5c 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -192,14 +192,101 @@ static const struct file_operations vgem_driver_fops = {
.release= drm_release,
};
+static int vgem_prime_pin(struct drm_gem_object *obj)
+{
+ long n_pages = obj->size >> PAGE_SHIFT;
+ struct page **pages;
+
+ /* Flush the object from the CPU cache so that importers can rely
+* on coherent indirect access via the exported dma-address.
+*/
+ pages = drm_gem_get_pages(obj);
+ if (IS_ERR(pages))
+ return PTR_ERR(pages);
+
+ drm_clflush_pages(pages, n_pages);
+ drm_gem_put_pages(obj, pages, true, false);
+
+ return 0;
+}
+
+static struct sg_table *vgem_prime_get_sg_table(struct drm_gem_object *obj)
+{
+ struct sg_table *st;
+ struct page **pages;
+
+ pages = drm_gem_get_pages(obj);
+ if (IS_ERR(pages))
+ return ERR_CAST(pages);
+
+ st = drm_prime_pages_to_sg(pages, obj->size >> PAGE_SHIFT);
+ drm_gem_put_pages(obj, pages, false, false);
+
+ return st;
+}
+
+static void *vgem_prime_vmap(struct drm_gem_object *obj)
+{
+ long n_pages = obj->size >> PAGE_SHIFT;
+ struct page **pages;
+ void *addr;
+
+ pages = drm_gem_get_pages(obj);
+ if (IS_ERR(pages))
+ return NULL;
+
+ addr = vmap(pages, n_pages, 0, pgprot_writecombine(PAGE_KERNEL_IO));
+ drm_gem_put_pages(obj, pages, false, false);
+
+ return addr;
+}
+
+static void vgem_prime_vunmap(struct drm_gem_object *obj, void *vaddr)
+{
+ vunmap(vaddr);
+}
+
+static int vgem_prime_mmap(struct drm_gem_object *obj,
+ struct vm_area_struct *vma)
+{
+ int ret;
+
+ if (obj->size < vma->vm_end - vma->vm_start)
+ return -EINVAL;
+
+ if (!obj->filp)
+ return -ENODEV;
+
+ ret = obj->filp->f_op->mmap(obj->filp, vma);
+ if (ret)
+ return ret;
+
+ fput(vma->vm_file);
+ vma->vm_file = get_file(obj->filp);
+ vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
+ vma->vm_page_prot =
pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
+
+ return 0;
+}
+
static struct drm_driver vgem_driver = {
- .driver_features= DRIVER_GEM,
+ .driver_features= DRIVER_GEM | DRIVER_PRIME,
.gem_free_object_unlocked = vgem_gem_free_object,
.gem_vm_ops = &vgem_gem_vm_ops,
.ioctls = vgem_ioctls,
.fops = &vgem_driver_fops,
+
.dumb_create= vgem_gem_dumb_create,
.dumb_map_offset= vgem_gem_dumb_map,
+
+ .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
+ .gem_prime_pin = vgem_prime_pin,
+ .gem_prime_export = drm_gem_prime_export,
+ .gem_prime_get_sg_table = vgem_prime_get_sg_table,
+ .gem_prime_vmap = vgem_prime_vmap,
+ .gem_prime_vunmap = vgem_prime_vunmap,
+ .gem_prime_mmap = vgem_prime_mmap,
+
.name = DRIVER_NAME,
.desc = DRIVER_DESC,
.date = DRIVER_DATE,
--
2.8.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/3] drm/vgem: Attach sw fences to exported vGEM dma-buf (ioctl)
vGEM buffers are useful for passing data between software clients and
hardware renders. By allowing the user to create and attach fences to
the exported vGEM buffers (on the dma-buf), the user can implement a
deferred renderer and queue hardware operations like flipping and then
signal the buffer readiness (i.e. this allows the user to schedule
operations out-of-order, but have them complete in-order).
This also makes it much easier to write tightly controlled testcases for
dma-buf fencing and signaling between hardware drivers.
Testcase: igt/vgem_basic/dmabuf-fence
Signed-off-by: Chris Wilson
Cc: Sean Paul
Cc: Zach Reizner
Cc: Gustavo Padovan
Acked-by: Zach Reizner
---
drivers/gpu/drm/vgem/Makefile | 2 +-
drivers/gpu/drm/vgem/vgem_drv.c | 34 ++
drivers/gpu/drm/vgem/vgem_drv.h | 18
drivers/gpu/drm/vgem/vgem_fence.c | 220 ++
include/uapi/drm/vgem_drm.h | 62 +++
5 files changed, 335 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/vgem/vgem_fence.c
create mode 100644 include/uapi/drm/vgem_drm.h
diff --git a/drivers/gpu/drm/vgem/Makefile b/drivers/gpu/drm/vgem/Makefile
index 3f4c7b842028..bfcdea1330e6 100644
--- a/drivers/gpu/drm/vgem/Makefile
+++ b/drivers/gpu/drm/vgem/Makefile
@@ -1,4 +1,4 @@
ccflags-y := -Iinclude/drm
-vgem-y := vgem_drv.o
+vgem-y := vgem_drv.o vgem_fence.o
obj-$(CONFIG_DRM_VGEM) += vgem.o
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index b5fb968d2d5c..2659e5cda857 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -83,6 +83,34 @@ static const struct vm_operations_struct vgem_gem_vm_ops = {
.close = drm_gem_vm_close,
};
+static int vgem_open(struct drm_device *dev, struct drm_file *file)
+{
+ struct vgem_file *vfile;
+ int ret;
+
+ vfile = kzalloc(sizeof(*vfile), GFP_KERNEL);
+ if (!vfile)
+ return -ENOMEM;
+
+ file->driver_priv = vfile;
+
+ ret = vgem_fence_open(vfile);
+ if (ret) {
+ kfree(vfile);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void vgem_preclose(struct drm_device *dev, struct drm_file *file)
+{
+ struct vgem_file *vfile = file->driver_priv;
+
+ vgem_fence_close(vfile);
+ kfree(vfile);
+}
+
/* ioctls */
static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
@@ -164,6 +192,8 @@ unref:
}
static struct drm_ioctl_desc vgem_ioctls[] = {
+ DRM_IOCTL_DEF_DRV(VGEM_FENCE_ATTACH, vgem_fence_attach_ioctl,
DRM_AUTH|DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF_DRV(VGEM_FENCE_SIGNAL, vgem_fence_signal_ioctl,
DRM_AUTH|DRM_RENDER_ALLOW),
};
static int vgem_mmap(struct file *filp, struct vm_area_struct *vma)
@@ -271,9 +301,12 @@ static int vgem_prime_mmap(struct drm_gem_object *obj,
static struct drm_driver vgem_driver = {
.driver_features= DRIVER_GEM | DRIVER_PRIME,
+ .open = vgem_open,
+ .preclose = vgem_preclose,
.gem_free_object_unlocked = vgem_gem_free_object,
.gem_vm_ops = &vgem_gem_vm_ops,
.ioctls = vgem_ioctls,
+ .num_ioctls = ARRAY_SIZE(vgem_ioctls),
.fops = &vgem_driver_fops,
.dumb_create= vgem_gem_dumb_create,
@@ -328,5 +361,6 @@ module_init(vgem_init);
module_exit(vgem_exit);
MODULE_AUTHOR("Red Hat, Inc.");
+MODULE_AUTHOR("Intel Corporation");
MODULE_DESCRIPTION(DRIVER_DESC);
MODULE_LICENSE("GPL and additional rights");
diff --git a/drivers/gpu/drm/vgem/vgem_drv.h b/drivers/gpu/drm/vgem/vgem_drv.h
index 988cbaae7588..88ce21010e28 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.h
+++ b/drivers/gpu/drm/vgem/vgem_drv.h
@@ -32,9 +32,27 @@
#include
#include
+#include
+
+struct vgem_file {
+ struct idr fence_idr;
+ struct mutex fence_mutex;
+ u64 fence_context;
+ atomic_t fence_seqno;
+};
+
#define to_vgem_bo(x) container_of(x, struct drm_vgem_gem_object, base)
struct drm_vgem_gem_object {
struct drm_gem_object base;
};
+int vgem_fence_open(struct vgem_file *file);
+int vgem_fence_attach_ioctl(struct drm_device *dev,
+ void *data,
+ struct drm_file *file);
+int vgem_fence_signal_ioctl(struct drm_device *dev,
+ void *data,
+ struct drm_file *file);
+void vgem_fence_close(struct vgem_file *file);
+
#endif
diff --git a/drivers/gpu/drm/vgem/vgem_fence.c
b/drivers/gpu/drm/vgem/vgem_fence.c
new file mode 100644
index ..649e9e1cee35
--- /dev/null
+++ b/drivers/gpu/drm/vgem/vgem_fence.c
@@ -0,0 +1,220 @@
+/*
+ * Copyright 2016 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associa
Re: [Intel-gfx] [PATCH 06/10] drm/i915: Define a separate variable and control for RPS waitboost frequency
On Mon, Jul 11, 2016 at 03:55:35PM +0300, Mika Kuoppala wrote: > Chris Wilson writes: > > > On Mon, Jul 11, 2016 at 02:39:19PM +0300, Mika Kuoppala wrote: > >> What should be the rules for boost_freq? With this change it can > >> be over max_freq_softlimit and thus will break pm_rps. > > > > The point is to be an independent variable that the user can set in > > addition to max freq, just like the cpu turbo. > > But can it be over max_freq? If so, pm_rps needs tweaking. max_freq being the hw limit? > Also, noticed that this is not set up in gen6_init_rps_frequencies() It's set after we determine the hw frequencies, in the common block after the backends do their bit. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/17] drm/i915: Support for GuC interrupts
On 7/11/2016 4:00 PM, Tvrtko Ursulin wrote:
On 10/07/16 14:41, akash.g...@intel.com wrote:
From: Sagar Arun Kamble
There are certain types of interrupts which Host can recieve from GuC.
GuC ukernel sends an interrupt to Host for certain events, like for
example retrieve/consume the logs generated by ukernel.
This patch adds support to receive interrupts from GuC but currently
enables & partially handles only the interrupt sent by GuC ukernel.
Future patches will add support for handling other interrupt types.
v2:
- Use common low level routines for PM IER/IIR programming (Chris)
- Rename interrupt functions to gen9_xxx from gen8_xxx (Chris)
- Replace disabling of wake ref asserts with rpm get/put (Chris)
v3:
- Update comments for more clarity. (Tvrtko)
- Remove the masking of GuC interrupt, which was kept masked till the
start of bottom half, its not really needed as there is only a
single instance of work item & wq is ordered. (Tvrtko)
v4:
- Rebase.
- Rename guc_events to pm_guc_events so as to be indicative of the
register/control block it is associated with. (Chris)
- Add handling for back to back log buffer flush interrupts.
Signed-off-by: Sagar Arun Kamble
Signed-off-by: Akash Goel
---
drivers/gpu/drm/i915/i915_drv.h| 1 +
drivers/gpu/drm/i915/i915_guc_submission.c | 5 ++
drivers/gpu/drm/i915/i915_irq.c| 98
--
drivers/gpu/drm/i915/i915_reg.h| 11
drivers/gpu/drm/i915/intel_drv.h | 3 +
drivers/gpu/drm/i915/intel_guc.h | 4 ++
drivers/gpu/drm/i915/intel_guc_loader.c| 4 ++
7 files changed, 122 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index c3a579f..6e2ddfa 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1794,6 +1794,7 @@ struct drm_i915_private {
u32 pm_imr;
u32 pm_ier;
u32 pm_rps_events;
+u32 pm_guc_events;
u32 pipestat_irq_mask[I915_MAX_PIPES];
struct i915_hotplug hotplug;
+
/**
* bdw_update_port_irq - update DE port interrupt
* @dev_priv: driver private
@@ -1174,6 +1208,21 @@ static void gen6_pm_rps_work(struct work_struct
*work)
mutex_unlock(&dev_priv->rps.hw_lock);
}
+static void gen9_guc2host_events_work(struct work_struct *work)
+{
+struct drm_i915_private *dev_priv =
+container_of(work, struct drm_i915_private, guc.events_work);
+
+spin_lock_irq(&dev_priv->irq_lock);
+/* Speed up work cancellation during disabling guc interrupts. */
+if (!dev_priv->guc.interrupts_enabled) {
+spin_unlock_irq(&dev_priv->irq_lock);
+return;
+}
+spin_unlock_irq(&dev_priv->irq_lock);
+
+/* TODO: Handle the events for which GuC interrupted host */
+}
/**
* ivybridge_parity_work - Workqueue called when a parity error
interrupt
@@ -1346,11 +1395,13 @@ static irqreturn_t gen8_gt_irq_ack(struct
drm_i915_private *dev_priv,
DRM_ERROR("The master control interrupt lied (GT3)!\n");
}
-if (master_ctl & GEN8_GT_PM_IRQ) {
+if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
gt_iir[2] = I915_READ_FW(GEN8_GT_IIR(2));
-if (gt_iir[2] & dev_priv->pm_rps_events) {
+if (gt_iir[2] & (dev_priv->pm_rps_events |
+ dev_priv->pm_guc_events)) {
I915_WRITE_FW(GEN8_GT_IIR(2),
- gt_iir[2] & dev_priv->pm_rps_events);
+ gt_iir[2] & (dev_priv->pm_rps_events |
+ dev_priv->pm_guc_events));
ret = IRQ_HANDLED;
} else
DRM_ERROR("The master control interrupt lied (PM)!\n");
@@ -1382,6 +1433,9 @@ static void gen8_gt_irq_handler(struct
drm_i915_private *dev_priv,
if (gt_iir[2] & dev_priv->pm_rps_events)
gen6_rps_irq_handler(dev_priv, gt_iir[2]);
+
+if (gt_iir[2] & dev_priv->pm_guc_events)
+gen9_guc_irq_handler(dev_priv, gt_iir[2]);
}
static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
@@ -1628,6 +1682,38 @@ static void gen6_rps_irq_handler(struct
drm_i915_private *dev_priv, u32 pm_iir)
}
}
+static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv,
u32 gt_iir)
+{
+if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
+spin_lock(&dev_priv->irq_lock);
+if (dev_priv->guc.interrupts_enabled) {
+/* Sample the log buffer flush related bits & clear them
+ * out now itself from the message identity register to
+ * minimize the probability of losing a flush interrupt,
+ * when there are back to back flush interrupts.
+ * There can be a new flush interrupt, for different log
+ * buffer type (like for ISR), whilst Host is handling
+ * one (for DPC). Since same bit is used in message
+ * register for ISR & DPC, it could happen that GuC
+ * sets the bit
Re: [Intel-gfx] [PATCH 05/17] drm/i915: Support for GuC interrupts
On 11/07/16 14:15, Goel, Akash wrote:
On 7/11/2016 4:00 PM, Tvrtko Ursulin wrote:
On 10/07/16 14:41, akash.g...@intel.com wrote:
From: Sagar Arun Kamble
There are certain types of interrupts which Host can recieve from GuC.
GuC ukernel sends an interrupt to Host for certain events, like for
example retrieve/consume the logs generated by ukernel.
This patch adds support to receive interrupts from GuC but currently
enables & partially handles only the interrupt sent by GuC ukernel.
Future patches will add support for handling other interrupt types.
v2:
- Use common low level routines for PM IER/IIR programming (Chris)
- Rename interrupt functions to gen9_xxx from gen8_xxx (Chris)
- Replace disabling of wake ref asserts with rpm get/put (Chris)
v3:
- Update comments for more clarity. (Tvrtko)
- Remove the masking of GuC interrupt, which was kept masked till the
start of bottom half, its not really needed as there is only a
single instance of work item & wq is ordered. (Tvrtko)
v4:
- Rebase.
- Rename guc_events to pm_guc_events so as to be indicative of the
register/control block it is associated with. (Chris)
- Add handling for back to back log buffer flush interrupts.
Signed-off-by: Sagar Arun Kamble
Signed-off-by: Akash Goel
---
drivers/gpu/drm/i915/i915_drv.h| 1 +
drivers/gpu/drm/i915/i915_guc_submission.c | 5 ++
drivers/gpu/drm/i915/i915_irq.c| 98
--
drivers/gpu/drm/i915/i915_reg.h| 11
drivers/gpu/drm/i915/intel_drv.h | 3 +
drivers/gpu/drm/i915/intel_guc.h | 4 ++
drivers/gpu/drm/i915/intel_guc_loader.c| 4 ++
7 files changed, 122 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index c3a579f..6e2ddfa 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1794,6 +1794,7 @@ struct drm_i915_private {
u32 pm_imr;
u32 pm_ier;
u32 pm_rps_events;
+u32 pm_guc_events;
u32 pipestat_irq_mask[I915_MAX_PIPES];
struct i915_hotplug hotplug;
+
/**
* bdw_update_port_irq - update DE port interrupt
* @dev_priv: driver private
@@ -1174,6 +1208,21 @@ static void gen6_pm_rps_work(struct work_struct
*work)
mutex_unlock(&dev_priv->rps.hw_lock);
}
+static void gen9_guc2host_events_work(struct work_struct *work)
+{
+struct drm_i915_private *dev_priv =
+container_of(work, struct drm_i915_private, guc.events_work);
+
+spin_lock_irq(&dev_priv->irq_lock);
+/* Speed up work cancellation during disabling guc interrupts. */
+if (!dev_priv->guc.interrupts_enabled) {
+spin_unlock_irq(&dev_priv->irq_lock);
+return;
+}
+spin_unlock_irq(&dev_priv->irq_lock);
+
+/* TODO: Handle the events for which GuC interrupted host */
+}
/**
* ivybridge_parity_work - Workqueue called when a parity error
interrupt
@@ -1346,11 +1395,13 @@ static irqreturn_t gen8_gt_irq_ack(struct
drm_i915_private *dev_priv,
DRM_ERROR("The master control interrupt lied (GT3)!\n");
}
-if (master_ctl & GEN8_GT_PM_IRQ) {
+if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
gt_iir[2] = I915_READ_FW(GEN8_GT_IIR(2));
-if (gt_iir[2] & dev_priv->pm_rps_events) {
+if (gt_iir[2] & (dev_priv->pm_rps_events |
+ dev_priv->pm_guc_events)) {
I915_WRITE_FW(GEN8_GT_IIR(2),
- gt_iir[2] & dev_priv->pm_rps_events);
+ gt_iir[2] & (dev_priv->pm_rps_events |
+ dev_priv->pm_guc_events));
ret = IRQ_HANDLED;
} else
DRM_ERROR("The master control interrupt lied (PM)!\n");
@@ -1382,6 +1433,9 @@ static void gen8_gt_irq_handler(struct
drm_i915_private *dev_priv,
if (gt_iir[2] & dev_priv->pm_rps_events)
gen6_rps_irq_handler(dev_priv, gt_iir[2]);
+
+if (gt_iir[2] & dev_priv->pm_guc_events)
+gen9_guc_irq_handler(dev_priv, gt_iir[2]);
}
static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
@@ -1628,6 +1682,38 @@ static void gen6_rps_irq_handler(struct
drm_i915_private *dev_priv, u32 pm_iir)
}
}
+static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv,
u32 gt_iir)
+{
+if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
+spin_lock(&dev_priv->irq_lock);
+if (dev_priv->guc.interrupts_enabled) {
+/* Sample the log buffer flush related bits & clear them
+ * out now itself from the message identity register to
+ * minimize the probability of losing a flush interrupt,
+ * when there are back to back flush interrupts.
+ * There can be a new flush interrupt, for different log
+ * buffer type (like for ISR), whilst Host is handling
+ * one (for DPC). Since same bit is used in message
+ * register for ISR & DPC, it could happen
Re: [Intel-gfx] [PATCH] drm/i915/dmc: Accept symbolic link in firmware name
On ma, 2016-07-11 at 13:55 +0100, ch...@chris-wilson.co.uk wrote: > On Mon, Jul 11, 2016 at 03:45:21PM +0300, Imre Deak wrote: > > On ma, 2016-07-11 at 13:39 +0100, ch...@chris-wilson.co.uk wrote: > > > On Mon, Jul 11, 2016 at 02:23:48PM +0300, Imre Deak wrote: > > > > On to, 2016-07-07 at 17:57 +0300, Mika Kuoppala wrote: > > > > > "Vivi, Rodrigo" writes: > > > > > > > > > > > Nak. > > > > > > > > > > > > I don't intend to update the symbolic links on linux- > > > > > > firmware.git > > > > > > repository anymore so if we receive a new minor version > > > > > > update > > > > > > we > > > > > > are > > > > > > not going to load. > > > > > > > > > > > > I was the one advocating in the favor for the symbolic link > > > > > > flexibility > > > > > > but I lost the discussions for the stability and validation > > > > > > etc. > > > > > > > > > > > > > > > > And I was one advocating in favor of getting rid of symlink. > > > > > But > > > > > the > > > > > filename versioning is superfluous as the contents has the > > > > > version > > > > > info > > > > > which we can solely rely to not run something we dont want. > > > > > > > > > > So I am not sure what we lose in stability and validation > > > > > front > > > > > with the strict version check. > > > > > > > > Bisection is more cumbersome with a symlink. > > > > > > Did you miss a without there? Because when bisecting the kernel > > > it's > > > harder without the symlink as the build breaks otherwise and the > > > runtime > > > is not bisectable either. > > > > No, I meant when bisecting we also want to load the proper fw > > version > > for each bisect commit point. So using exact filenames we'll > > automatically load the proper firmware file for a given commit, > > whereas > > with symlinks we have to adjust the symlink at each bisect step. > > You mean you have to adjust the kernel build at each point. That is > the > root of the problem as we do not have deferred firmware loading. Yes. Looking at the bug now: it's true that using CONFIG_EXTRA_FIRMWARE you have to update this config option at each bisect point, but you would have to anyway update the symlink in that case too. > And then you get random changes in the firmare whilst bisecting the > kernel. What do you mean random? During bisecting we want to load the firmware version that was used with a particular commit. With a symlink pointing to the wrong firmware file for a given commit, we'd fail loading the firmware due to the version check and hide/introduce bugs for that commit. --Imre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: fix wrong ifdef condition for mutex owner
Got compiling error if disabling MUTEX_SPIN_ON_OWNER and DEBUG_MUTEXES: drivers/gpu/drm/i915/i915_gem_shrinker.c:44:14: error: 'struct mutex' has no member named 'owner'. Signed-off-by: Hong Liu --- drivers/gpu/drm/i915/i915_gem_shrinker.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c index 067632a..0066b9c 100644 --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c @@ -40,7 +40,7 @@ static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task) if (!mutex_is_locked(mutex)) return false; -#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_MUTEXES) +#if defined(CONFIG_SMP) && defined(CONFIG_DEBUG_MUTEXES) return mutex->owner == task; #else /* Since UP may be pre-empted, we cannot assume that we own the lock */ -- 2.7.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/10] drm/i915: Move overclocking detection to alongside RPS frequency detection
Chris Wilson writes:
> Move the overclocking max frequency detection alongside the regular
> frequency detection, before we expose the undefined value to userspace.
>
> Signed-off-by: Chris Wilson
> ---
> drivers/gpu/drm/i915/intel_pm.c | 24 +++-
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 54f739fbd133..24b23a51c56b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5343,7 +5343,7 @@ static void gen8_enable_rps(struct drm_i915_private
> *dev_priv)
> static void gen6_enable_rps(struct drm_i915_private *dev_priv)
> {
> struct intel_engine_cs *engine;
> - u32 rc6vids, pcu_mbox = 0, rc6_mask = 0;
> + u32 rc6vids, rc6_mask = 0;
> u32 gtfifodbg;
> int rc6_mode;
> int ret;
> @@ -5417,14 +5417,6 @@ static void gen6_enable_rps(struct drm_i915_private
> *dev_priv)
> if (ret)
> DRM_DEBUG_DRIVER("Failed to set the min frequency\n");
>
> - ret = sandybridge_pcode_read(dev_priv, GEN6_READ_OC_PARAMS, &pcu_mbox);
> - if (!ret && (pcu_mbox & (1<<31))) { /* OC supported */
> - DRM_DEBUG_DRIVER("Overclocking supported. Max: %dMHz, Overclock
> max: %dMHz\n",
> - (dev_priv->rps.max_freq_softlimit & 0xff) * 50,
> - (pcu_mbox & 0xff) * 50);
> - dev_priv->rps.max_freq = pcu_mbox & 0xff;
> - }
> -
> reset_rps(dev_priv, gen6_set_rps);
>
> rc6vids = 0;
> @@ -6526,6 +6518,20 @@ void intel_init_gt_powersave(struct drm_i915_private
> *dev_priv)
> dev_priv->rps.efficient_freq,
> intel_freq_opcode(dev_priv, 450));
>
> + /* After setting max-softlimit, find the overclock max freq */
> + if (IS_GEN6(dev_priv) ||
> + IS_IVYBRIDGE(dev_priv) || IS_HASWELL(dev_priv)) {
> + u32 params = 0;
> +
> + sandybridge_pcode_read(dev_priv, GEN6_READ_OC_PARAMS, ¶ms);
> + if (params & BIT(31)) { /* OC supported */
> + DRM_DEBUG_DRIVER("Overclocking supported, max: %dMHz,
> overclock: %dMHz\n",
> + (dev_priv->rps.max_freq & 0xff) * 50,
> + (params & 0xff) * 50);
Yeah max_freq makes more sense.
Reviewed-by: Mika Kuoppala
> + dev_priv->rps.max_freq = params & 0xff;
> + }
> + }
> +
> mutex_unlock(&dev_priv->rps.hw_lock);
> }
>
> --
> 2.8.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/17] drm/i915: Support for GuC interrupts
On 7/11/2016 6:53 PM, Tvrtko Ursulin wrote:
On 11/07/16 14:15, Goel, Akash wrote:
On 7/11/2016 4:00 PM, Tvrtko Ursulin wrote:
+static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv,
u32 gt_iir)
+{
+if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
+spin_lock(&dev_priv->irq_lock);
+if (dev_priv->guc.interrupts_enabled) {
+/* Sample the log buffer flush related bits & clear them
+ * out now itself from the message identity register to
+ * minimize the probability of losing a flush interrupt,
+ * when there are back to back flush interrupts.
+ * There can be a new flush interrupt, for different log
+ * buffer type (like for ISR), whilst Host is handling
+ * one (for DPC). Since same bit is used in message
+ * register for ISR & DPC, it could happen that GuC
+ * sets the bit for 2nd interrupt but Host clears out
+ * the bit on handling the 1st interrupt.
+ */
+u32 msg = I915_READ(SOFT_SCRATCH(15)) &
+(GUC2HOST_MSG_CRASH_DUMP_POSTED |
+ GUC2HOST_MSG_FLUSH_LOG_BUFFER);
+if (msg) {
+/* Clear the message bits that are handled */
+I915_WRITE(SOFT_SCRATCH(15),
+I915_READ(SOFT_SCRATCH(15)) & ~msg);
+
+/* Handle flush interrupt event in bottom half */
+queue_work(dev_priv->wq, &dev_priv->guc.events_work);
Since the later patch is changing this to use a thread, since you have
established worker is too slow - especially the shared one - I would
really recommend you start with the kthread straight away. Not have the
worker for a while in the same series and then later change it to a
thread.
Actually it won't be appropriate to say that shared worker thread is too
slow, but having a dedicated kthread definitely helps.
I kept the kthread patch at the last so that as per the response,
review comments can drop it also.
I think it should only be one implementation in the patch series. If we
agreed on a kthread make it so from the start.
Agree but actually right now, added the kthread patch more as a RFC and
presumed this won't be the final version of the series.
Will do the needful, as per the review comments, in the next version.
And describe in the commit message why it was selected etc.
+}
+}
+spin_unlock(&dev_priv->irq_lock);
Why does the above needs to be done under the irq_lock ?
Using the irq_lock for 'guc.interrupts_enabled', especially useful
while disabling the interrupt.
Why? I don't see how it gains you anything and so it seems preferable
not to hold it over mmio accesses.
Yes not needed for the mmio access part.
Just needed for the inspection of 'guc.interrupts_enabled' value.
Will reorder the code.
Best regards
Akash
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/10] drm/i915: Perform static RPS frequency setup before userspace
Chris Wilson writes:
> As these RPS frequency values are part of our userspace interface, they
> must be established before that userspace interface is registered.
>
> Signed-off-by: Chris Wilson
> ---
> drivers/gpu/drm/i915/intel_pm.c | 98
> +
> 1 file changed, 31 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index df72f8e7b4b3..54f739fbd133 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5102,35 +5102,31 @@ int sanitize_rc6_option(struct drm_i915_private
> *dev_priv, int enable_rc6)
>
> static void gen6_init_rps_frequencies(struct drm_i915_private *dev_priv)
> {
> - uint32_t rp_state_cap;
> - u32 ddcc_status = 0;
> - int ret;
> -
> /* All of these values are in units of 50MHz */
> - dev_priv->rps.cur_freq = 0;
> +
> /* static values from HW: RP0 > RP1 > RPn (min_freq) */
> if (IS_BROXTON(dev_priv)) {
> - rp_state_cap = I915_READ(BXT_RP_STATE_CAP);
> + u32 rp_state_cap = I915_READ(BXT_RP_STATE_CAP);
> dev_priv->rps.rp0_freq = (rp_state_cap >> 16) & 0xff;
> dev_priv->rps.rp1_freq = (rp_state_cap >> 8) & 0xff;
> dev_priv->rps.min_freq = (rp_state_cap >> 0) & 0xff;
> } else {
> - rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
> + u32 rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
> dev_priv->rps.rp0_freq = (rp_state_cap >> 0) & 0xff;
> dev_priv->rps.rp1_freq = (rp_state_cap >> 8) & 0xff;
> dev_priv->rps.min_freq = (rp_state_cap >> 16) & 0xff;
> }
> -
> /* hw_max = RP0 until we check for overclocking */
> - dev_priv->rps.max_freq = dev_priv->rps.rp0_freq;
> + dev_priv->rps.max_freq = dev_priv->rps.rp0_freq;
>
> dev_priv->rps.efficient_freq = dev_priv->rps.rp1_freq;
> if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv) ||
> IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
> - ret = sandybridge_pcode_read(dev_priv,
> - HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL,
> - &ddcc_status);
> - if (0 == ret)
> + u32 ddcc_status = 0;
> +
> + if (sandybridge_pcode_read(dev_priv,
> +HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL,
> +&ddcc_status) == 0)
We do the read now without forcewake, but this should not matter.
Reviewed-by: Mika Kuoppala
> dev_priv->rps.efficient_freq =
> clamp_t(u8,
> ((ddcc_status >> 8) & 0xff),
> @@ -5140,30 +5136,14 @@ static void gen6_init_rps_frequencies(struct
> drm_i915_private *dev_priv)
>
> if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
> /* Store the frequency values in 16.66 MHZ units, which is
> -the natural hardware unit for SKL */
> + * the natural hardware unit for SKL
> + */
> dev_priv->rps.rp0_freq *= GEN9_FREQ_SCALER;
> dev_priv->rps.rp1_freq *= GEN9_FREQ_SCALER;
> dev_priv->rps.min_freq *= GEN9_FREQ_SCALER;
> dev_priv->rps.max_freq *= GEN9_FREQ_SCALER;
> dev_priv->rps.efficient_freq *= GEN9_FREQ_SCALER;
> }
> -
> - dev_priv->rps.idle_freq = dev_priv->rps.min_freq;
> - dev_priv->rps.cur_freq = dev_priv->rps.idle_freq;
> -
> - /* Preserve min/max settings in case of re-init */
> - if (dev_priv->rps.max_freq_softlimit == 0)
> - dev_priv->rps.max_freq_softlimit = dev_priv->rps.max_freq;
> -
> - if (dev_priv->rps.min_freq_softlimit == 0) {
> - if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> - dev_priv->rps.min_freq_softlimit =
> - max_t(int, dev_priv->rps.efficient_freq,
> - intel_freq_opcode(dev_priv, 450));
> - else
> - dev_priv->rps.min_freq_softlimit =
> - dev_priv->rps.min_freq;
> - }
> }
>
> static void reset_rps(struct drm_i915_private *dev_priv,
> @@ -5183,8 +5163,6 @@ static void gen9_enable_rps(struct drm_i915_private
> *dev_priv)
> {
> intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>
> - gen6_init_rps_frequencies(dev_priv);
> -
> /* WaGsvDisableTurbo: Workaround to disable turbo on BXT A* */
> if (IS_BXT_REVID(dev_priv, 0, BXT_REVID_A1)) {
> /*
> @@ -5301,9 +5279,6 @@ static void gen8_enable_rps(struct drm_i915_private
> *dev_priv)
> /* 2a: Disable RC states. */
> I915_WRITE(GEN6_RC_CONTROL, 0);
>
> - /* Initialize rps frequencies */
> - gen6_init_rps_frequencies(dev_priv);
> -
> /* 2b: Program RC
Re: [Intel-gfx] [PATCH 05/17] drm/i915: Support for GuC interrupts
On 11/07/16 14:38, Goel, Akash wrote:
On 7/11/2016 6:53 PM, Tvrtko Ursulin wrote:
On 11/07/16 14:15, Goel, Akash wrote:
On 7/11/2016 4:00 PM, Tvrtko Ursulin wrote:
+static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv,
u32 gt_iir)
+{
+if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
+spin_lock(&dev_priv->irq_lock);
+if (dev_priv->guc.interrupts_enabled) {
+/* Sample the log buffer flush related bits & clear them
+ * out now itself from the message identity register to
+ * minimize the probability of losing a flush interrupt,
+ * when there are back to back flush interrupts.
+ * There can be a new flush interrupt, for different log
+ * buffer type (like for ISR), whilst Host is handling
+ * one (for DPC). Since same bit is used in message
+ * register for ISR & DPC, it could happen that GuC
+ * sets the bit for 2nd interrupt but Host clears out
+ * the bit on handling the 1st interrupt.
+ */
+u32 msg = I915_READ(SOFT_SCRATCH(15)) &
+(GUC2HOST_MSG_CRASH_DUMP_POSTED |
+ GUC2HOST_MSG_FLUSH_LOG_BUFFER);
+if (msg) {
+/* Clear the message bits that are handled */
+I915_WRITE(SOFT_SCRATCH(15),
+I915_READ(SOFT_SCRATCH(15)) & ~msg);
+
+/* Handle flush interrupt event in bottom half */
+queue_work(dev_priv->wq, &dev_priv->guc.events_work);
Since the later patch is changing this to use a thread, since you have
established worker is too slow - especially the shared one - I would
really recommend you start with the kthread straight away. Not have the
worker for a while in the same series and then later change it to a
thread.
Actually it won't be appropriate to say that shared worker thread is too
slow, but having a dedicated kthread definitely helps.
I kept the kthread patch at the last so that as per the response,
review comments can drop it also.
I think it should only be one implementation in the patch series. If we
agreed on a kthread make it so from the start.
Agree but actually right now, added the kthread patch more as a RFC and
presumed this won't be the final version of the series.
Will do the needful, as per the review comments, in the next version.
Ack.
And describe in the commit message why it was selected etc.
+}
+}
+spin_unlock(&dev_priv->irq_lock);
Why does the above needs to be done under the irq_lock ?
Using the irq_lock for 'guc.interrupts_enabled', especially useful
while disabling the interrupt.
Why? I don't see how it gains you anything and so it seems preferable
not to hold it over mmio accesses.
Yes not needed for the mmio access part.
Just needed for the inspection of 'guc.interrupts_enabled' value.
Will reorder the code.
You don't need it just for reading that value, you can just drop it.
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Update ifdeffery for mutex->owner
In commit 7608a43d8f2e ("locking/mutexes: Use MUTEX_SPIN_ON_OWNER when
appropriate") the owner field in the mutex was updated from being
dependent upon CONFIG_SMP to using optimistic spin. Update our peek
function to suite.
Fixes:7608a43d8f2e ("locking/mutexes: Use MUTEX_SPIN_ON_OWNER...")
Reported-by: Hong Liu
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/i915_gem_shrinker.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c
b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index 8ad95695f80c..ef2bd970ef0b 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -40,7 +40,7 @@ static bool mutex_is_locked_by(struct mutex *mutex, struct
task_struct *task)
if (!mutex_is_locked(mutex))
return false;
-#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_MUTEXES)
+#if defined(CONFIG_DEBUG_MUTEXES) || defined(CONFIG_MUTEX_SPIN_ON_OWNER)
return mutex->owner == task;
#else
/* Since UP may be pre-empted, we cannot assume that we own the lock */
--
2.8.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/dmc: Accept symbolic link in firmware name
On Mon, Jul 11, 2016 at 04:24:50PM +0300, Imre Deak wrote: > On ma, 2016-07-11 at 13:55 +0100, ch...@chris-wilson.co.uk wrote: > > And then you get random changes in the firmare whilst bisecting the > > kernel. > > What do you mean random? During bisecting we want to load the firmware > version that was used with a particular commit. With a symlink pointing > to the wrong firmware file for a given commit, we'd fail loading the > firmware due to the version check and hide/introduce bugs for that > commit. No. You want to be changing exactly one variable, which means leaving the firmware constant. The firmware should be side-ways compatible for everything with the same minor version (thus resolvable from the same symlink), right? -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/dmc: Accept symbolic link in firmware name
On ma, 2016-07-11 at 14:50 +0100, ch...@chris-wilson.co.uk wrote: > On Mon, Jul 11, 2016 at 04:24:50PM +0300, Imre Deak wrote: > > On ma, 2016-07-11 at 13:55 +0100, ch...@chris-wilson.co.uk wrote: > > > And then you get random changes in the firmare whilst bisecting > > > the > > > kernel. > > > > What do you mean random? During bisecting we want to load the > > firmware > > version that was used with a particular commit. With a symlink > > pointing > > to the wrong firmware file for a given commit, we'd fail loading > > the > > firmware due to the version check and hide/introduce bugs for that > > commit. > > No. You want to be changing exactly one variable, which means leaving > the firmware constant. Hm, not sure. When looking for a working snapshot you also want to consider bugs introduced by the firmware itself. This is in a way the exact reason why we want stricter control on the firmware version and introduced a white list. This also means that loading a firmware version other than what the driver allows (at a given commit) won't work anyway. > The firmware should be side-ways compatible for > everything with the same minor version (thus resolvable from the same > symlink), right? From the same major version I guess it should, but the reason things don't work that way is why we introduced version white listing. --Imre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ Ro.CI.BAT: failure for series starting with [1/3] drm/vgem: Fix mmaping
== Series Details == Series: series starting with [1/3] drm/vgem: Fix mmaping URL : https://patchwork.freedesktop.org/series/9716/ State : failure == Summary == Series 9716v1 Series without cover letter http://patchwork.freedesktop.org/api/1.0/series/9716/revisions/1/mbox Test drv_module_reload_basic: pass -> DMESG-WARN (ro-bdw-i7-5600u) Test gem_sync: Subgroup basic-store-each: pass -> DMESG-FAIL (ro-bdw-i7-5600u) Test kms_pipe_crc_basic: Subgroup nonblocking-crc-pipe-b-frame-sequence: pass -> SKIP (fi-skl-i5-6260u) Subgroup suspend-read-crc-pipe-a: skip -> DMESG-WARN (ro-bdw-i7-5557U) Test prime_vgem: Subgroup basic-busy-default: skip -> FAIL (ro-bdw-i7-5557U) skip -> FAIL (ro-ilk1-i5-650) skip -> FAIL (ro-hsw-i3-4010u) skip -> FAIL (fi-skl-i5-6260u) skip -> FAIL (ro-bdw-i7-5600u) skip -> FAIL (ro-snb-i7-2620M) skip -> FAIL (ro-bdw-i5-5250u) skip -> FAIL (ro-hsw-i7-4770r) skip -> FAIL (fi-snb-i7-2600) skip -> FAIL (ro-ivb-i7-3770) skip -> FAIL (ro-byt-n2820) skip -> FAIL (fi-skl-i7-6700k) skip -> FAIL (ro-skl3-i5-6260u) Subgroup basic-gtt: skip -> PASS (ro-bdw-i7-5557U) skip -> PASS (ro-ilk1-i5-650) skip -> PASS (ro-hsw-i3-4010u) skip -> PASS (fi-skl-i5-6260u) skip -> PASS (ro-bdw-i7-5600u) skip -> PASS (ro-snb-i7-2620M) skip -> PASS (ro-bdw-i5-5250u) skip -> PASS (ro-hsw-i7-4770r) skip -> PASS (fi-snb-i7-2600) skip -> PASS (ro-ivb-i7-3770) skip -> PASS (ro-byt-n2820) skip -> PASS (fi-skl-i7-6700k) skip -> PASS (ro-skl3-i5-6260u) Subgroup basic-read: skip -> PASS (ro-bdw-i7-5557U) skip -> PASS (ro-ilk1-i5-650) skip -> PASS (ro-hsw-i3-4010u) skip -> PASS (fi-skl-i5-6260u) skip -> PASS (ro-bdw-i7-5600u) skip -> PASS (ro-snb-i7-2620M) skip -> PASS (ro-bdw-i5-5250u) skip -> PASS (ro-hsw-i7-4770r) skip -> PASS (fi-snb-i7-2600) skip -> PASS (ro-ivb-i7-3770) skip -> PASS (ro-byt-n2820) skip -> PASS (fi-skl-i7-6700k) skip -> PASS (ro-skl3-i5-6260u) Subgroup basic-sync-default: skip -> FAIL (ro-bdw-i7-5557U) skip -> FAIL (ro-ilk1-i5-650) skip -> FAIL (ro-hsw-i3-4010u) skip -> FAIL (fi-skl-i5-6260u) skip -> FAIL (ro-bdw-i7-5600u) skip -> FAIL (ro-snb-i7-2620M) skip -> FAIL (ro-bdw-i5-5250u) skip -> FAIL (ro-hsw-i7-4770r) skip -> FAIL (fi-snb-i7-2600) skip -> FAIL (ro-ivb-i7-3770) skip -> FAIL (ro-byt-n2820) skip -> FAIL (fi-skl-i7-6700k) skip -> FAIL (ro-skl3-i5-6260u) Subgroup basic-wait-default: skip -> FAIL (ro-bdw-i7-5557U) skip -> FAIL (ro-ilk1-i5-650) skip -> FAIL (ro-hsw-i3-4010u) skip -> FAIL (fi-skl-i5-6260u) skip -> FAIL (ro-bdw-i7-5600u) skip -> FAIL (ro-snb-i7-2620M) skip -> FAIL (ro-bdw-i5-5250u) skip -> FAIL (ro-hsw-i7-4770r) skip -> FAIL (fi-snb-i7-2600) skip -> FAIL (ro-ivb-i7-3770) skip -> FAIL (ro-byt-n2820) skip -> FAIL (fi-skl-i7-6700k) skip -> FAIL (ro-skl3-i5-6260u) Subgroup basic-write: skip -> PASS (ro-bdw-i7-5557U) skip -> PASS (ro-ilk1-i5-650) skip -> PASS (ro-hsw-i3-4010u) skip -> PASS (fi-skl-i5-6260u) skip
Re: [Intel-gfx] [PATCH 07/10] drm/i915: Remove superfluous powersave work flushing
Chris Wilson writes:
> Instead of flushing the outstanding enabling, remember the requested
> frequency to apply when the powersave work runs.
>
> Signed-off-by: Chris Wilson
> Cc: Ville Syrjälä
Reviewed-by: Mika Kuoppala
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 30 ++---
> drivers/gpu/drm/i915/i915_sysfs.c | 52
> ++---
> 2 files changed, 16 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index d1ff4cb9b90e..90aef4540193 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1205,8 +1205,6 @@ static int i915_frequency_info(struct seq_file *m, void
> *unused)
>
> intel_runtime_pm_get(dev_priv);
>
> - flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> -
> if (IS_GEN5(dev)) {
> u16 rgvswctl = I915_READ16(MEMSWCTL);
> u16 rgvstat = I915_READ16(MEMSTAT_ILK);
> @@ -1898,8 +1896,6 @@ static int i915_ring_freq_table(struct seq_file *m,
> void *unused)
>
> intel_runtime_pm_get(dev_priv);
>
> - flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> -
> ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
> if (ret)
> goto out;
> @@ -4952,20 +4948,11 @@ i915_max_freq_get(void *data, u64 *val)
> {
> struct drm_device *dev = data;
> struct drm_i915_private *dev_priv = to_i915(dev);
> - int ret;
>
> if (INTEL_INFO(dev)->gen < 6)
> return -ENODEV;
>
> - flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> -
> - ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
> - if (ret)
> - return ret;
> -
> *val = intel_gpu_freq(dev_priv, dev_priv->rps.max_freq_softlimit);
> - mutex_unlock(&dev_priv->rps.hw_lock);
> -
> return 0;
> }
>
> @@ -4980,8 +4967,6 @@ i915_max_freq_set(void *data, u64 val)
> if (INTEL_INFO(dev)->gen < 6)
> return -ENODEV;
>
> - flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> -
> DRM_DEBUG_DRIVER("Manually setting max freq to %llu\n", val);
>
> ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
> @@ -5019,20 +5004,11 @@ i915_min_freq_get(void *data, u64 *val)
> {
> struct drm_device *dev = data;
> struct drm_i915_private *dev_priv = to_i915(dev);
> - int ret;
>
> - if (INTEL_INFO(dev)->gen < 6)
> + if (INTEL_GEN(dev_priv) < 6)
> return -ENODEV;
>
> - flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> -
> - ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
> - if (ret)
> - return ret;
> -
> *val = intel_gpu_freq(dev_priv, dev_priv->rps.min_freq_softlimit);
> - mutex_unlock(&dev_priv->rps.hw_lock);
> -
> return 0;
> }
>
> @@ -5044,11 +5020,9 @@ i915_min_freq_set(void *data, u64 val)
> u32 hw_max, hw_min;
> int ret;
>
> - if (INTEL_INFO(dev)->gen < 6)
> + if (INTEL_GEN(dev_priv) < 6)
> return -ENODEV;
>
> - flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> -
> DRM_DEBUG_DRIVER("Manually setting min freq to %llu\n", val);
>
> ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c
> b/drivers/gpu/drm/i915/i915_sysfs.c
> index 8c045ff47f0e..d47281b4b1c1 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -271,8 +271,6 @@ static ssize_t gt_act_freq_mhz_show(struct device *kdev,
> struct drm_i915_private *dev_priv = to_i915(dev);
> int ret;
>
> - flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> -
> intel_runtime_pm_get(dev_priv);
>
> mutex_lock(&dev_priv->rps.hw_lock);
> @@ -303,19 +301,10 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
> struct drm_minor *minor = dev_to_drm_minor(kdev);
> struct drm_device *dev = minor->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> - int ret;
> -
> - flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> -
> - intel_runtime_pm_get(dev_priv);
> -
> - mutex_lock(&dev_priv->rps.hw_lock);
> - ret = intel_gpu_freq(dev_priv, dev_priv->rps.cur_freq);
> - mutex_unlock(&dev_priv->rps.hw_lock);
>
> - intel_runtime_pm_put(dev_priv);
> -
> - return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> + return snprintf(buf, PAGE_SIZE, "%d\n",
> + intel_gpu_freq(dev_priv,
> +dev_priv->rps.cur_freq));
> }
>
> static ssize_t gt_boost_freq_mhz_show(struct device *kdev, struct
> device_attribute *attr, char *buf)
> @@ -324,7 +313,8 @@ static ssize_t gt_boost_freq_mhz_show(struct device
> *kdev, struct device_attribu
> struct drm_i915_private *dev_priv = to_i915(minor->dev);
>
> return snprintf(buf, PAGE_SIZE, "%d\n",
> -
Re: [Intel-gfx] [PATCH 08/10] drm/i915: Defer enabling rc6 til after we submit the first batch/context
Chris Wilson writes:
> Some hardware requires a valid render context before it can initiate
> rc6 power gating of the GPU; the default state of the GPU is not
> sufficient and may lead to undefined behaviour. The first execution of
> any batch will load the "golden render state", at which point it is safe
> to enable rc6. As we do not forcibly load the kernel context at resume,
> we have to hook into the batch submission to be sure that the render
> state is setup before enabling rc6.
>
> However, since we don't enable powersaving until that first batch, we
> queued a delayed task in order to guarantee that the batch is indeed
> submitted.
>
> v2: Rearrange intel_disable_gt_powersave() to match.
> v3: Apply user specified cur_freq (or idle_freq if not set).
> v4: Give in, and supply a delayed work to autoenable rc6
> v5: Mika suggested a couple of better names for delayed_resume_work
>
> Signed-off-by: Chris Wilson
> Cc: Mika Kuoppala
> Cc: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/i915_drv.c | 7 +-
> drivers/gpu/drm/i915/i915_drv.h | 2 +-
> drivers/gpu/drm/i915/i915_gem.c | 1 +
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> drivers/gpu/drm/i915/intel_drv.h | 2 +
> drivers/gpu/drm/i915/intel_pm.c | 122
> +++
> drivers/gpu/drm/i915/intel_uncore.c | 2 +-
> 7 files changed, 89 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b9a811750ca8..ff3342b78a74 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1343,7 +1343,7 @@ void i915_driver_unload(struct drm_device *dev)
> i915_destroy_error_state(dev);
>
> /* Flush any outstanding unpin_work. */
> - flush_workqueue(dev_priv->wq);
> + drain_workqueue(dev_priv->wq);
>
For this to make sense, should we also use the dev priv workqueue
for the gt autoenabling?
-Mika
> intel_guc_fini(dev);
> i915_gem_fini(dev);
> @@ -1652,6 +1652,7 @@ static int i915_drm_resume(struct drm_device *dev)
>
> intel_opregion_notify_adapter(dev_priv, PCI_D0);
>
> + intel_autoenable_gt_powersave(dev_priv);
> drm_kms_helper_poll_enable(dev);
>
> enable_rpm_wakeref_asserts(dev_priv);
> @@ -1835,8 +1836,7 @@ int i915_reset(struct drm_i915_private *dev_priv)
>* previous concerns that it doesn't respond well to some forms
>* of re-init after reset.
>*/
> - if (INTEL_INFO(dev)->gen > 5)
> - intel_enable_gt_powersave(dev_priv);
> + intel_autoenable_gt_powersave(dev_priv);
>
> return 0;
>
> @@ -2459,7 +2459,6 @@ static int intel_runtime_resume(struct device *device)
>* we can do is to hope that things will still work (and disable RPM).
>*/
> i915_gem_init_swizzling(dev);
> - gen6_update_ring_freq(dev_priv);
>
> intel_runtime_pm_enable_interrupts(dev_priv);
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bb59bc637f7b..8d74db096189 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1188,7 +1188,7 @@ struct intel_gen6_power_mgmt {
> bool client_boost;
>
> bool enabled;
> - struct delayed_work delayed_resume_work;
> + struct delayed_work autoenable_work;
> unsigned boosts;
>
> struct intel_rps_client semaphores, mmioflips;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index adeca0ec4cfb..6f2227b24711 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2842,6 +2842,7 @@ static void i915_gem_mark_busy(const struct
> intel_engine_cs *engine)
> intel_runtime_pm_get_noresume(dev_priv);
> dev_priv->gt.awake = true;
>
> + intel_enable_gt_powersave(dev_priv);
> i915_update_gfx_val(dev_priv);
> if (INTEL_GEN(dev_priv) >= 6)
> gen6_rps_busy(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index be3b2cab2640..92cd0a70f8c8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15456,7 +15456,6 @@ void intel_modeset_init_hw(struct drm_device *dev)
> dev_priv->atomic_cdclk_freq = dev_priv->cdclk_freq;
>
> intel_init_clock_gating(dev);
> - intel_enable_gt_powersave(dev_priv);
> }
>
> /*
> @@ -16252,6 +16251,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
>
> + intel_suspend_gt_powersave(dev_priv);
> intel_disable_gt_powersave(dev_priv);
>
> /*
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 55aeaf041749..c68dd6166c52 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1697,7 +1697,9 @@ void intel_gpu_ips_init(struct drm_i915_pri
Re: [Intel-gfx] [PATCH 05/17] drm/i915: Support for GuC interrupts
On 7/11/2016 7:13 PM, Tvrtko Ursulin wrote:
On 11/07/16 14:38, Goel, Akash wrote:
On 7/11/2016 6:53 PM, Tvrtko Ursulin wrote:
On 11/07/16 14:15, Goel, Akash wrote:
On 7/11/2016 4:00 PM, Tvrtko Ursulin wrote:
+static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv,
u32 gt_iir)
+{
+if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
+spin_lock(&dev_priv->irq_lock);
+if (dev_priv->guc.interrupts_enabled) {
+/* Sample the log buffer flush related bits & clear them
+ * out now itself from the message identity register to
+ * minimize the probability of losing a flush interrupt,
+ * when there are back to back flush interrupts.
+ * There can be a new flush interrupt, for different log
+ * buffer type (like for ISR), whilst Host is handling
+ * one (for DPC). Since same bit is used in message
+ * register for ISR & DPC, it could happen that GuC
+ * sets the bit for 2nd interrupt but Host clears out
+ * the bit on handling the 1st interrupt.
+ */
+u32 msg = I915_READ(SOFT_SCRATCH(15)) &
+(GUC2HOST_MSG_CRASH_DUMP_POSTED |
+ GUC2HOST_MSG_FLUSH_LOG_BUFFER);
+if (msg) {
+/* Clear the message bits that are handled */
+I915_WRITE(SOFT_SCRATCH(15),
+I915_READ(SOFT_SCRATCH(15)) & ~msg);
+
+/* Handle flush interrupt event in bottom half */
+queue_work(dev_priv->wq,
&dev_priv->guc.events_work);
Since the later patch is changing this to use a thread, since you have
established worker is too slow - especially the shared one - I would
really recommend you start with the kthread straight away. Not have
the
worker for a while in the same series and then later change it to a
thread.
Actually it won't be appropriate to say that shared worker thread is
too
slow, but having a dedicated kthread definitely helps.
I kept the kthread patch at the last so that as per the response,
review comments can drop it also.
I think it should only be one implementation in the patch series. If we
agreed on a kthread make it so from the start.
Agree but actually right now, added the kthread patch more as a RFC and
presumed this won't be the final version of the series.
Will do the needful, as per the review comments, in the next version.
Ack.
And describe in the commit message why it was selected etc.
+}
+}
+spin_unlock(&dev_priv->irq_lock);
Why does the above needs to be done under the irq_lock ?
Using the irq_lock for 'guc.interrupts_enabled', especially useful
while disabling the interrupt.
Why? I don't see how it gains you anything and so it seems preferable
not to hold it over mmio accesses.
Yes not needed for the mmio access part.
Just needed for the inspection of 'guc.interrupts_enabled' value.
Will reorder the code.
You don't need it just for reading that value, you can just drop it.
Its not strictly needed as its a mere read. But as per my limited
understanding, without the spinlock (which provides an implicit barrier
also) ISR might miss the reset of 'interrupts_enabled' flag, from a
thread on other CPU, and queue the new work. The update will be
visible eventually though. And same applies to the case when
'interrupts_enabled' flag is set from other CPU.
Good practice to use locks for accessing shared variables ?.
Best regards
Akash
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/10] drm/i915: Defer enabling rc6 til after we submit the first batch/context
On Mon, Jul 11, 2016 at 05:14:22PM +0300, Mika Kuoppala wrote: > Chris Wilson writes: > > > Some hardware requires a valid render context before it can initiate > > rc6 power gating of the GPU; the default state of the GPU is not > > sufficient and may lead to undefined behaviour. The first execution of > > any batch will load the "golden render state", at which point it is safe > > to enable rc6. As we do not forcibly load the kernel context at resume, > > we have to hook into the batch submission to be sure that the render > > state is setup before enabling rc6. > > > > However, since we don't enable powersaving until that first batch, we > > queued a delayed task in order to guarantee that the batch is indeed > > submitted. > > > > v2: Rearrange intel_disable_gt_powersave() to match. > > v3: Apply user specified cur_freq (or idle_freq if not set). > > v4: Give in, and supply a delayed work to autoenable rc6 > > v5: Mika suggested a couple of better names for delayed_resume_work > > > > Signed-off-by: Chris Wilson > > Cc: Mika Kuoppala > > Cc: Ville Syrjälä > > --- > > drivers/gpu/drm/i915/i915_drv.c | 7 +- > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > drivers/gpu/drm/i915/i915_gem.c | 1 + > > drivers/gpu/drm/i915/intel_display.c | 2 +- > > drivers/gpu/drm/i915/intel_drv.h | 2 + > > drivers/gpu/drm/i915/intel_pm.c | 122 > > +++ > > drivers/gpu/drm/i915/intel_uncore.c | 2 +- > > 7 files changed, 89 insertions(+), 49 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > b/drivers/gpu/drm/i915/i915_drv.c > > index b9a811750ca8..ff3342b78a74 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -1343,7 +1343,7 @@ void i915_driver_unload(struct drm_device *dev) > > i915_destroy_error_state(dev); > > > > /* Flush any outstanding unpin_work. */ > > - flush_workqueue(dev_priv->wq); > > + drain_workqueue(dev_priv->wq); > > > > For this to make sense, should we also use the dev priv workqueue > for the gt autoenabling? Sure. (That change was a bit of paranoia that stuck.) There's nothing stopping the autoenable work from using the dev_priv->wq and that would indeed make both it semantically cleaner. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ Ro.CI.BAT: failure for drm/i915: fix wrong ifdef condition for mutex owner (rev2)
== Series Details == Series: drm/i915: fix wrong ifdef condition for mutex owner (rev2) URL : https://patchwork.freedesktop.org/series/9718/ State : failure == Summary == Series 9718v2 drm/i915: fix wrong ifdef condition for mutex owner http://patchwork.freedesktop.org/api/1.0/series/9718/revisions/2/mbox Test gem_sync: Subgroup basic-store-each: pass -> DMESG-FAIL (ro-bdw-i7-5600u) Test kms_pipe_crc_basic: Subgroup nonblocking-crc-pipe-b: pass -> SKIP (fi-skl-i5-6260u) Subgroup nonblocking-crc-pipe-c-frame-sequence: pass -> SKIP (fi-skl-i5-6260u) Subgroup suspend-read-crc-pipe-a: skip -> DMESG-WARN (ro-bdw-i7-5557U) fi-kbl-qkkr total:237 pass:168 dwarn:27 dfail:2 fail:5 skip:35 fi-skl-i5-6260u total:237 pass:210 dwarn:0 dfail:1 fail:4 skip:22 fi-skl-i7-6700k total:237 pass:198 dwarn:0 dfail:1 fail:4 skip:34 fi-snb-i7-2600 total:237 pass:184 dwarn:0 dfail:1 fail:4 skip:48 ro-bdw-i5-5250u total:237 pass:207 dwarn:1 dfail:1 fail:4 skip:24 ro-bdw-i7-5557U total:237 pass:207 dwarn:2 dfail:1 fail:4 skip:23 ro-bdw-i7-5600u total:237 pass:192 dwarn:0 dfail:2 fail:4 skip:39 ro-bsw-n3050 total:217 pass:170 dwarn:0 dfail:0 fail:4 skip:42 ro-byt-n2820 total:237 pass:183 dwarn:0 dfail:1 fail:7 skip:46 ro-hsw-i3-4010u total:237 pass:200 dwarn:0 dfail:1 fail:4 skip:32 ro-hsw-i7-4770r total:237 pass:200 dwarn:0 dfail:1 fail:4 skip:32 ro-ilk-i7-620lm total:237 pass:160 dwarn:0 dfail:1 fail:5 skip:71 ro-ilk1-i5-650 total:232 pass:160 dwarn:0 dfail:1 fail:5 skip:66 ro-ivb-i7-3770 total:237 pass:191 dwarn:0 dfail:1 fail:4 skip:41 ro-skl3-i5-6260u total:237 pass:211 dwarn:1 dfail:1 fail:4 skip:20 ro-snb-i7-2620M total:237 pass:182 dwarn:0 dfail:1 fail:5 skip:49 Results at /archive/results/CI_IGT_test/RO_Patchwork_1466/ e549c0b drm-intel-nightly: 2016y-07m-11d-12h-49m-29s UTC integration manifest 288d53a drm/i915: Update ifdeffery for mutex->owner ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/vgem: Attach sw fences to exported vGEM dma-buf (ioctl)
2016-07-11 Chris Wilson :
> vGEM buffers are useful for passing data between software clients and
> hardware renders. By allowing the user to create and attach fences to
> the exported vGEM buffers (on the dma-buf), the user can implement a
> deferred renderer and queue hardware operations like flipping and then
> signal the buffer readiness (i.e. this allows the user to schedule
> operations out-of-order, but have them complete in-order).
>
> This also makes it much easier to write tightly controlled testcases for
> dma-buf fencing and signaling between hardware drivers.
>
> Testcase: igt/vgem_basic/dmabuf-fence
> Signed-off-by: Chris Wilson
> Cc: Sean Paul
> Cc: Zach Reizner
> Cc: Gustavo Padovan
> Acked-by: Zach Reizner
> ---
> drivers/gpu/drm/vgem/Makefile | 2 +-
> drivers/gpu/drm/vgem/vgem_drv.c | 34 ++
> drivers/gpu/drm/vgem/vgem_drv.h | 18
> drivers/gpu/drm/vgem/vgem_fence.c | 220
> ++
> include/uapi/drm/vgem_drm.h | 62 +++
> 5 files changed, 335 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/vgem/vgem_fence.c
> create mode 100644 include/uapi/drm/vgem_drm.h
>
> diff --git a/drivers/gpu/drm/vgem/Makefile b/drivers/gpu/drm/vgem/Makefile
> index 3f4c7b842028..bfcdea1330e6 100644
> --- a/drivers/gpu/drm/vgem/Makefile
> +++ b/drivers/gpu/drm/vgem/Makefile
> @@ -1,4 +1,4 @@
> ccflags-y := -Iinclude/drm
> -vgem-y := vgem_drv.o
> +vgem-y := vgem_drv.o vgem_fence.o
>
> obj-$(CONFIG_DRM_VGEM) += vgem.o
> diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
> index b5fb968d2d5c..2659e5cda857 100644
> --- a/drivers/gpu/drm/vgem/vgem_drv.c
> +++ b/drivers/gpu/drm/vgem/vgem_drv.c
> @@ -83,6 +83,34 @@ static const struct vm_operations_struct vgem_gem_vm_ops =
> {
> .close = drm_gem_vm_close,
> };
>
> +static int vgem_open(struct drm_device *dev, struct drm_file *file)
> +{
> + struct vgem_file *vfile;
> + int ret;
> +
> + vfile = kzalloc(sizeof(*vfile), GFP_KERNEL);
> + if (!vfile)
> + return -ENOMEM;
> +
> + file->driver_priv = vfile;
> +
> + ret = vgem_fence_open(vfile);
> + if (ret) {
> + kfree(vfile);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void vgem_preclose(struct drm_device *dev, struct drm_file *file)
> +{
> + struct vgem_file *vfile = file->driver_priv;
> +
> + vgem_fence_close(vfile);
> + kfree(vfile);
> +}
> +
> /* ioctls */
>
> static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
> @@ -164,6 +192,8 @@ unref:
> }
>
> static struct drm_ioctl_desc vgem_ioctls[] = {
> + DRM_IOCTL_DEF_DRV(VGEM_FENCE_ATTACH, vgem_fence_attach_ioctl,
> DRM_AUTH|DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF_DRV(VGEM_FENCE_SIGNAL, vgem_fence_signal_ioctl,
> DRM_AUTH|DRM_RENDER_ALLOW),
> };
>
> static int vgem_mmap(struct file *filp, struct vm_area_struct *vma)
> @@ -271,9 +301,12 @@ static int vgem_prime_mmap(struct drm_gem_object *obj,
>
> static struct drm_driver vgem_driver = {
> .driver_features= DRIVER_GEM | DRIVER_PRIME,
> + .open = vgem_open,
> + .preclose = vgem_preclose,
> .gem_free_object_unlocked = vgem_gem_free_object,
> .gem_vm_ops = &vgem_gem_vm_ops,
> .ioctls = vgem_ioctls,
> + .num_ioctls = ARRAY_SIZE(vgem_ioctls),
> .fops = &vgem_driver_fops,
>
> .dumb_create= vgem_gem_dumb_create,
> @@ -328,5 +361,6 @@ module_init(vgem_init);
> module_exit(vgem_exit);
>
> MODULE_AUTHOR("Red Hat, Inc.");
> +MODULE_AUTHOR("Intel Corporation");
> MODULE_DESCRIPTION(DRIVER_DESC);
> MODULE_LICENSE("GPL and additional rights");
> diff --git a/drivers/gpu/drm/vgem/vgem_drv.h b/drivers/gpu/drm/vgem/vgem_drv.h
> index 988cbaae7588..88ce21010e28 100644
> --- a/drivers/gpu/drm/vgem/vgem_drv.h
> +++ b/drivers/gpu/drm/vgem/vgem_drv.h
> @@ -32,9 +32,27 @@
> #include
> #include
>
> +#include
> +
> +struct vgem_file {
> + struct idr fence_idr;
> + struct mutex fence_mutex;
> + u64 fence_context;
> + atomic_t fence_seqno;
> +};
> +
> #define to_vgem_bo(x) container_of(x, struct drm_vgem_gem_object, base)
> struct drm_vgem_gem_object {
> struct drm_gem_object base;
> };
>
> +int vgem_fence_open(struct vgem_file *file);
> +int vgem_fence_attach_ioctl(struct drm_device *dev,
> + void *data,
> + struct drm_file *file);
> +int vgem_fence_signal_ioctl(struct drm_device *dev,
> + void *data,
> + struct drm_file *file);
> +void vgem_fence_close(struct vgem_file *file);
> +
> #endif
> diff --git a/drivers/gpu/drm/vgem/vgem_fence.c
> b/drivers/gpu/drm/vgem/vgem_fence.c
> new file mode 100644
> index 00
Re: [Intel-gfx] [PATCH 3/3] drm/vgem: Attach sw fences to exported vGEM dma-buf (ioctl)
On Mon, Jul 11, 2016 at 12:10:40PM -0300, Gustavo Padovan wrote:
> 2016-07-11 Chris Wilson :
>
> > vGEM buffers are useful for passing data between software clients and
> > hardware renders. By allowing the user to create and attach fences to
> > the exported vGEM buffers (on the dma-buf), the user can implement a
> > deferred renderer and queue hardware operations like flipping and then
> > signal the buffer readiness (i.e. this allows the user to schedule
> > operations out-of-order, but have them complete in-order).
> >
> > This also makes it much easier to write tightly controlled testcases for
> > dma-buf fencing and signaling between hardware drivers.
> >
> > Testcase: igt/vgem_basic/dmabuf-fence
> > Signed-off-by: Chris Wilson
> > Cc: Sean Paul
> > Cc: Zach Reizner
> > Cc: Gustavo Padovan
> > Acked-by: Zach Reizner
> > ---
> > drivers/gpu/drm/vgem/Makefile | 2 +-
> > drivers/gpu/drm/vgem/vgem_drv.c | 34 ++
> > drivers/gpu/drm/vgem/vgem_drv.h | 18
> > drivers/gpu/drm/vgem/vgem_fence.c | 220
> > ++
> > include/uapi/drm/vgem_drm.h | 62 +++
> > 5 files changed, 335 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/gpu/drm/vgem/vgem_fence.c
> > create mode 100644 include/uapi/drm/vgem_drm.h
> >
> > diff --git a/drivers/gpu/drm/vgem/Makefile b/drivers/gpu/drm/vgem/Makefile
> > index 3f4c7b842028..bfcdea1330e6 100644
> > --- a/drivers/gpu/drm/vgem/Makefile
> > +++ b/drivers/gpu/drm/vgem/Makefile
> > @@ -1,4 +1,4 @@
> > ccflags-y := -Iinclude/drm
> > -vgem-y := vgem_drv.o
> > +vgem-y := vgem_drv.o vgem_fence.o
> >
> > obj-$(CONFIG_DRM_VGEM) += vgem.o
> > diff --git a/drivers/gpu/drm/vgem/vgem_drv.c
> > b/drivers/gpu/drm/vgem/vgem_drv.c
> > index b5fb968d2d5c..2659e5cda857 100644
> > --- a/drivers/gpu/drm/vgem/vgem_drv.c
> > +++ b/drivers/gpu/drm/vgem/vgem_drv.c
> > @@ -83,6 +83,34 @@ static const struct vm_operations_struct vgem_gem_vm_ops
> > = {
> > .close = drm_gem_vm_close,
> > };
> >
> > +static int vgem_open(struct drm_device *dev, struct drm_file *file)
> > +{
> > + struct vgem_file *vfile;
> > + int ret;
> > +
> > + vfile = kzalloc(sizeof(*vfile), GFP_KERNEL);
> > + if (!vfile)
> > + return -ENOMEM;
> > +
> > + file->driver_priv = vfile;
> > +
> > + ret = vgem_fence_open(vfile);
> > + if (ret) {
> > + kfree(vfile);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void vgem_preclose(struct drm_device *dev, struct drm_file *file)
> > +{
> > + struct vgem_file *vfile = file->driver_priv;
> > +
> > + vgem_fence_close(vfile);
> > + kfree(vfile);
> > +}
> > +
> > /* ioctls */
> >
> > static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
> > @@ -164,6 +192,8 @@ unref:
> > }
> >
> > static struct drm_ioctl_desc vgem_ioctls[] = {
> > + DRM_IOCTL_DEF_DRV(VGEM_FENCE_ATTACH, vgem_fence_attach_ioctl,
> > DRM_AUTH|DRM_RENDER_ALLOW),
> > + DRM_IOCTL_DEF_DRV(VGEM_FENCE_SIGNAL, vgem_fence_signal_ioctl,
> > DRM_AUTH|DRM_RENDER_ALLOW),
> > };
> >
> > static int vgem_mmap(struct file *filp, struct vm_area_struct *vma)
> > @@ -271,9 +301,12 @@ static int vgem_prime_mmap(struct drm_gem_object *obj,
> >
> > static struct drm_driver vgem_driver = {
> > .driver_features= DRIVER_GEM | DRIVER_PRIME,
> > + .open = vgem_open,
> > + .preclose = vgem_preclose,
> > .gem_free_object_unlocked = vgem_gem_free_object,
> > .gem_vm_ops = &vgem_gem_vm_ops,
> > .ioctls = vgem_ioctls,
> > + .num_ioctls = ARRAY_SIZE(vgem_ioctls),
> > .fops = &vgem_driver_fops,
> >
> > .dumb_create= vgem_gem_dumb_create,
> > @@ -328,5 +361,6 @@ module_init(vgem_init);
> > module_exit(vgem_exit);
> >
> > MODULE_AUTHOR("Red Hat, Inc.");
> > +MODULE_AUTHOR("Intel Corporation");
> > MODULE_DESCRIPTION(DRIVER_DESC);
> > MODULE_LICENSE("GPL and additional rights");
> > diff --git a/drivers/gpu/drm/vgem/vgem_drv.h
> > b/drivers/gpu/drm/vgem/vgem_drv.h
> > index 988cbaae7588..88ce21010e28 100644
> > --- a/drivers/gpu/drm/vgem/vgem_drv.h
> > +++ b/drivers/gpu/drm/vgem/vgem_drv.h
> > @@ -32,9 +32,27 @@
> > #include
> > #include
> >
> > +#include
> > +
> > +struct vgem_file {
> > + struct idr fence_idr;
> > + struct mutex fence_mutex;
> > + u64 fence_context;
> > + atomic_t fence_seqno;
> > +};
> > +
> > #define to_vgem_bo(x) container_of(x, struct drm_vgem_gem_object, base)
> > struct drm_vgem_gem_object {
> > struct drm_gem_object base;
> > };
> >
> > +int vgem_fence_open(struct vgem_file *file);
> > +int vgem_fence_attach_ioctl(struct drm_device *dev,
> > + void *data,
> > + struct drm_file *file);
> > +int vgem_fence_signal_ioctl(struct drm_device *dev,
>
Re: [Intel-gfx] [PATCH 10/10] drm/i915: Remove temporary RPM wakeref assert disables
Chris Wilson writes: > Now that the last couple of hacks have been removed from the runtime > powermanagement users, we can fully enable the asserts. > > Signed-off-by: Chris Wilson Reviewed-by: Mika Kuoppala > --- > drivers/gpu/drm/i915/intel_drv.h | 7 --- > 1 file changed, 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 9c78fda491f0..26bfcccad57e 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1665,13 +1665,6 @@ enable_rpm_wakeref_asserts(struct drm_i915_private > *dev_priv) > atomic_dec(&dev_priv->pm.wakeref_count); > } > > -/* TODO: convert users of these to rely instead on proper RPM refcounting */ > -#define DISABLE_RPM_WAKEREF_ASSERTS(dev_priv)\ > - disable_rpm_wakeref_asserts(dev_priv) > - > -#define ENABLE_RPM_WAKEREF_ASSERTS(dev_priv) \ > - enable_rpm_wakeref_asserts(dev_priv) > - > void intel_runtime_pm_get(struct drm_i915_private *dev_priv); > bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *dev_priv); > void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv); > -- > 2.8.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915/guc: symbolic names for user load/submission preferences
The existing code that accesses the "enable_guc_loading" and
"enable_guc_submission" parameters uses explicit numerical
values for the various possibilities, including in some cases
relying on boolean 0/1 mapping to specific values (which could
be confusing for maintainers).
So this patch just provides and uses names for the values
representing the DEFAULT, DISABLED, PREFERRED, and MANDATORY
options that the user can select (-1, 0, 1, 2 respectively).
This should produce identical code to the previous version!
Signed-off-by: Dave Gordon
Cc: Tvrtko Ursulin
---
drivers/gpu/drm/i915/i915_guc_submission.c | 2 +-
drivers/gpu/drm/i915/intel_guc.h | 15 +++
drivers/gpu/drm/i915/intel_guc_loader.c| 26 ++
drivers/gpu/drm/i915/intel_lrc.c | 6 +++---
4 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 2112e02..33c0e0ab 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -971,7 +971,7 @@ int i915_guc_submission_init(struct drm_i915_private
*dev_priv)
bitmap_clear(guc->doorbell_bitmap, 0, GUC_MAX_DOORBELLS);
i915_guc_submission_disable(dev_priv);
- if (!i915.enable_guc_submission)
+ if (i915.enable_guc_submission == GUC_SUBMISSION_DISABLED)
return 0; /* not enabled */
if (guc->ctx_pool_obj)
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 3e3e743..7ac835c 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -90,6 +90,21 @@ struct i915_guc_client {
uint64_t submissions[I915_NUM_ENGINES];
};
+/* These represent user-requested preferences */
+enum {
+ GUC_SUBMISSION_DEFAULT = -1,
+ GUC_SUBMISSION_DISABLED = 0,
+ GUC_SUBMISSION_PREFERRED,
+ GUC_SUBMISSION_MANDATORY
+};
+enum {
+ FIRMWARE_LOAD_DEFAULT = -1,
+ FIRMWARE_LOAD_DISABLED = 0,
+ FIRMWARE_LOAD_PREFERRED,
+ FIRMWARE_LOAD_MANDATORY
+};
+
+/* These represent the actual firmware status */
enum intel_guc_fw_status {
GUC_FIRMWARE_FAIL = -1,
GUC_FIRMWARE_NONE = 0,
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
b/drivers/gpu/drm/i915/intel_guc_loader.c
index 605c696..2cd37db 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -189,7 +189,7 @@ static void set_guc_init_params(struct drm_i915_private
*dev_priv)
}
/* If GuC submission is enabled, set up additional parameters here */
- if (i915.enable_guc_submission) {
+ if (i915.enable_guc_submission != GUC_SUBMISSION_DISABLED) {
u32 pgs = i915_gem_obj_ggtt_offset(dev_priv->guc.ctx_pool_obj);
u32 ctx_in_16 = GUC_MAX_GPU_CONTEXTS / 16;
@@ -424,7 +424,7 @@ int intel_guc_setup(struct drm_device *dev)
intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
/* Loading forbidden, or no firmware to load? */
- if (!i915.enable_guc_loading) {
+ if (i915.enable_guc_loading == FIRMWARE_LOAD_DISABLED) {
err = 0;
goto fail;
} else if (fw_path == NULL) {
@@ -493,7 +493,7 @@ int intel_guc_setup(struct drm_device *dev)
intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
- if (i915.enable_guc_submission) {
+ if (i915.enable_guc_submission != GUC_SUBMISSION_DISABLED) {
err = i915_guc_submission_enable(dev_priv);
if (err)
goto fail;
@@ -519,9 +519,9 @@ int intel_guc_setup(struct drm_device *dev)
* nonfatal error (i.e. it doesn't prevent driver load, but
* marks the GPU as wedged until reset).
*/
- if (i915.enable_guc_loading > 1) {
+ if (i915.enable_guc_loading >= FIRMWARE_LOAD_MANDATORY) {
ret = -EIO;
- } else if (i915.enable_guc_submission > 1) {
+ } else if (i915.enable_guc_submission >= GUC_SUBMISSION_MANDATORY) {
ret = -EIO;
} else {
ret = 0;
@@ -536,7 +536,7 @@ int intel_guc_setup(struct drm_device *dev)
else
DRM_ERROR("GuC firmware load failed: %d\n", err);
- if (i915.enable_guc_submission) {
+ if (i915.enable_guc_submission != GUC_SUBMISSION_DISABLED) {
if (fw_path == NULL)
DRM_INFO("GuC submission without firmware not
supported\n");
if (ret == 0)
@@ -544,7 +544,7 @@ int intel_guc_setup(struct drm_device *dev)
else
DRM_ERROR("GuC init failed: %d\n", ret);
}
- i915.enable_guc_submission = 0;
+ i915.enable_guc_submission = GUC_SUBMISSION_DISABLED;
return ret;
}
@@ -686,10 +686,12 @@ vo
[Intel-gfx] [PATCH 2/3] drm/i915/guc: downgrade some DRM_ERROR() messages to DRM_WARN()
Where we're going to continue regardless of the problem, rather than
fail, then the message should be a WARNing rather than an ERROR.
Signed-off-by: Dave Gordon
---
drivers/gpu/drm/i915/i915_guc_submission.c | 18 +++---
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 2112e02..e299b64 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -114,10 +114,8 @@ static int host2guc_action(struct intel_guc *guc, u32
*data, u32 len)
if (ret != -ETIMEDOUT)
ret = -EIO;
- DRM_ERROR("GUC: host2guc action 0x%X failed. ret=%d "
- "status=0x%08X response=0x%08X\n",
- data[0], ret, status,
- I915_READ(SOFT_SCRATCH(15)));
+ DRM_WARN("Action 0x%X failed; ret=%d status=0x%08X
response=0x%08X\n",
+data[0], ret, status, I915_READ(SOFT_SCRATCH(15)));
dev_priv->guc.action_fail += 1;
dev_priv->guc.action_err = ret;
@@ -553,8 +551,8 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)
if (db_ret.db_status == GUC_DOORBELL_DISABLED)
break;
- DRM_ERROR("Cookie mismatch. Expected %d, returned %d\n",
- db_cmp.cookie, db_ret.cookie);
+ DRM_WARN("Cookie mismatch. Expected %d, found %d\n",
+db_cmp.cookie, db_ret.cookie);
/* update the cookie to newly read cookie from GuC */
db_cmp.cookie = db_ret.cookie;
@@ -726,8 +724,8 @@ static void guc_init_doorbell_hw(struct intel_guc *guc)
/* Restore to original value */
err = guc_update_doorbell_id(guc, client, db_id);
if (err)
- DRM_ERROR("Failed to restore doorbell to %d, err %d\n",
- db_id, err);
+ DRM_WARN("Failed to restore doorbell to %d, err %d\n",
+db_id, err);
for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
i915_reg_t drbreg = GEN8_DRBREGL(i);
@@ -819,8 +817,6 @@ static void guc_init_doorbell_hw(struct intel_guc *guc)
return client;
err:
- DRM_ERROR("FAILED to create priority %u GuC client!\n", priority);
-
guc_client_free(dev_priv, client);
return NULL;
}
@@ -998,7 +994,7 @@ int i915_guc_submission_enable(struct drm_i915_private
*dev_priv)
GUC_CTX_PRIORITY_KMD_NORMAL,
dev_priv->kernel_context);
if (!client) {
- DRM_ERROR("Failed to create execbuf guc_client\n");
+ DRM_ERROR("Failed to create normal GuC client!\n");
return -ENOMEM;
}
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/3] drm: extra printk() wrapper macros
We had only DRM_INFO() and DRM_ERROR(), whereas the underlying printk()
provides several other useful intermediate levels such as NOTICE and
WARNING. So this patch fills out the set by providing both regular and
once-only macros for each of the levels INFO, NOTICE, and WARNING, using
a common underlying macro that does all the token-pasting.
DRM_ERROR is unchanged, as it's not just a printk wrapper.
Signed-off-by: Dave Gordon
---
include/drm/drmP.h | 26 --
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index cf918e3e..82648b1 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -162,6 +162,26 @@ void drm_err(const char *format, ...);
/** \name Macros to make printk easier */
/*@{*/
+#define_DRM_PRINTK(once, level, fmt, ...)
\
+ do {\
+ printk##once(KERN_##level "[" DRM_NAME "] " fmt,\
+##__VA_ARGS__);\
+ } while (0)
+
+#define DRM_INFO(fmt, ...) \
+ _DRM_PRINTK(, INFO, fmt, ##__VA_ARGS__)
+#define DRM_NOTE(fmt, ...) \
+ _DRM_PRINTK(, NOTICE, fmt, ##__VA_ARGS__)
+#define DRM_WARN(fmt, ...) \
+ _DRM_PRINTK(, WARNING, fmt, ##__VA_ARGS__)
+
+#define DRM_INFO_ONCE(fmt, ...)
\
+ _DRM_PRINTK(_once, INFO, fmt, __VA_ARGS__)
+#define DRM_NOTE_ONCE(fmt, ...)\
+ _DRM_PRINTK(_once, NOTICE, fmt, ##__VA_ARGS__)
+#define DRM_WARN_ONCE(fmt, ...)
\
+ _DRM_PRINTK(_once, WARNING, fmt, ##__VA_ARGS__)
+
/**
* Error output.
*
@@ -187,12 +207,6 @@ void drm_err(const char *format, ...);
drm_err(fmt, ##__VA_ARGS__);\
})
-#define DRM_INFO(fmt, ...) \
- printk(KERN_INFO "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
-
-#define DRM_INFO_ONCE(fmt, ...)\
- printk_once(KERN_INFO "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
-
/**
* Debug output.
*
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/3] drm/i915/guc: revisit GuC loader message levels
Some downgraded from DRM_ERROR() to DRM_WARN(), some eliminated,
and a few upgraded from DRM_INFO() to DRM_NOTE() or DRM_WARN().
Signed-off-by: Dave Gordon
---
drivers/gpu/drm/i915/intel_guc_loader.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
b/drivers/gpu/drm/i915/intel_guc_loader.c
index 605c696..fd032eb 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -140,12 +140,14 @@ static u32 get_gttype(struct drm_i915_private *dev_priv)
static u32 get_core_family(struct drm_i915_private *dev_priv)
{
- switch (INTEL_INFO(dev_priv)->gen) {
+ u32 gen = INTEL_GEN(dev_priv);
+
+ switch (gen) {
case 9:
return GFXCORE_FAMILY_GEN9;
default:
- DRM_ERROR("GUC: unsupported core family\n");
+ DRM_WARN("GEN%d does not support GuC operation\n", gen);
return GFXCORE_FAMILY_UNKNOWN;
}
}
@@ -433,7 +435,7 @@ int intel_guc_setup(struct drm_device *dev)
goto fail;
} else if (*fw_path == '\0') {
/* Device has a GuC but we don't know what f/w to load? */
- DRM_INFO("No GuC firmware known for this platform\n");
+ DRM_WARN("No GuC firmware known for this platform\n");
err = -ENODEV;
goto fail;
}
@@ -471,10 +473,8 @@ int intel_guc_setup(struct drm_device *dev)
* that the state and timing are fairly predictable
*/
err = i915_reset_guc(dev_priv);
- if (err) {
- DRM_ERROR("GuC reset failed: %d\n", err);
+ if (err)
goto fail;
- }
err = guc_ucode_xfer(dev_priv);
if (!err)
@@ -532,15 +532,15 @@ int intel_guc_setup(struct drm_device *dev)
else if (err == 0)
DRM_INFO("GuC firmware load skipped\n");
else if (ret != -EIO)
- DRM_INFO("GuC firmware load failed: %d\n", err);
+ DRM_NOTE("GuC firmware load failed: %d\n", err);
else
- DRM_ERROR("GuC firmware load failed: %d\n", err);
+ DRM_WARN("GuC firmware load failed: %d\n", err);
if (i915.enable_guc_submission) {
if (fw_path == NULL)
DRM_INFO("GuC submission without firmware not
supported\n");
if (ret == 0)
- DRM_INFO("Falling back from GuC submission to execlist
mode\n");
+ DRM_NOTE("Falling back from GuC submission to execlist
mode\n");
else
DRM_ERROR("GuC init failed: %d\n", ret);
}
@@ -656,7 +656,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct
intel_guc_fw *guc_fw)
fail:
DRM_DEBUG_DRIVER("GuC fw fetch status FAIL; err %d, fw %p, obj %p\n",
err, fw, guc_fw->guc_fw_obj);
- DRM_ERROR("Failed to fetch GuC firmware from %s (error %d)\n",
+ DRM_WARN("Failed to fetch GuC firmware from %s (error %d)\n",
guc_fw->guc_fw_path, err);
mutex_lock(&dev->struct_mutex);
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PACTH i-g-t] lib/igt_kms: Fix different order of properties and their name strings
This is a reminder of this series.
On 2016-06-29 07:22 AM, robert.f...@collabora.com wrote:
From: Robert Foss
igt_crtc_prop_names and igt_atomic_crtc_properties have different orders of
properties, which is fixed in this patch.
Signed-off-by: Robert Foss
---
lib/igt_kms.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index af72b90..dd4eb84 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -164,8 +164,8 @@ static const char
*igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
static const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
"background_color",
- "DEGAMMA_LUT",
"CTM",
+ "DEGAMMA_LUT",
"GAMMA_LUT",
};
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/guc: symbolic names for user load/submission preferences
On Mon, Jul 11, 2016 at 06:12:40PM +0100, Dave Gordon wrote: > The existing code that accesses the "enable_guc_loading" and > "enable_guc_submission" parameters uses explicit numerical > values for the various possibilities, including in some cases > relying on boolean 0/1 mapping to specific values (which could > be confusing for maintainers). > > So this patch just provides and uses names for the values > representing the DEFAULT, DISABLED, PREFERRED, and MANDATORY > options that the user can select (-1, 0, 1, 2 respectively). When is MANDATORY a good idea? If the hw doesn't support any other mechanism, then it will shut itself down gracefully if setup fails. If the user wants to force guc for testing, they only need to set the module parameter then check the guc is enabled afterwards and fail the test. At what point do we need such a warty user interface to the kernel? -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915/skl: Add support for the SAGV, fix underrun hangs
Since the watermark calculations for Skylake are still broken, we're apt
to hitting underruns very easily under multi-monitor configurations.
While it would be lovely if this was fixed, it's not. Another problem
that's been coming from this however, is the mysterious issue of
underruns causing full system hangs. An easy way to reproduce this with
a skylake system:
- Get a laptop with a skylake GPU, and hook up two external monitors to
it
- Move the cursor from the built-in LCD to one of the external displays
as quickly as you can
- You'll get a few pipe underruns, and eventually the entire system will
just freeze.
After doing a lot of investigation and reading through the bspec, I
found the existence of the SAGV, which is responsible for adjusting the
system agent voltage and clock frequencies depending on how much power
we need. According to the bspec:
"The display engine access to system memory is blocked during the
adjustment time. SAGV defaults to enabled. Software must use the
GT-driver pcode mailbox to disable SAGV when the display engine is not
able to tolerate the blocking time."
The rest of the bspec goes on to explain that software can simply leave
the SAGV enabled, and disable it when we use interlaced pipes/have more
then one pipe active.
Sure enough, with this patchset the system hangs resulting from pipe
underruns on Skylake have completely vanished on my T460s.
Cc: Daniel Vetter
Cc: Ville Syrjälä
Signed-off-by: Lyude
---
drivers/gpu/drm/i915/i915_drv.h | 2 +
drivers/gpu/drm/i915/i915_reg.h | 5 ++
drivers/gpu/drm/i915/intel_pm.c | 103
3 files changed, 110 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 03e1bfa..660d0a6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1959,6 +1959,8 @@ struct drm_i915_private {
struct i915_suspend_saved_registers regfile;
struct vlv_s0ix_state vlv_s0ix_state;
+ bool skl_sagv_enabled;
+
struct {
/*
* Raw watermark latency values:
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8bfde75..9b2eb0b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7162,6 +7162,11 @@ enum {
#define HSW_PCODE_DE_WRITE_FREQ_REQ 0x17
#define DISPLAY_IPS_CONTROL 0x19
#define HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL 0x1A
+#define GEN9_PCODE_SAGV_CONTROL 0x21
+#define GEN9_SAGV_DISABLE 0x0
+#define GEN9_SAGV_LOW_FREQ 0x1
+#define GEN9_SAGV_HIGH_FREQ0x2
+#define GEN9_SAGV_DYNAMIC_FREQ 0x3
#define GEN6_PCODE_DATA_MMIO(0x138128)
#define GEN6_PCODE_FREQ_IA_RATIO_SHIFT 8
#define GEN6_PCODE_FREQ_RING_RATIO_SHIFT 16
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 5a8ee0c..07807aa 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2876,6 +2876,102 @@ skl_wm_plane_id(const struct intel_plane *plane)
}
static void
+skl_sagv_get_hw_state(struct drm_i915_private *dev_priv)
+{
+ u32 temp;
+ int ret;
+
+ mutex_lock(&dev_priv->rps.hw_lock);
+ ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL, &temp);
+ mutex_unlock(&dev_priv->rps.hw_lock);
+
+ if (!ret) {
+ dev_priv->skl_sagv_enabled = !!(temp & GEN9_SAGV_DYNAMIC_FREQ);
+ } else {
+ /*
+* If for some reason we can't access the SAGV state, follow
+* the bspec and assume it's enabled
+*/
+ DRM_ERROR("Failed to get SAGV state, assuming enabled\n");
+ dev_priv->skl_sagv_enabled = true;
+ }
+}
+
+/*
+ * SAGV dynamically adjusts the system agent voltage and clock frequencies
+ * depending on power and performance requirements. The display engine access
+ * to system memory is blocked during the adjustment time. Having this enabled
+ * in multi-pipe configurations can cause issues (such as underruns causing
+ * full system hangs), and the bspec also suggests that software disable it
+ * when more then one pipe is enabled.
+ */
+static int
+skl_enable_sagv(struct drm_i915_private *dev_priv)
+{
+ int ret;
+
+ if (dev_priv->skl_sagv_enabled)
+ return 0;
+
+ mutex_lock(&dev_priv->rps.hw_lock);
+ DRM_DEBUG_KMS("Enabling the SAGV\n");
+
+ ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL,
+ GEN9_SAGV_DYNAMIC_FREQ);
+ if (!ret)
+ dev_priv->skl_sagv_enabled = true;
+ else
+ DRM_ERROR("Failed to enable the SAGV\n");
+
+ /* We don't need to wait for SAGV when enabling */
+ mutex_unlock(&dev_priv->rps.hw_lock);
+
Re: [Intel-gfx] [PATCH] drm/i915/gen9: Only copy WM results for changed pipes to skl_hw
Just posting this so it's on the ML.
NAK on this patch, this causes leftover cursors on SKL. Additionally, there's a
noticeable lag every time I move the cursor from one monitor to another, and the
flickering from the broken watermarks still happens.
On Thu, 2016-07-07 at 11:37 -0700, Matt Roper wrote:
> When we write watermark values to the hardware, those values are stored
> in dev_priv->wm.skl_hw. However with recent watermark changes, the
> results structure we're copying from only contains valid watermark and
> DDB values for the pipes that are actually changing; the values for
> other pipes remain 0. Thus a blind copy of the entire skl_wm_values
> structure will clobber the values for unchanged pipes...we need to be
> more selective and only copy over the values for the changing pipes.
>
> This mistake was hidden until recently due to another bug that caused us
> to erroneously re-calculate watermarks for all active pipes rather than
> changing pipes. Only when that bug was fixed was the impact of this bug
> discovered (e.g., modesets failing with "Requested display configuration
> exceeds system watermark limitations" messages and leaving watermarks
> non-functional, even ones initiated by intel_fbdev_restore_mode).
>
> Cc: Maarten Lankhorst
> Cc: Lyude Paul
> Fixes: 734fa01f3a17 ("drm/i915/gen9: Calculate watermarks during atomic
> 'check' (v2)")
> Fixes: 9b6130227495 ("drm/i915/gen9: Re-allocate DDB only for changed pipes")
> Signed-off-by: Matt Roper
> ---
> drivers/gpu/drm/i915/intel_pm.c | 22 --
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 5a8ee0c..2913535 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4018,8 +4018,10 @@ static void skl_update_wm(struct drm_crtc *crtc)
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct skl_wm_values *results = &dev_priv->wm.skl_results;
> + struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw;
> struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> + int pipe;
>
> if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
> return;
> @@ -4031,8 +4033,24 @@ static void skl_update_wm(struct drm_crtc *crtc)
> skl_write_wm_values(dev_priv, results);
> skl_flush_wm_values(dev_priv, results);
>
> - /* store the new configuration */
> - dev_priv->wm.skl_hw = *results;
> + /*
> + * Store the new configuration (but only for the pipes that have
> + * changed; the other values weren't recomputed).
> + */
> + for_each_pipe_masked(dev_priv, pipe, results->dirty_pipes) {
> + hw_vals->wm_linetime[pipe] = results->wm_linetime[pipe];
> + hw_vals->ddb.pipe[pipe] = results->ddb.pipe[pipe];
> + memcpy(&hw_vals->ddb.plane[pipe], &results->ddb.plane[pipe],
> + sizeof(hw_vals->ddb.plane[pipe]));
> + memcpy(&hw_vals->ddb.y_plane[pipe], &results-
> >ddb.y_plane[pipe],
> + sizeof(hw_vals->ddb.y_plane[pipe]));
> + memcpy(&hw_vals->plane[pipe],
> + &results->plane[pipe],
> + sizeof(results->plane[pipe]));
> + memcpy(&hw_vals->plane_trans[pipe],
> + &results->plane_trans[pipe],
> + sizeof(results->plane_trans[pipe]));
> + }
>
> mutex_unlock(&dev_priv->wm.wm_mutex);
> }
--
Cheers,
Lyude
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: intel_dp_check_link_status should only return status of link
On Fri, Jul 01, 2016 at 05:35:25PM -0700, Rodrigo Vivi wrote:
> On Fri, Jul 1, 2016 at 3:47 PM, Manasi Navare
> wrote:
> > Intel_dp_check_link_status() function reads the Link status registers
> > and returns a boolean to indicate link is good or bad.
> > If the link is bad, it is retrained outside the function based
> > on the return value.
> >
> > Signed-off-by: Manasi Navare
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 41
> > -
> > 1 file changed, 24 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 6d586b7..c795c9e 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3863,7 +3863,7 @@ go_again:
> > return -EINVAL;
> > }
> >
> > -static void
> > +static bool
> > intel_dp_check_link_status(struct intel_dp *intel_dp)
> > {
> > struct intel_encoder *intel_encoder =
> > &dp_to_dig_port(intel_dp)->base;
> > @@ -3873,26 +3873,25 @@ intel_dp_check_link_status(struct intel_dp
> > *intel_dp)
> > WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> >
> > if (!intel_dp_get_link_status(intel_dp, link_status)) {
> > - DRM_ERROR("Failed to get link status\n");
> > - return;
> > + DRM_DEBUG_KMS("Failed to get link status\n");
> > + return false;
> > }
> >
> > - if (!intel_encoder->base.crtc)
> > - return;
>
> where did this check go? why don't we need this anymore?
>
This function is being refactored to handle upront link training independent
of modeset and according to the spec, this function should only read the DPCD
registers
to check if the link is valid. In case of upfront link training, we do not care
about
the crtc or if the crtc is active hence this and the next check for crtc acrive
are
not needed.
> > + /* Check if the link is valid by reading the bits of Link status
> > +* registers
> > +*/
> > + if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count) ||
> > + !drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
> > + DRM_DEBUG_KMS("Channel EQ or CR not ok, need to retrain\n");
> > + return false;
> > + }
> >
> > - if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> > - return;
>
> what happens if it is not active now? are we going to attemtp the
> retraing in this case?
> in other words: we really don't need this check anymore as the one above?
>
> Thanks,
> Rodrigo.
>
> > + DRM_DEBUG_KMS("Link is good, no need to retrain\n");
> > + return true;
> >
> > - /* if link training is requested we should perform it always */
> > - if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
> > - (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
> > - DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
> > - intel_encoder->base.name);
> > - intel_dp_start_link_train(intel_dp);
> > - intel_dp_stop_link_train(intel_dp);
> > - }
> > }
> >
> > +
> > /*
> > * According to DP spec
> > * 5.1.2:
> > @@ -3950,7 +3949,11 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
> > }
> >
> > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > - intel_dp_check_link_status(intel_dp);
> > + if (!intel_dp_check_link_status(intel_dp) ||
> > + intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) {
> > + intel_dp_start_link_train(intel_dp);
> > + intel_dp_stop_link_train(intel_dp);
> > + }
> > drm_modeset_unlock(&dev->mode_config.connection_mutex);
> >
> > return true;
> > @@ -4271,7 +4274,11 @@ intel_dp_long_pulse(struct intel_connector
> > *intel_connector)
> > * link loss triggerring long pulse
> > */
> > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > - intel_dp_check_link_status(intel_dp);
> > + if (!intel_dp_check_link_status(intel_dp) ||
> > + intel_dp->compliance_test_type ==
> > DP_TEST_LINK_TRAINING) {
> > + intel_dp_start_link_train(intel_dp);
> > + intel_dp_stop_link_train(intel_dp);
> > + }
> > drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > goto out;
> > }
> > --
> > 1.9.1
> >
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop
Re: [Intel-gfx] [PATCH] drm/i915: intel_dp_check_link_status should only return status of link
On Sat, Jul 02, 2016 at 11:29:05AM +0200, Lukas Wunner wrote:
> On Fri, Jul 01, 2016 at 03:47:56PM -0700, Manasi Navare wrote:
> > Intel_dp_check_link_status() function reads the Link status registers
> > and returns a boolean to indicate link is good or bad.
> > If the link is bad, it is retrained outside the function based
> > on the return value.
> >
> > Signed-off-by: Manasi Navare
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 41
> > -
> > 1 file changed, 24 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 6d586b7..c795c9e 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3863,7 +3863,7 @@ go_again:
> > return -EINVAL;
> > }
> >
> > -static void
> > +static bool
> > intel_dp_check_link_status(struct intel_dp *intel_dp)
> > {
>
> A common pattern is to name bool functions like a yes/no question,
> so you might want to adjust the name to something like
> intel_dp_link_status_ok() or intel_dp_link_is_good() here.
>
> Best regards,
>
> Lukas
I will change the name to follow this naming pattern.
Regards
Manasi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: intel_dp_link_is_valid() should only return status of link
Intel_dp_link_is_valid() function reads the Link status registers
and returns a boolean to indicate link is valid or not.
If the link has lost lock and is not valid any more, it is
retrained outside the function else no previously trained link
is retained.
v2: Changed the function name from intel_dp_check_link_status()
to intel_dp_link_is_valid() (Lukas Wunner)
Signed-off-by: Manasi Navare
---
drivers/gpu/drm/i915/intel_dp.c | 44 +++--
1 file changed, 25 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0c5ba34..5b3e355 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3862,36 +3862,34 @@ go_again:
return -EINVAL;
}
-static void
-intel_dp_check_link_status(struct intel_dp *intel_dp)
+static bool
+intel_dp_link_is_valid(struct intel_dp *intel_dp)
{
- struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
struct drm_device *dev = intel_dp_to_dev(intel_dp);
u8 link_status[DP_LINK_STATUS_SIZE];
WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
if (!intel_dp_get_link_status(intel_dp, link_status)) {
- DRM_ERROR("Failed to get link status\n");
- return;
+ DRM_DEBUG_KMS("Failed to get link status\n");
+ return false;
}
- if (!intel_encoder->base.crtc)
- return;
+ /* Check if the link is valid by reading the bits of Link status
+* registers
+*/
+ if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count) ||
+ !drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
+ DRM_DEBUG_KMS("Channel EQ or CR not ok, need to retrain\n");
+ return false;
+ }
- if (!to_intel_crtc(intel_encoder->base.crtc)->active)
- return;
+ DRM_DEBUG_KMS("Link is good, no need to retrain\n");
+ return true;
- /* if link training is requested we should perform it always */
- if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
- (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
- DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
- intel_encoder->base.name);
- intel_dp_start_link_train(intel_dp);
- intel_dp_stop_link_train(intel_dp);
- }
}
+
/*
* According to DP spec
* 5.1.2:
@@ -3949,7 +3947,11 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
}
drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
- intel_dp_check_link_status(intel_dp);
+ if (!intel_dp_link_is_valid(intel_dp) ||
+ intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) {
+ intel_dp_start_link_train(intel_dp);
+ intel_dp_stop_link_train(intel_dp);
+ }
drm_modeset_unlock(&dev->mode_config.connection_mutex);
return true;
@@ -4270,7 +4272,11 @@ intel_dp_long_pulse(struct intel_connector
*intel_connector)
* link loss triggerring long pulse
*/
drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
- intel_dp_check_link_status(intel_dp);
+ if (!intel_dp_link_is_valid(intel_dp) ||
+ intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) {
+ intel_dp_start_link_train(intel_dp);
+ intel_dp_stop_link_train(intel_dp);
+ }
drm_modeset_unlock(&dev->mode_config.connection_mutex);
goto out;
}
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ Ro.CI.BAT: failure for drm/i915/guc: symbolic names for user load/submission preferences
== Series Details == Series: drm/i915/guc: symbolic names for user load/submission preferences URL : https://patchwork.freedesktop.org/series/9724/ State : failure == Summary == Series 9724v1 drm/i915/guc: symbolic names for user load/submission preferences http://patchwork.freedesktop.org/api/1.0/series/9724/revisions/1/mbox Test drv_module_reload_basic: pass -> DMESG-WARN (ro-bdw-i7-5600u) Test gem_sync: Subgroup basic-store-each: pass -> DMESG-FAIL (ro-bdw-i7-5600u) fi-kbl-qkkr total:237 pass:168 dwarn:27 dfail:2 fail:4 skip:36 fi-skl-i5-6260u total:237 pass:212 dwarn:0 dfail:1 fail:4 skip:20 fi-skl-i7-6700k total:237 pass:198 dwarn:0 dfail:1 fail:4 skip:34 fi-snb-i7-2600 total:237 pass:184 dwarn:0 dfail:1 fail:4 skip:48 ro-bdw-i5-5250u total:237 pass:207 dwarn:1 dfail:1 fail:4 skip:24 ro-bdw-i7-5557U total:237 pass:207 dwarn:1 dfail:1 fail:4 skip:24 ro-bdw-i7-5600u total:237 pass:191 dwarn:1 dfail:2 fail:4 skip:39 ro-bsw-n3050 total:217 pass:170 dwarn:0 dfail:0 fail:4 skip:42 ro-byt-n2820 total:237 pass:183 dwarn:0 dfail:1 fail:7 skip:46 ro-hsw-i3-4010u total:237 pass:200 dwarn:0 dfail:1 fail:4 skip:32 ro-hsw-i7-4770r total:237 pass:200 dwarn:0 dfail:1 fail:4 skip:32 ro-ilk-i7-620lm total:237 pass:160 dwarn:0 dfail:1 fail:5 skip:71 ro-ilk1-i5-650 total:232 pass:160 dwarn:0 dfail:1 fail:5 skip:66 ro-ivb-i7-3770 total:237 pass:191 dwarn:0 dfail:1 fail:4 skip:41 ro-skl3-i5-6260u total:237 pass:211 dwarn:1 dfail:1 fail:4 skip:20 ro-snb-i7-2620M total:237 pass:182 dwarn:0 dfail:1 fail:5 skip:49 Results at /archive/results/CI_IGT_test/RO_Patchwork_1467/ e549c0b drm-intel-nightly: 2016y-07m-11d-12h-49m-29s UTC integration manifest 99c8b84 drm/i915/guc: symbolic names for user load/submission preferences ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ Ro.CI.BAT: failure for drm/i915/skl: Add support for the SAGV, fix underrun hangs
== Series Details == Series: drm/i915/skl: Add support for the SAGV, fix underrun hangs URL : https://patchwork.freedesktop.org/series/9736/ State : failure == Summary == Series 9736v1 drm/i915/skl: Add support for the SAGV, fix underrun hangs http://patchwork.freedesktop.org/api/1.0/series/9736/revisions/1/mbox Test drv_module_reload_basic: pass -> SKIP (fi-skl-i5-6260u) Test gem_exec_flush: Subgroup basic-wb-set-default: pass -> INCOMPLETE (ro-byt-n2820) Test gem_sync: Subgroup basic-many-each: pass -> TIMEOUT(ro-byt-n2820) Subgroup basic-store-each: pass -> DMESG-FAIL (ro-bdw-i7-5600u) Test kms_pipe_crc_basic: Subgroup suspend-read-crc-pipe-a: skip -> DMESG-WARN (ro-bdw-i7-5557U) Subgroup suspend-read-crc-pipe-b: skip -> DMESG-WARN (ro-bdw-i7-5557U) fi-kbl-qkkr total:237 pass:168 dwarn:29 dfail:1 fail:4 skip:35 fi-skl-i5-6260u total:237 pass:211 dwarn:0 dfail:1 fail:4 skip:21 fi-skl-i7-6700k total:237 pass:198 dwarn:0 dfail:1 fail:4 skip:34 fi-snb-i7-2600 total:237 pass:184 dwarn:0 dfail:1 fail:4 skip:48 ro-bdw-i5-5250u total:237 pass:207 dwarn:1 dfail:1 fail:4 skip:24 ro-bdw-i7-5557U total:237 pass:207 dwarn:3 dfail:1 fail:4 skip:22 ro-bdw-i7-5600u total:237 pass:192 dwarn:0 dfail:2 fail:4 skip:39 ro-bsw-n3050 total:218 pass:170 dwarn:0 dfail:0 fail:5 skip:42 ro-byt-n2820 total:91 pass:63 dwarn:0 dfail:0 fail:3 skip:23 ro-hsw-i3-4010u total:237 pass:200 dwarn:0 dfail:1 fail:4 skip:32 ro-hsw-i7-4770r total:237 pass:200 dwarn:0 dfail:1 fail:4 skip:32 ro-ilk-i7-620lm total:237 pass:160 dwarn:0 dfail:1 fail:5 skip:71 ro-ilk1-i5-650 total:232 pass:160 dwarn:0 dfail:1 fail:5 skip:66 ro-ivb-i7-3770 total:237 pass:191 dwarn:0 dfail:1 fail:4 skip:41 ro-skl3-i5-6260u total:237 pass:211 dwarn:1 dfail:1 fail:4 skip:20 ro-snb-i7-2620M total:237 pass:182 dwarn:0 dfail:1 fail:5 skip:49 Results at /archive/results/CI_IGT_test/RO_Patchwork_1468/ e549c0b drm-intel-nightly: 2016y-07m-11d-12h-49m-29s UTC integration manifest a367104 drm/i915/skl: Add support for the SAGV, fix underrun hangs ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ Ro.CI.BAT: failure for drm/i915: intel_dp_link_is_valid() should only return status of link
== Series Details == Series: drm/i915: intel_dp_link_is_valid() should only return status of link URL : https://patchwork.freedesktop.org/series/9737/ State : failure == Summary == Series 9737v1 drm/i915: intel_dp_link_is_valid() should only return status of link http://patchwork.freedesktop.org/api/1.0/series/9737/revisions/1/mbox Test drv_module_reload_basic: pass -> DMESG-WARN (ro-bdw-i7-5600u) pass -> INCOMPLETE (ro-bdw-i7-5557U) Test kms_pipe_crc_basic: Subgroup suspend-read-crc-pipe-c: pass -> DMESG-WARN (ro-skl3-i5-6260u) ro-bdw-i5-5250u total:237 pass:207 dwarn:1 dfail:1 fail:4 skip:24 ro-bdw-i7-5557U total:29 pass:23 dwarn:0 dfail:0 fail:0 skip:5 ro-bdw-i7-5600u total:237 pass:192 dwarn:1 dfail:1 fail:4 skip:39 ro-bsw-n3050 total:237 pass:170 dwarn:0 dfail:0 fail:5 skip:42 ro-byt-n2820 total:237 pass:183 dwarn:0 dfail:1 fail:7 skip:46 ro-hsw-i3-4010u total:237 pass:200 dwarn:0 dfail:1 fail:4 skip:32 ro-hsw-i7-4770r total:237 pass:200 dwarn:0 dfail:1 fail:4 skip:32 ro-ilk-i7-620lm total:237 pass:160 dwarn:0 dfail:1 fail:5 skip:71 ro-ilk1-i5-650 total:232 pass:160 dwarn:0 dfail:1 fail:5 skip:66 ro-ivb-i7-3770 total:237 pass:191 dwarn:0 dfail:1 fail:4 skip:41 ro-skl3-i5-6260u total:237 pass:210 dwarn:2 dfail:1 fail:4 skip:20 ro-snb-i7-2620M total:237 pass:182 dwarn:0 dfail:1 fail:5 skip:49 fi-kbl-qkkr failed to connect after reboot fi-skl-i5-6260u failed to connect after reboot fi-skl-i7-6700k failed to connect after reboot fi-snb-i7-2600 failed to connect after reboot Results at /archive/results/CI_IGT_test/RO_Patchwork_1469/ e549c0b drm-intel-nightly: 2016y-07m-11d-12h-49m-29s UTC integration manifest f65b9f3 drm/i915: intel_dp_link_is_valid() should only return status of link ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
