Re: [Intel-gfx] [PATCH v7 9/9] drm/i915: Set PWM divider to match desired frequency in vbt
On Fri, May 12, 2017 at 5:12 PM, Pandiyan, Dhinakaran <
dhinakaran.pandi...@intel.com> wrote:
> On Thu, 2017-05-11 at 16:02 -0700, Puthikorn Voravootivat wrote:
> > Read desired PWM frequency from panel vbt and calculate the
> > value for divider in DPCD address 0x724 and 0x728 to have
> > as many bits as possible for PWM duty cyle for granularity of
> > brightness adjustment while the frequency is still within 25%
> > of the desired frequency.
>
> I read a few eDP panel data sheets, the PWM frequencies all start from
> ~200Hz. If the VBT chooses this lowest value to allow for more
> brightness control, and then this patch lowers the value by another 25%,
> we'll end up below the panel allowed PWM frequency.
>
> In fact, one of the systems I checked had PWM frequency as 200Hz in VBT
> and the panel datasheet also had PWM frequency range starting from
> 200Hz. Have you considered this case?
>
> The spec said "A given LCD panel typically has a limited range of
backlight frequency capability.
To limit the programmable frequency range, limitations are placed on the
allowable total divider ratio with the Sink device"
So I think it should be auto cap to 200Hz in this case.
> -DK
> >
> > Signed-off-by: Puthikorn Voravootivat
> > ---
> > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 81
> +++
> > 1 file changed, 81 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > index 0b48851013cc..6f10a2f1ab76 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > @@ -113,12 +113,86 @@ intel_dp_aux_set_dynamic_backlight_percent(struct
> intel_dp *intel_dp,
> > }
> > }
> >
> > +/*
> > + * Set PWM Frequency divider to match desired frequency in vbt.
> > + * The PWM Frequency is calculated as 27Mhz / (F x P).
> > + * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0
> of the
> > + * EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)
> > + * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of
> the
> > + * EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
> > + */
> > +static void intel_dp_aux_set_pwm_freq(struct intel_connector
> *connector)
> > +{
> > + struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > + struct intel_dp *intel_dp = enc_to_intel_dp(&connector->
> encoder->base);
> > + int freq, fxp, fxp_min, fxp_max, fxp_actual, f = 1;
> > + u8 pn, pn_min, pn_max;
> > +
> > + /* Find desired value of (F x P)
> > + * Note that, if F x P is out of supported range, the maximum
> value or
> > + * minimum value will applied automatically. So no need to check
> that.
> > + */
> > + freq = dev_priv->vbt.backlight.pwm_freq_hz;
> > + DRM_DEBUG_KMS("VBT defined backlight frequency %u Hz\n", freq);
> > + if (!freq) {
> > + DRM_DEBUG_KMS("Use panel default backlight frequency\n");
> > + return;
> > + }
> > +
> > + fxp = KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ) / freq;
> > +
> > + /* Use highest possible value of Pn for more granularity of
> brightness
> > + * adjustment while satifying the conditions below.
> > + * - Pn is in the range of Pn_min and Pn_max
> > + * - F is in the range of 1 and 255
> > + * - Effective frequency is within 25% of desired frequency.
> > + */
> > + if (drm_dp_dpcd_readb(&intel_dp->aux,
> > +DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min)
> != 1) {
> > + DRM_DEBUG_KMS("Failed to read pwmgen bit count cap min\n");
> > + return;
> > + }
> > + if (drm_dp_dpcd_readb(&intel_dp->aux,
> > +DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max)
> != 1) {
> > + DRM_DEBUG_KMS("Failed to read pwmgen bit count cap max\n");
> > + return;
> > + }
> > + pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > + pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > +
> > + fxp_min = fxp * 3 / 4;
> > + fxp_max = fxp * 5 / 4;
>
> You are allowing fxp between +/- 25% of the actual. This isn't same as
> the "Effective frequency is within 25% of desired frequency." right? The
> frequency can vary between -20% and +33%.
>
> You are right.
You want me to change commit message to reflect this or change the code to
match the commit message?
>
> > + if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
> > + DRM_DEBUG_KMS("VBT defined backlight frequency out of
> range\n");
> > + return;
> > + }
> > +
> > + for (pn = pn_max; pn > pn_min; pn--) {
> > + f = clamp(fxp >> pn, 1, 255);
> > + fxp_actual = f << pn;
> > + if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
> > + break;
> > + }
> > +
> > + if (drm_dp_dpcd_writeb(&intel_dp->aux,
> > +DP_EDP
Re: [Intel-gfx] [PATCH v7 9/9] drm/i915: Set PWM divider to match desired frequency in vbt
On Thu, 2017-05-11 at 16:02 -0700, Puthikorn Voravootivat wrote:
> Read desired PWM frequency from panel vbt and calculate the
> value for divider in DPCD address 0x724 and 0x728 to have
> as many bits as possible for PWM duty cyle for granularity of
> brightness adjustment while the frequency is still within 25%
> of the desired frequency.
I read a few eDP panel data sheets, the PWM frequencies all start from
~200Hz. If the VBT chooses this lowest value to allow for more
brightness control, and then this patch lowers the value by another 25%,
we'll end up below the panel allowed PWM frequency.
In fact, one of the systems I checked had PWM frequency as 200Hz in VBT
and the panel datasheet also had PWM frequency range starting from
200Hz. Have you considered this case?
-DK
>
> Signed-off-by: Puthikorn Voravootivat
> ---
> drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 81
> +++
> 1 file changed, 81 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index 0b48851013cc..6f10a2f1ab76 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -113,12 +113,86 @@ intel_dp_aux_set_dynamic_backlight_percent(struct
> intel_dp *intel_dp,
> }
> }
>
> +/*
> + * Set PWM Frequency divider to match desired frequency in vbt.
> + * The PWM Frequency is calculated as 27Mhz / (F x P).
> + * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0 of the
> + * EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)
> + * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of the
> + * EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
> + */
> +static void intel_dp_aux_set_pwm_freq(struct intel_connector *connector)
> +{
> + struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> + struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
> + int freq, fxp, fxp_min, fxp_max, fxp_actual, f = 1;
> + u8 pn, pn_min, pn_max;
> +
> + /* Find desired value of (F x P)
> + * Note that, if F x P is out of supported range, the maximum value or
> + * minimum value will applied automatically. So no need to check that.
> + */
> + freq = dev_priv->vbt.backlight.pwm_freq_hz;
> + DRM_DEBUG_KMS("VBT defined backlight frequency %u Hz\n", freq);
> + if (!freq) {
> + DRM_DEBUG_KMS("Use panel default backlight frequency\n");
> + return;
> + }
> +
> + fxp = KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ) / freq;
> +
> + /* Use highest possible value of Pn for more granularity of brightness
> + * adjustment while satifying the conditions below.
> + * - Pn is in the range of Pn_min and Pn_max
> + * - F is in the range of 1 and 255
> + * - Effective frequency is within 25% of desired frequency.
> + */
> + if (drm_dp_dpcd_readb(&intel_dp->aux,
> +DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min) != 1) {
> + DRM_DEBUG_KMS("Failed to read pwmgen bit count cap min\n");
> + return;
> + }
> + if (drm_dp_dpcd_readb(&intel_dp->aux,
> +DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max) != 1) {
> + DRM_DEBUG_KMS("Failed to read pwmgen bit count cap max\n");
> + return;
> + }
> + pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> + pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> +
> + fxp_min = fxp * 3 / 4;
> + fxp_max = fxp * 5 / 4;
You are allowing fxp between +/- 25% of the actual. This isn't same as
the "Effective frequency is within 25% of desired frequency." right? The
frequency can vary between -20% and +33%.
> + if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
> + DRM_DEBUG_KMS("VBT defined backlight frequency out of range\n");
> + return;
> + }
> +
> + for (pn = pn_max; pn > pn_min; pn--) {
> + f = clamp(fxp >> pn, 1, 255);
> + fxp_actual = f << pn;
> + if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
> + break;
> + }
> +
> + if (drm_dp_dpcd_writeb(&intel_dp->aux,
> +DP_EDP_PWMGEN_BIT_COUNT, pn) < 0) {
> + DRM_DEBUG_KMS("Failed to write aux pwmgen bit count\n");
> + return;
> + }
> + if (drm_dp_dpcd_writeb(&intel_dp->aux,
> +DP_EDP_BACKLIGHT_FREQ_SET, (u8) f) < 0) {
> + DRM_DEBUG_KMS("Failed to write aux backlight freq\n");
> + return;
> + }
> +}
> +
> static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
> {
> struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
> uint8_t dpcd_buf = 0;
> uint8_t new_dpcd_buf = 0;
> uint8_t edp_backlight_mode = 0;
> + bool freq_cap;
>
> if (drm_dp_dpcd_readb(&intel_d
[Intel-gfx] ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Compute the fw_domain id from the mask (rev2)
== Series Details == Series: series starting with [1/2] drm/i915: Compute the fw_domain id from the mask (rev2) URL : https://patchwork.freedesktop.org/series/24359/ State : warning == Summary == Series 24359v2 Series without cover letter https://patchwork.freedesktop.org/api/1.0/series/24359/revisions/2/mbox/ Test kms_force_connector_basic: Subgroup force-connector-state: pass -> SKIP (fi-snb-2520m) pass -> SKIP (fi-ivb-3520m) Subgroup force-edid: pass -> SKIP (fi-ivb-3520m) Subgroup force-load-detect: pass -> SKIP (fi-ivb-3520m) Subgroup prune-stale-modes: pass -> SKIP (fi-ivb-3520m) fi-bdw-gvtdvmtotal:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:434s fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:517s fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time:497s fi-byt-n2820 total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:484s fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:426s fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:421s fi-ivb-3520m total:278 pass:256 dwarn:0 dfail:0 fail:0 skip:22 time:495s fi-kbl-7500u total:278 pass:255 dwarn:5 dfail:0 fail:0 skip:18 time:466s fi-kbl-7560u total:278 pass:263 dwarn:5 dfail:0 fail:0 skip:10 time:578s fi-skl-6700hqtotal:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time:574s fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time:476s fi-skl-6770hqtotal:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:504s fi-skl-gvtdvmtotal:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time:439s fi-snb-2520m total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time:539s fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time:413s fi-bxt-t5700 failed to collect. IGT log at Patchwork_4689/fi-bxt-t5700/igt.log 38d26beef9f31fa7ca983563cf86d4131defd94a drm-tip: 2017y-05m-12d-13h-15m-01s UTC integration manifest e645e4f drm/i915: Keep the forcewake timer alive for 1ms past the most recent use 98e890f drm/i915: Compute the fw_domain id from the mask == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4689/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 10/11] drm/i915/skl: New ddb allocation algorithm
On Mon, May 08, 2017 at 05:19:01PM +0530, Mahesh Kumar wrote:
> This patch implements new DDB allocation algorithm as per HW team
> recommendation. This algo takecare of scenario where we allocate less DDB
> for the planes with lower relative pixel rate, but they require more DDB
> to work.
> It also takes care of enabling same watermark level for each
> plane in crtc, for efficient power saving.
>
> Changes since v1:
> - Rebase on top of Paulo's patch series
>
> Changes since v2:
> - Fix the for loop condition to enable WM
>
> Changes since v3:
> - Fix crash in cursor i-g-t reported by Maarten
> - Rebase after addressing Paulo's comments
> - Few other ULT fixes
> Changes since v4:
> - Rebase on drm-tip
> - Added separate function to enable WM levels
> Changes since v4:
> - Fix a crash identified in skl-6770HQ system
>
> Signed-off-by: Mahesh Kumar
> ---
> drivers/gpu/drm/i915/intel_pm.c | 253
>
> 1 file changed, 152 insertions(+), 101 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bcf5d2523e4a..92600cf42e12 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3573,13 +3573,41 @@ skl_ddb_calc_min(const struct intel_crtc_state
> *cstate, int num_active,
> minimum[PLANE_CURSOR] = skl_cursor_allocation(num_active);
> }
>
> +static void
> +skl_enable_plane_wm_levels(const struct drm_i915_private *dev_priv,
> +uint16_t plane_ddb,
> +uint16_t max_level,
> +struct skl_plane_wm *wm)
> +{
> + int level;
> + /*
> + * Now enable all levels in WM structure which can be enabled
> + * using current DDB allocation
> + */
> + for (level = ilk_wm_max_level(dev_priv); level >= 0; level--) {
> + struct skl_wm_level *level_wm = &wm->wm[level];
> +
> + if (level > max_level || level_wm->plane_res_b == 0
> + || level_wm->plane_res_l >= 31
> + || level_wm->plane_res_b >= plane_ddb) {
> + level_wm->plane_en = false;
> + level_wm->plane_res_b = 0;
> + level_wm->plane_res_l = 0;
> + } else {
> + level_wm->plane_en = true;
> + }
> + }
> +}
> +
> static int
> skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> - struct skl_ddb_allocation *ddb /* out */)
> + struct skl_ddb_allocation *ddb /* out */,
> + struct skl_pipe_wm *pipe_wm)
> {
> struct drm_atomic_state *state = cstate->base.state;
> struct drm_crtc *crtc = cstate->base.crtc;
> struct drm_device *dev = crtc->dev;
> + struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> enum pipe pipe = intel_crtc->pipe;
> struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
> @@ -3592,6 +3620,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> unsigned plane_data_rate[I915_MAX_PLANES] = {};
> unsigned plane_y_data_rate[I915_MAX_PLANES] = {};
> uint16_t total_min_blocks = 0;
> + uint16_t total_level_ddb;
> + int max_level, level;
>
> /* Clear the partitioning for disabled planes. */
> memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> @@ -3630,10 +3660,43 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> return-EINVAL;
> }
>
> - alloc_size -= total_min_blocks;
> - ddb->plane[pipe][PLANE_CURSOR].start = alloc->end -
> minimum[PLANE_CURSOR];
> + alloc_size -= minimum[PLANE_CURSOR];
> + ddb->plane[pipe][PLANE_CURSOR].start = alloc->end -
> + minimum[PLANE_CURSOR];
> ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
>
> + for (level = ilk_wm_max_level(dev_priv); level >= 0; level--) {
Why do we walk backwards here (max level down to 0)? Shouldn't we be
going the other direction so that we allocate blocks for WM0, then move
on to the higher levels until we eventually fail? Maybe I'm missing
something...
> + total_level_ddb = 0;
> + for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> + /*
> + * TODO: We should calculate watermark values for Y/UV
> + * plane both in case of NV12 format and use both values
> + * for ddb calculation. NV12 is disabled as of now, So
> + * using only single/UV plane value here.
> + */
> + struct skl_plane_wm *wm = &pipe_wm->planes[plane_id];
> + uint16_t plane_res_b = wm->wm[level].plane_res_b;
> + uint16_t min = minimum[plane_id] + y_minimum[plane_id];
> +
> + if (plane_id
[Intel-gfx] [PATCH v2] drm/i915: Keep the forcewake timer alive for 1ms past the most recent use
Currently the timer is armed for 1ms after the first use and is killed
immediately, dropping the forcewake as early as possible. However, for
very frequent operations the forcewake dance has a large impact on
latency and keeping the timer alive until we are idle is preferred. To
achieve this, if we call intel_uncore_forcewake_get whilst the timer is
alive (repeated use), then set a flag to restart the timer on expiry
rather than drop the forcewake usage count. The timer is racy, the
consequence of the race is to expire the timer earlier than is now
desired but does not impact on correct behaviour. The offset the race
slightly, we set the active flag again on intel_uncore_forcewake_put.
Signed-off-by: Chris Wilson
Cc: Mika Kuoppala
Cc: Tvrtko Ursulin
---
drivers/gpu/drm/i915/intel_uncore.c | 15 ---
drivers/gpu/drm/i915/intel_uncore.h | 1 +
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c
b/drivers/gpu/drm/i915/intel_uncore.c
index 7eaa592aed26..2fd0989805eb 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -216,6 +216,9 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
assert_rpm_device_not_suspended(dev_priv);
+ if (xchg(&domain->active, false))
+ return HRTIMER_RESTART;
+
spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
if (WARN_ON(domain->wake_count == 0))
domain->wake_count++;
@@ -246,6 +249,7 @@ static void intel_uncore_forcewake_reset(struct
drm_i915_private *dev_priv,
active_domains = 0;
for_each_fw_domain(domain, dev_priv, tmp) {
+ smp_store_mb(domain->active, false);
if (hrtimer_cancel(&domain->timer) == 0)
continue;
@@ -453,9 +457,12 @@ static void __intel_uncore_forcewake_get(struct
drm_i915_private *dev_priv,
fw_domains &= dev_priv->uncore.fw_domains;
- for_each_fw_domain_masked(domain, fw_domains, dev_priv, tmp)
- if (domain->wake_count++)
+ for_each_fw_domain_masked(domain, fw_domains, dev_priv, tmp) {
+ if (domain->wake_count++) {
fw_domains &= ~domain->mask;
+ domain->active = true;
+ }
+ }
if (fw_domains)
dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_domains);
@@ -520,8 +527,10 @@ static void __intel_uncore_forcewake_put(struct
drm_i915_private *dev_priv,
if (WARN_ON(domain->wake_count == 0))
continue;
- if (--domain->wake_count)
+ if (--domain->wake_count) {
+ domain->active = true;
continue;
+ }
fw_domain_arm_timer(domain);
}
diff --git a/drivers/gpu/drm/i915/intel_uncore.h
b/drivers/gpu/drm/i915/intel_uncore.h
index 5fec5fd4346c..dfead585835c 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -95,6 +95,7 @@ struct intel_uncore {
struct intel_uncore_forcewake_domain {
unsigned int mask;
unsigned int wake_count;
+ bool active;
struct hrtimer timer;
i915_reg_t reg_set;
i915_reg_t reg_ack;
--
2.11.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v7 02/20] drm/i915: Modify error handler for per engine hang recovery
On 5/12/2017 2:09 PM, Chris Wilson wrote: On Fri, May 12, 2017 at 01:55:11PM -0700, Michel Thierry wrote: On 5/8/2017 11:31 AM, Michel Thierry wrote: On 4/29/2017 7:19 AM, Chris Wilson wrote: On Thu, Apr 27, 2017 at 04:12:42PM -0700, Michel Thierry wrote: From: Arun Siluvery ... +} + intel_prepare_reset(dev_priv); set_bit(I915_RESET_HANDOFF, &dev_priv->gpu_error.flags); @@ -2680,6 +2706,7 @@ static void i915_reset_and_wakeup(struct drm_i915_private *dev_priv) kobject_uevent_env(kobj, KOBJ_CHANGE, reset_done_event); +finish: /* * Note: The wake_up also serves as a memory barrier so that * waiters see the updated value of the dev_priv->gpu_error. @@ -2781,7 +2808,7 @@ void i915_handle_error(struct drm_i915_private *dev_priv, &dev_priv->gpu_error.flags)) goto out; -i915_reset_and_wakeup(dev_priv); +i915_reset_and_wakeup(dev_priv, engine_mask); ? You don't need to wakeup the struct_mutex so we don't need this after per-engine resets. Time to split up i915_reset_and_wakeup(), because we certainly shouldn't be calling intel_finish_reset() without first calling intel_prepare_reset(). Which is right here in my tree... Looking at your tree, it wouldn't call finish_reset there either, only these two are called after a successful reset: finish: clear_bit(I915_RESET_BACKOFF, &dev_priv->gpu_error.flags); wake_up_all(&dev_priv->gpu_error.reset_queue); But you're right, we only need to clear the error flag, no need to call wake_up_all. Should I move the per-engine reset to i915_handle_error, and then leave i915_reset_and_wakeup just for full resets? That would also make the promotion from per-engine to global look a bit 'clearer'. I just noticed an issue if I don't call wake_up_all. There can be someone else waiting for the reset to complete (i915_mutex_lock_interruptible -> i915_gem_wait_for_error). I915_RESET_BACKOFF has/had 2 roles, stop any other user to grab the struct mutex (which we won't need in reset-engine) and prevent two concurrent reset attempts (which we still want). Time to add a new flag for the later? (I915_RESET_ENGINE_IN_PROGRESS?) Yes, that would be a good idea to avoid dual purposing the bits. Now that we do direct resets along the wait path, we can completely drop the i915_mutex_interruptible(). (No one else should be holding the mutex indefinitely.) I think that's a better approach -- I think we've already moved all the EIO magic aware to the ABI points where we deemed it necessary. And it seems to work ok with the new flag and no wake_up. I'll run more tests. Thanks ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v7 02/20] drm/i915: Modify error handler for per engine hang recovery
On Fri, May 12, 2017 at 01:55:11PM -0700, Michel Thierry wrote: > On 5/8/2017 11:31 AM, Michel Thierry wrote: > >On 4/29/2017 7:19 AM, Chris Wilson wrote: > >>On Thu, Apr 27, 2017 at 04:12:42PM -0700, Michel Thierry wrote: > >>>From: Arun Siluvery > >>> > ... > >>>+} > >>>+ > >>> intel_prepare_reset(dev_priv); > >>> > >>> set_bit(I915_RESET_HANDOFF, &dev_priv->gpu_error.flags); > >>>@@ -2680,6 +2706,7 @@ static void i915_reset_and_wakeup(struct > >>>drm_i915_private *dev_priv) > >>> kobject_uevent_env(kobj, > >>>KOBJ_CHANGE, reset_done_event); > >>> > >>>+finish: > >>> /* > >>> * Note: The wake_up also serves as a memory barrier so that > >>> * waiters see the updated value of the dev_priv->gpu_error. > >>>@@ -2781,7 +2808,7 @@ void i915_handle_error(struct drm_i915_private > >>>*dev_priv, > >>> &dev_priv->gpu_error.flags)) > >>> goto out; > >>> > >>>-i915_reset_and_wakeup(dev_priv); > >>>+i915_reset_and_wakeup(dev_priv, engine_mask); > >> > >>? You don't need to wakeup the struct_mutex so we don't need this after > >>per-engine resets. Time to split up i915_reset_and_wakeup(), because we > >>certainly shouldn't be calling intel_finish_reset() without first calling > >>intel_prepare_reset(). Which is right here in my tree... > >> > > > >Looking at your tree, it wouldn't call finish_reset there either, only > >these two are called after a successful reset: > > > >finish: > >clear_bit(I915_RESET_BACKOFF, &dev_priv->gpu_error.flags); > >wake_up_all(&dev_priv->gpu_error.reset_queue); > > > >But you're right, we only need to clear the error flag, no need to call > >wake_up_all. > > > >Should I move the per-engine reset to i915_handle_error, and then leave > >i915_reset_and_wakeup just for full resets? > >That would also make the promotion from per-engine to global look a bit > >'clearer'. > > > > I just noticed an issue if I don't call wake_up_all. There can be > someone else waiting for the reset to complete > (i915_mutex_lock_interruptible -> i915_gem_wait_for_error). > > I915_RESET_BACKOFF has/had 2 roles, stop any other user to grab the > struct mutex (which we won't need in reset-engine) and prevent two > concurrent reset attempts (which we still want). Time to add a new > flag for the later? (I915_RESET_ENGINE_IN_PROGRESS?) Yes, that would be a good idea to avoid dual purposing the bits. Now that we do direct resets along the wait path, we can completely drop the i915_mutex_interruptible(). (No one else should be holding the mutex indefinitely.) I think that's a better approach -- I think we've already moved all the EIO magic aware to the ABI points where we deemed it necessary. -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 v7 02/20] drm/i915: Modify error handler for per engine hang recovery
On 5/8/2017 11:31 AM, Michel Thierry wrote: On 4/29/2017 7:19 AM, Chris Wilson wrote: On Thu, Apr 27, 2017 at 04:12:42PM -0700, Michel Thierry wrote: From: Arun Siluvery ... +} + intel_prepare_reset(dev_priv); set_bit(I915_RESET_HANDOFF, &dev_priv->gpu_error.flags); @@ -2680,6 +2706,7 @@ static void i915_reset_and_wakeup(struct drm_i915_private *dev_priv) kobject_uevent_env(kobj, KOBJ_CHANGE, reset_done_event); +finish: /* * Note: The wake_up also serves as a memory barrier so that * waiters see the updated value of the dev_priv->gpu_error. @@ -2781,7 +2808,7 @@ void i915_handle_error(struct drm_i915_private *dev_priv, &dev_priv->gpu_error.flags)) goto out; -i915_reset_and_wakeup(dev_priv); +i915_reset_and_wakeup(dev_priv, engine_mask); ? You don't need to wakeup the struct_mutex so we don't need this after per-engine resets. Time to split up i915_reset_and_wakeup(), because we certainly shouldn't be calling intel_finish_reset() without first calling intel_prepare_reset(). Which is right here in my tree... Looking at your tree, it wouldn't call finish_reset there either, only these two are called after a successful reset: finish: clear_bit(I915_RESET_BACKOFF, &dev_priv->gpu_error.flags); wake_up_all(&dev_priv->gpu_error.reset_queue); But you're right, we only need to clear the error flag, no need to call wake_up_all. Should I move the per-engine reset to i915_handle_error, and then leave i915_reset_and_wakeup just for full resets? That would also make the promotion from per-engine to global look a bit 'clearer'. I just noticed an issue if I don't call wake_up_all. There can be someone else waiting for the reset to complete (i915_mutex_lock_interruptible -> i915_gem_wait_for_error). I915_RESET_BACKOFF has/had 2 roles, stop any other user to grab the struct mutex (which we won't need in reset-engine) and prevent two concurrent reset attempts (which we still want). Time to add a new flag for the later? (I915_RESET_ENGINE_IN_PROGRESS?) Here's an example without calling wake_up_all (10s timeout): [ 126.816054] [drm:i915_reset_engine [i915]] resetting rcs0 ... [ 137.499910] [IGT] gem_ringfill: exiting, ret=0 Compared to the one that does, [ 69.799519] [drm:i915_reset_engine [i915]] resetting rcs0 ... [ 69.801335] [IGT] gem_tdr: exiting, ret=0 Thanks, -Michel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Fix some tracepoints to capture full 64b
== Series Details == Series: series starting with [1/2] drm/i915: Fix some tracepoints to capture full 64b URL : https://patchwork.freedesktop.org/series/24374/ State : success == Summary == Series 24374v1 Series without cover letter https://patchwork.freedesktop.org/api/1.0/series/24374/revisions/1/mbox/ fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time:446s fi-bdw-gvtdvmtotal:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:440s fi-bsw-n3050 total:278 pass:242 dwarn:0 dfail:0 fail:0 skip:36 time:581s fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:518s fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time:495s fi-byt-n2820 total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:482s fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:415s fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:412s fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:429s fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:499s fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:467s fi-kbl-7500u total:278 pass:255 dwarn:5 dfail:0 fail:0 skip:18 time:463s fi-kbl-7560u total:278 pass:263 dwarn:5 dfail:0 fail:0 skip:10 time:570s fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:456s fi-skl-6700hqtotal:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time:576s fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time:470s fi-skl-6770hqtotal:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:508s fi-skl-gvtdvmtotal:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time:441s fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:541s fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time:416s fi-bxt-t5700 failed to collect. IGT log at Patchwork_4688/fi-bxt-t5700/igt.log 38d26beef9f31fa7ca983563cf86d4131defd94a drm-tip: 2017y-05m-12d-13h-15m-01s UTC integration manifest dae55a3 drm/i915: Remove defunct trace points f0ea043 drm/i915: Fix some tracepoints to capture full 64b == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4688/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Exclude top-page for ppgtt as well as ggtt
On 12/05/17 12:06, Chris Wilson wrote: On Fri, May 12, 2017 at 06:55:35PM +0100, Chris Wilson wrote: On Fri, May 12, 2017 at 06:49:09PM +0100, Chris Wilson wrote: We have always excluded the top-page of the Global GTT to prevent prefetching past the end of the address space. We have been lax about applying this restriction to the per-process GTT, but there is no reason to believe that the hw restriction is any less severe. Other than empirical, but the note in bspec is still there and who knows they may start to be strict again in future. Fwiw, it looks like we can sleep on this again. Jason no longer believes this to be an immediate issue. -Chris It might be true for 32b ppgtt, but anything slightly recent should already be using 48b. -Michel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: Remove defunct trace points
trace_i915_gem_evict_everything and trace_i915_gem_ring_flush stopped
being used when their parent functions were removed.
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/i915_trace.h | 72 +--
1 file changed, 17 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_trace.h
b/drivers/gpu/drm/i915/i915_trace.h
index 16f2b03ff1c8..1d76631221b7 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -539,38 +539,6 @@ TRACE_EVENT(i915_gem_evict,
__entry->flags & PIN_MAPPABLE ? ", mappable" : "")
);
-TRACE_EVENT(i915_gem_evict_everything,
- TP_PROTO(struct drm_device *dev),
- TP_ARGS(dev),
-
- TP_STRUCT__entry(
-__field(u32, dev)
- ),
-
- TP_fast_assign(
- __entry->dev = dev->primary->index;
- ),
-
- TP_printk("dev=%d", __entry->dev)
-);
-
-TRACE_EVENT(i915_gem_evict_vm,
- TP_PROTO(struct i915_address_space *vm),
- TP_ARGS(vm),
-
- TP_STRUCT__entry(
-__field(u32, dev)
-__field(struct i915_address_space *, vm)
- ),
-
- TP_fast_assign(
- __entry->dev = vm->i915->drm.primary->index;
- __entry->vm = vm;
- ),
-
- TP_printk("dev=%d, vm=%p", __entry->dev, __entry->vm)
-);
-
TRACE_EVENT(i915_gem_evict_node,
TP_PROTO(struct i915_address_space *vm, struct drm_mm_node *node,
unsigned int flags),
TP_ARGS(vm, node, flags),
@@ -599,6 +567,23 @@ TRACE_EVENT(i915_gem_evict_node,
__entry->color, __entry->flags)
);
+TRACE_EVENT(i915_gem_evict_vm,
+ TP_PROTO(struct i915_address_space *vm),
+ TP_ARGS(vm),
+
+ TP_STRUCT__entry(
+__field(u32, dev)
+__field(struct i915_address_space *, vm)
+ ),
+
+ TP_fast_assign(
+ __entry->dev = vm->i915->drm.primary->index;
+ __entry->vm = vm;
+ ),
+
+ TP_printk("dev=%d, vm=%p", __entry->dev, __entry->vm)
+);
+
TRACE_EVENT(i915_gem_ring_sync_to,
TP_PROTO(struct drm_i915_gem_request *to,
struct drm_i915_gem_request *from),
@@ -649,29 +634,6 @@ TRACE_EVENT(i915_gem_request_queue,
__entry->flags)
);
-TRACE_EVENT(i915_gem_ring_flush,
- TP_PROTO(struct drm_i915_gem_request *req, u32 invalidate, u32
flush),
- TP_ARGS(req, invalidate, flush),
-
- TP_STRUCT__entry(
-__field(u32, dev)
-__field(u32, ring)
-__field(u32, invalidate)
-__field(u32, flush)
-),
-
- TP_fast_assign(
- __entry->dev = req->i915->drm.primary->index;
- __entry->ring = req->engine->id;
- __entry->invalidate = invalidate;
- __entry->flush = flush;
- ),
-
- TP_printk("dev=%u, ring=%x, invalidate=%04x, flush=%04x",
- __entry->dev, __entry->ring,
- __entry->invalidate, __entry->flush)
-);
-
DECLARE_EVENT_CLASS(i915_gem_request,
TP_PROTO(struct drm_i915_gem_request *req),
TP_ARGS(req),
--
2.11.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] drm/i915: Fix some tracepoints to capture full 64b
The tracepoints need some tlc, in particular we've neglected to update
them for the 64b era.
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/i915_trace.h | 42 +++
1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_trace.h
b/drivers/gpu/drm/i915/i915_trace.h
index b24a83d43559..16f2b03ff1c8 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -345,7 +345,7 @@ TRACE_EVENT(i915_gem_object_create,
TP_STRUCT__entry(
__field(struct drm_i915_gem_object *, obj)
-__field(u32, size)
+__field(u64, size)
),
TP_fast_assign(
@@ -353,7 +353,7 @@ TRACE_EVENT(i915_gem_object_create,
__entry->size = obj->base.size;
),
- TP_printk("obj=%p, size=%u", __entry->obj, __entry->size)
+ TP_printk("obj=%p, size=%llx", __entry->obj, __entry->size)
);
TRACE_EVENT(i915_gem_shrink,
@@ -384,7 +384,7 @@ TRACE_EVENT(i915_vma_bind,
__field(struct drm_i915_gem_object *, obj)
__field(struct i915_address_space *, vm)
__field(u64, offset)
-__field(u32, size)
+__field(u64, size)
__field(unsigned, flags)
),
@@ -396,7 +396,7 @@ TRACE_EVENT(i915_vma_bind,
__entry->flags = flags;
),
- TP_printk("obj=%p, offset=%016llx size=%x%s vm=%p",
+ TP_printk("obj=%p, offset=%016llx size=%llx%s vm=%p",
__entry->obj, __entry->offset, __entry->size,
__entry->flags & PIN_MAPPABLE ? ", mappable" : "",
__entry->vm)
@@ -410,7 +410,7 @@ TRACE_EVENT(i915_vma_unbind,
__field(struct drm_i915_gem_object *, obj)
__field(struct i915_address_space *, vm)
__field(u64, offset)
-__field(u32, size)
+__field(u64, size)
),
TP_fast_assign(
@@ -420,18 +420,18 @@ TRACE_EVENT(i915_vma_unbind,
__entry->size = vma->node.size;
),
- TP_printk("obj=%p, offset=%016llx size=%x vm=%p",
+ TP_printk("obj=%p, offset=%016llx size=%llx vm=%p",
__entry->obj, __entry->offset, __entry->size, __entry->vm)
);
TRACE_EVENT(i915_gem_object_pwrite,
- TP_PROTO(struct drm_i915_gem_object *obj, u32 offset, u32 len),
+ TP_PROTO(struct drm_i915_gem_object *obj, u64 offset, u64 len),
TP_ARGS(obj, offset, len),
TP_STRUCT__entry(
__field(struct drm_i915_gem_object *, obj)
-__field(u32, offset)
-__field(u32, len)
+__field(u64, offset)
+__field(u64, len)
),
TP_fast_assign(
@@ -440,18 +440,18 @@ TRACE_EVENT(i915_gem_object_pwrite,
__entry->len = len;
),
- TP_printk("obj=%p, offset=%u, len=%u",
+ TP_printk("obj=%p, offset=%llx, len=%llx",
__entry->obj, __entry->offset, __entry->len)
);
TRACE_EVENT(i915_gem_object_pread,
- TP_PROTO(struct drm_i915_gem_object *obj, u32 offset, u32 len),
+ TP_PROTO(struct drm_i915_gem_object *obj, u64 offset, u64 len),
TP_ARGS(obj, offset, len),
TP_STRUCT__entry(
__field(struct drm_i915_gem_object *, obj)
-__field(u32, offset)
-__field(u32, len)
+__field(u64, offset)
+__field(u64, len)
),
TP_fast_assign(
@@ -460,17 +460,17 @@ TRACE_EVENT(i915_gem_object_pread,
__entry->len = len;
),
- TP_printk("obj=%p, offset=%u, len=%u",
+ TP_printk("obj=%p, offset=%llx, len=%llx",
__entry->obj, __entry->offset, __entry->len)
);
TRACE_EVENT(i915_gem_object_fault,
- TP_PROTO(struct drm_i915_gem_object *obj, u32 index, bool gtt, bool
write),
+ TP_PROTO(struct drm_i915_gem_object *obj, u64 index, bool gtt, bool
write),
TP_ARGS(obj, index, gtt, write),
TP_STRUCT__entry(
__field(struct drm_i915_gem_object *, obj)
-__field(u32, index)
+__field(u64, index)
Re: [Intel-gfx] [PATCH v7 3/9] drm/i915: Drop AUX backlight enable check for backlight control
On Fri, 2017-05-12 at 11:10 -0700, Puthikorn Voravootivat wrote:
>
>
> On Fri, May 12, 2017 at 6:14 AM, Jani Nikula
> wrote:
> On Fri, 12 May 2017, "Pandiyan, Dhinakaran"
> wrote:
> > On Thu, 2017-05-11 at 16:02 -0700, Puthikorn Voravootivat
> wrote:
> >> There are some panel that
> >> (1) does not support display backlight enable via AUX
> >> (2) support display backlight adjustment via AUX
> >> (3) support display backlight enable via eDP BL_ENABLE pin
> >>
> >> The current driver required that (1) must be support to
> enable (2).
> >> This patch drops that requirement.
> >>
> >
> > You sent this version before I finished my follow-up
> questions, copying
> > the conversation here for context.
>
> Puthikorn, please don't send new versions before the review is
> addressed.
> Sorry I thought I was explained it clear enough.
>
> Pushed patches 1, 2, 5, and 7. Thanks for the patches and
> review.
>
> BR,
> Jani.
>
> > DK: Won't DP_EDP_BACKLIGHT_AUX_ENABLE_CAP be 1 always? The
> code below,
> > in
> > intel_dp_aux_display_control_capable(), makes sure
> > DP_EDP_BACKLIGHT_PIN_ENABLE_CAP=0. The spec says at least
> one of these
> > has to be 1.
> >
> > Puthikorn: We will drop the
> DP_EDP_BACKLIGHT_PIN_ENABLE_CAP != 0 check
> > in next patch set.
> > This patch adds check here to prepare for that.
> >
> >
> > 1) So, this patch does not really fix what the commit
> message claims
> > because it is dependent on the following patch. Does it make
> sense to
> > remove this check in this patch? That way, this patch by
> itself is the
> > fix that the commit message says.
> >
> > - !((intel_dp->edp_dpcd[1] &
> DP_EDP_BACKLIGHT_PIN_ENABLE_CAP)
> >
>
> Sure. I can remove this here and adds it in next patch instead.
>
>
> >
> > 2) If a panel supports backlight enable via AUX and
> BL_ENABLE pin, this
> > patch (along with the next) enables backlight twice, doesn't
> it?
> > _intel_edp_backlight_on(intel_dp) in intel_dp.c is called
> > unconditionally after intel_dp_aux_enable_backlight(). I
> don't know how
> > likely this configuration is or if it's alright to enable
> via both AUX
> > and BL_ENABLE pin.
> >
>
> The eDP spec did not mention this case explicitly.
> But it should not hurt to enable backlight twice as we want the
> backlight to be enabled anyway.
>
Hope so, at least a TODO would be a reasonable warning.
> >
> >
> >
> >> Signed-off-by: Puthikorn Voravootivat
> >> ---
> >> drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 5 -
> >> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >> index 870c03fc0f3a..c22712762957 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >> @@ -28,6 +28,10 @@ static void
> set_aux_backlight_enable(struct intel_dp *intel_dp, bool
> enable)
> >> {
> >> uint8_t reg_val = 0;
> >>
> >> + /* Early return when display use other mechanism to
> enable backlight. */
> >> +if (!(intel_dp->edp_dpcd[1] &
> DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))
> >> +return;
> >> +
> >> if (drm_dp_dpcd_readb(&intel_dp->aux,
> DP_EDP_DISPLAY_CONTROL_REGISTER,
> >>®_val) < 0) {
> >> DRM_DEBUG_KMS("Failed to read DPCD register 0x
> %x\n",
> >> @@ -164,7 +168,6 @@
> intel_dp_aux_display_control_capable(struct intel_connector
> *connector)
> >> * the panel can support backlight control over the
> aux channel
> >> */
> >> if (intel_dp->edp_dpcd[1] &
> DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP &&
> >> -(intel_dp->edp_dpcd[1] &
> DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> >> (intel_dp->edp_dpcd[2] &
> DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) &&
> >> !((intel_dp->edp_dpcd[1] &
> DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) ||
> >>(intel_dp->edp_dpcd[2] &
> DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))) {
> >
>
>
> --
> Jani Nikula, Intel Open Source Technology Center
>
>
>
[Intel-gfx] ✓ Fi.CI.BAT: success for dma-buf/sync-file: Defer creation of sync_file->name (rev2)
== Series Details == Series: dma-buf/sync-file: Defer creation of sync_file->name (rev2) URL : https://patchwork.freedesktop.org/series/24353/ State : success == Summary == Series 24353v2 dma-buf/sync-file: Defer creation of sync_file->name https://patchwork.freedesktop.org/api/1.0/series/24353/revisions/2/mbox/ Test gem_exec_flush: Subgroup basic-batch-kernel-default-uc: pass -> FAIL (fi-snb-2600) fdo#17 fdo#17 https://bugs.freedesktop.org/show_bug.cgi?id=17 fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time:440s fi-bdw-gvtdvmtotal:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:436s fi-bsw-n3050 total:278 pass:242 dwarn:0 dfail:0 fail:0 skip:36 time:579s fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:514s fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time:492s fi-byt-n2820 total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:489s fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:418s fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:420s fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:420s fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:487s fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:468s fi-kbl-7500u total:278 pass:255 dwarn:5 dfail:0 fail:0 skip:18 time:462s fi-kbl-7560u total:278 pass:263 dwarn:5 dfail:0 fail:0 skip:10 time:569s fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:460s fi-skl-6700hqtotal:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time:585s fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time:469s fi-skl-6770hqtotal:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:504s fi-skl-gvtdvmtotal:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time:441s fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:539s fi-snb-2600 total:278 pass:248 dwarn:0 dfail:0 fail:1 skip:29 time:405s fi-bxt-t5700 failed to collect. IGT log at Patchwork_4687/fi-bxt-t5700/igt.log 38d26beef9f31fa7ca983563cf86d4131defd94a drm-tip: 2017y-05m-12d-13h-15m-01s UTC integration manifest f35f9bf dma-buf/sync-file: Defer creation of sync_file->name == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4687/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Exclude top-page for ppgtt as well as ggtt
On Fri, May 12, 2017 at 06:55:35PM +0100, Chris Wilson wrote: > On Fri, May 12, 2017 at 06:49:09PM +0100, Chris Wilson wrote: > > We have always excluded the top-page of the Global GTT to prevent > > prefetching past the end of the address space. We have been lax about > > applying this restriction to the per-process GTT, but there is no reason > > to believe that the hw restriction is any less severe. > > Other than empirical, but the note in bspec is still there and who knows > they may start to be strict again in future. Fwiw, it looks like we can sleep on this again. Jason no longer believes this to be an immediate issue. -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 2/3] drm/i915/guc: Introduce buffer based cmd transport
On Fri, May 12, 2017 at 08:53:15PM +0200, Michal Wajdeczko wrote:
> On Fri, May 12, 2017 at 04:52:04PM +0100, Chris Wilson wrote:
> > On Fri, May 12, 2017 at 03:02:59PM +, Michal Wajdeczko wrote:
> > > + if (!ctch->vma) {
> > > + /* allocate vma */
> > > + vma = intel_guc_allocate_vma(guc, sizeof(*blob));
> > > + if (IS_ERR(vma))
> > > + return PTR_ERR(vma);
> > > + ctch->vma = vma;
> > > +
> > > + /* get unique owner id */
> > > + err = ida_simple_get(&guc->ct.owner_ida,
> > > + 0, INTEL_GUC_CT_MAX_CHANNELS,
> > > + GFP_KERNEL);
> >
> > What's the point of an ida if MAX is 1!
>
> We may have MAX > 1 in the future.
MAX should reflect the firmware limits. Because at the moment it looks
like this is imposing a restriction far above and beyond the existing
limits. If it just required a unique identifier and you plan to have one
per client, why we do more than one unique identifier?
> > > + base = guc_ggtt_offset(ctch->vma);
> > > + DRM_DEBUG_DRIVER("CT: vma base=%#x\n", base);
> > > +
> > > + /* initialize descriptors */
> > > + for (i = 0; i < ARRAY_SIZE(*blob); i++) {
> > > + struct guc_ct_buffer_desc *desc = &(*blob)[i].desc;
> > > +
> > > + guc_ct_buffer_desc_init(desc,
> > > + base + arr_offset(*blob, i, cmds),
> > > + sizeof((*blob)[0]) -
> > > + ptr_offset(&(*blob)[0], cmds),
> >
> > So chosing a non-power-of-two ringbuf was entirely your own fault, as
> > we tell the guc the size. Similarly the position of the desc.
> >
> > I would have picked
> >
> > struct guc_ct_buffer_desc *desc[] __cacheline_aligned;
> >
> > desc = kmap(vma);
> > for (i = 0; i < 2; i+)
> > guc_ct_buffer_desc_init(&desc[i],
> > (char *)desc + 1024 * (i + 1),
> > 1024);
> > (You can then replace arr_offset with just desc[CT_RECV] and
> > desc[CT_SEND].)
> >
> > Oh, and move the allocation branch to a new function and always do the
> > reset.
>
> Moving allocation branch to a new function can be tricky unless we decide
> to share blob definition or implictly assume that cmds are just PAGE/4.
> Will try to rework this.
>
> And btw, plese note that desc_init() is already doing full reset.
Exactly with a bit of rework, reduce the code duplication. And you don't
need to make assumptions as you already record the offsets, or you can
use simplified layout above.
> >
> > > + ctch->owner);
> > > +
> > > + ctch->ctbs[i].desc = desc;
> > > + ctch->ctbs[i].cmds = (*blob)[i].cmds;
> > > + }
> > > +
> > > + } else {
> > > + /* vma is already allocated and kmap'ed */
> > > + base = guc_ggtt_offset(ctch->vma);
> > > +
> > > + /* reset descriptors */
> > > + for (i = 0; i < ARRAY_SIZE(ctch->ctbs); i++) {
> > > + guc_ct_buffer_desc_reset(ctch->ctbs[i].desc);
> > > + }
> > > + }
> > > +
> > > +
> > > + /* register buffers, recv first */
> > > + err = guc_action_register_ct_buffer(guc,
> > > + base +
> > > + arr_offset(*blob, 1, desc),
> > > + INTEL_GUC_CT_BUFFER_TYPE_RECV);
> > > + if (unlikely(err))
> > > + goto err_kunmap;
> > > +
> > > + err = guc_action_register_ct_buffer(guc,
> > > + base +
> > > + arr_offset(*blob, 0, desc),
> > > + INTEL_GUC_CT_BUFFER_TYPE_SEND);
> > > + if (unlikely(err))
> > > + goto err_deregister;
> > > +
> > > + return 0;
> > > +
> > > +err_deregister:
> > > + guc_action_deregister_ct_buffer(guc,
> > > + ctch->owner,
> > > + INTEL_GUC_CT_BUFFER_TYPE_RECV);
> > > +err_kunmap:
> > > + kunmap(i915_vma_first_page(ctch->vma));
> > > + ida_simple_remove(&guc->ct.owner_ida, ctch->owner);
> > > +err_vma:
> > > + i915_vma_unpin_and_release(&ctch->vma);
> > > + return err;
> > > +}
> > > +
> > > +static void ctch_close(struct intel_guc *guc,
> > > +struct intel_guc_ct_channel *ctch)
> > > +{
> > > + GEM_BUG_ON(!ctch_is_open(ctch));
> > > +
> > > + guc_action_deregister_ct_buffer(guc,
> > > + ctch->owner,
> > > + INTEL_GUC_CT_BUFFER_TYPE_SEND);
> > > + guc_action_deregister_ct_buffer(guc,
> > > + ctch->owner,
> > > + INTEL_GUC_CT_BUFFER_TYPE_RECV);
> > > +
> > > + ida_simple_remove(&guc->ct.owner_ida, ctch->owner);
> > > +
> > > + /* Unmap the page we mapped in the _open() */
> > > + kunmap(i915_vma_first_page(ctch->vma));
Re: [Intel-gfx] [PATCH] drm/i915: Exclude top-page for ppgtt as well as ggtt
On 12 May 2017 at 18:49, Chris Wilson wrote: > We have always excluded the top-page of the Global GTT to prevent > prefetching past the end of the address space. We have been lax about > applying this restriction to the per-process GTT, but there is no reason > to believe that the hw restriction is any less severe. > > Signed-off-by: Chris Wilson Reviewed-by: Matthew Auld ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3] dma-buf/sync-file: Defer creation of sync_file->name
Constructing the name takes the majority of the time for allocating a
sync_file to wrap a fence, and the name is very rarely used (only via
the sync_file status user interface). To reduce the impact on the common
path (that of creating sync_file to pass around), defer the construction
of the name until it is first used.
v2: Update kerneldoc (kbuild test robot)
v3: sync_debug.c was peeking at the name
Signed-off-by: Chris Wilson
Cc: Sumit Semwal
Cc: Gustavo Padovan
---
drivers/dma-buf/sync_debug.c | 3 ++-
drivers/dma-buf/sync_file.c | 34 +++---
include/linux/sync_file.h| 5 +++--
3 files changed, 32 insertions(+), 10 deletions(-)
diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c
index 4b1731ee7458..9a93f1085c63 100644
--- a/drivers/dma-buf/sync_debug.c
+++ b/drivers/dma-buf/sync_debug.c
@@ -134,7 +134,8 @@ static void sync_print_sync_file(struct seq_file *s,
{
int i;
- seq_printf(s, "[%p] %s: %s\n", sync_file, sync_file->name,
+ seq_printf(s, "[%p] %s: %s\n", sync_file,
+ sync_file_get_name(sync_file),
sync_status_str(dma_fence_get_status(sync_file->fence)));
if (dma_fence_is_array(sync_file->fence)) {
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index c9eb4997cfcc..d105079ec45c 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -80,11 +80,6 @@ struct sync_file *sync_file_create(struct dma_fence *fence)
sync_file->fence = dma_fence_get(fence);
- snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
-fence->ops->get_driver_name(fence),
-fence->ops->get_timeline_name(fence), fence->context,
-fence->seqno);
-
return sync_file;
}
EXPORT_SYMBOL(sync_file_create);
@@ -129,6 +124,31 @@ struct dma_fence *sync_file_get_fence(int fd)
}
EXPORT_SYMBOL(sync_file_get_fence);
+/**
+ * sync_file_get_name - get the name of the sync_file
+ * @sync_file: sync_file to get the fence from
+ *
+ * Each sync_file may have a name assigned either by the user (when merging
+ * sync_files together) or created from the fence it contains. However,
+ * construction of the name is deferred until first use.
+ *
+ * Returns: a string representing the name
+ */
+char *sync_file_get_name(struct sync_file *sync_file)
+{
+ if (!sync_file->user_name[0]) {
+ scnprintf(sync_file->user_name,
+ sizeof(sync_file->user_name),
+ "%s-%s%llu-%d",
+
sync_file->fence->ops->get_driver_name(sync_file->fence),
+
sync_file->fence->ops->get_timeline_name(sync_file->fence),
+ sync_file->fence->context,
+ sync_file->fence->seqno);
+ }
+
+ return sync_file->user_name;
+}
+
static int sync_file_set_fence(struct sync_file *sync_file,
struct dma_fence **fences, int num_fences)
{
@@ -266,7 +286,7 @@ static struct sync_file *sync_file_merge(const char *name,
struct sync_file *a,
goto err;
}
- strlcpy(sync_file->name, name, sizeof(sync_file->name));
+ strlcpy(sync_file->user_name, name, sizeof(sync_file->user_name));
return sync_file;
err:
@@ -419,7 +439,7 @@ static long sync_file_ioctl_fence_info(struct sync_file
*sync_file,
}
no_fences:
- strlcpy(info.name, sync_file->name, sizeof(info.name));
+ strlcpy(info.name, sync_file_get_name(sync_file), sizeof(info.name));
info.status = dma_fence_is_signaled(sync_file->fence);
info.num_fences = num_fences;
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index d37beefdfbd5..0f3f05325c84 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -23,7 +23,7 @@
/**
* struct sync_file - sync file to export to the userspace
* @file: file representing this fence
- * @name: name of sync_file. Useful for debugging
+ * @user_name: name of sync_file. Useful for debugging
* @sync_file_list:membership in global file list
* @wq:wait queue for fence signaling
* @fence: fence with the fences in the sync_file
@@ -31,7 +31,7 @@
*/
struct sync_file {
struct file *file;
- charname[32];
+ charuser_name[32];
#ifdef CONFIG_DEBUG_FS
struct list_headsync_file_list;
#endif
@@ -46,5 +46,6 @@ struct sync_file {
struct sync_file *sync_file_create(struct dma_fence *fence);
struct dma_fence *sync_file_get_fence(int fd);
+char *sync_file_get_name(struct sync_file *sync_file);
#endif /* _LINUX_SYNC_H */
--
2.11.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listin
Re: [Intel-gfx] [PATCH 2/3] drm/i915/guc: Introduce buffer based cmd transport
On Fri, May 12, 2017 at 04:52:04PM +0100, Chris Wilson wrote:
> On Fri, May 12, 2017 at 03:02:59PM +, Michal Wajdeczko wrote:
> > Buffer based command transport can replace MMIO based mechanism.
> > It may be used to perform host-2-guc and guc-to-host communication.
>
> Hmm, sad to see a ringbuffer used for synchronous comms.
>
> > Portions of this patch are based on work by:
> > Michel Thierry
> > Robert Beckett
> > Daniele Ceraolo Spurio
> >
> > Signed-off-by: Michal Wajdeczko
> > Cc: Daniele Ceraolo Spurio
> > Cc: Oscar Mateo
> > Cc: Joonas Lahtinen
> > ---
> > drivers/gpu/drm/i915/Makefile | 1 +
> > drivers/gpu/drm/i915/i915_drv.c | 2 +
> > drivers/gpu/drm/i915/i915_drv.h | 2 +
> > drivers/gpu/drm/i915/i915_utils.h | 4 +
> > drivers/gpu/drm/i915/intel_guc_ct.c | 440
> > ++
> > drivers/gpu/drm/i915/intel_guc_ct.h | 92 +++
> > drivers/gpu/drm/i915/intel_guc_fwif.h | 42
> > drivers/gpu/drm/i915/intel_uc.c | 25 +-
> > drivers/gpu/drm/i915/intel_uc.h | 4 +-
> > 9 files changed, 610 insertions(+), 2 deletions(-)
> > create mode 100644 drivers/gpu/drm/i915/intel_guc_ct.c
> > create mode 100644 drivers/gpu/drm/i915/intel_guc_ct.h
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index 7b05fb8..16dccf5 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -58,6 +58,7 @@ i915-y += i915_cmd_parser.o \
> >
> > # general-purpose microcontroller (GuC) support
> > i915-y += intel_uc.o \
> > + intel_guc_ct.o \
> > intel_guc_log.o \
> > intel_guc_loader.o \
> > intel_huc.o \
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index 72fb47a..71f7915 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -869,6 +869,7 @@ static int i915_driver_init_early(struct
> > drm_i915_private *dev_priv,
> > i915_workqueues_cleanup(dev_priv);
> > err_engines:
> > i915_engines_cleanup(dev_priv);
> > + intel_uc_cleanup(dev_priv);
> > return ret;
> > }
> >
> > @@ -883,6 +884,7 @@ static void i915_driver_cleanup_early(struct
> > drm_i915_private *dev_priv)
> > intel_irq_fini(dev_priv);
> > i915_workqueues_cleanup(dev_priv);
> > i915_engines_cleanup(dev_priv);
> > + intel_uc_cleanup(dev_priv);
> > }
> >
> > static int i915_mmio_setup(struct drm_i915_private *dev_priv)
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index ff3574a..ec2d45f 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -712,6 +712,7 @@ struct intel_csr {
> > func(has_gmbus_irq); \
> > func(has_gmch_display); \
> > func(has_guc); \
> > + func(has_guc_ct); \
> > func(has_hotplug); \
> > func(has_l3_dpf); \
> > func(has_llc); \
> > @@ -2831,6 +2832,7 @@ intel_info(const struct drm_i915_private *dev_priv)
> > * properties, so we have separate macros to test them.
> > */
> > #define HAS_GUC(dev_priv) ((dev_priv)->info.has_guc)
> > +#define HAS_GUC_CT(dev_priv) ((dev_priv)->info.has_guc_ct)
> > #define HAS_GUC_UCODE(dev_priv)(HAS_GUC(dev_priv))
> > #define HAS_GUC_SCHED(dev_priv)(HAS_GUC(dev_priv))
> > #define HAS_HUC_UCODE(dev_priv)(HAS_GUC(dev_priv))
> > diff --git a/drivers/gpu/drm/i915/i915_utils.h
> > b/drivers/gpu/drm/i915/i915_utils.h
> > index f9d6607..cd4a0d9 100644
> > --- a/drivers/gpu/drm/i915/i915_utils.h
> > +++ b/drivers/gpu/drm/i915/i915_utils.h
> > @@ -86,6 +86,10 @@
> >
> > #define ptr_offset(ptr, member) offsetof(typeof(*(ptr)), member)
> >
> > +#define arr_offset(arr, index, member) ({ \
> > + (index) * sizeof((arr)[0]) + ptr_offset(&(arr)[0], member); \
> > +})
> > +
> > #define fetch_and_zero(ptr) ({
> > \
> > typeof(*ptr) __T = *(ptr); \
> > *(ptr) = (typeof(*ptr))0; \
> > diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c
> > b/drivers/gpu/drm/i915/intel_guc_ct.c
> > new file mode 100644
> > index 000..6eb46e0
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/intel_guc_ct.c
> > @@ -0,0 +1,440 @@
> > +/*
> > + * Copyright © 2016-2017 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission n
[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Exclude top-page for ppgtt as well as ggtt
== Series Details == Series: drm/i915: Exclude top-page for ppgtt as well as ggtt URL : https://patchwork.freedesktop.org/series/24366/ State : success == Summary == Series 24366v1 drm/i915: Exclude top-page for ppgtt as well as ggtt https://patchwork.freedesktop.org/api/1.0/series/24366/revisions/1/mbox/ fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time:448s fi-bdw-gvtdvmtotal:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:431s fi-bsw-n3050 total:278 pass:242 dwarn:0 dfail:0 fail:0 skip:36 time:575s fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:514s fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time:497s fi-byt-n2820 total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:485s fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:415s fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:413s fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:418s fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:496s fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:461s fi-kbl-7500u total:278 pass:255 dwarn:5 dfail:0 fail:0 skip:18 time:460s fi-kbl-7560u total:278 pass:263 dwarn:5 dfail:0 fail:0 skip:10 time:580s fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:459s fi-skl-6700hqtotal:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time:577s fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time:469s fi-skl-6770hqtotal:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:503s fi-skl-gvtdvmtotal:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time:438s fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:537s fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time:411s fi-bxt-t5700 failed to collect. IGT log at Patchwork_4686/fi-bxt-t5700/igt.log 38d26beef9f31fa7ca983563cf86d4131defd94a drm-tip: 2017y-05m-12d-13h-15m-01s UTC integration manifest a8f0d10 drm/i915: Exclude top-page for ppgtt as well as ggtt == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4686/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v7 3/9] drm/i915: Drop AUX backlight enable check for backlight control
On Fri, May 12, 2017 at 6:14 AM, Jani Nikula
wrote:
> On Fri, 12 May 2017, "Pandiyan, Dhinakaran"
> wrote:
> > On Thu, 2017-05-11 at 16:02 -0700, Puthikorn Voravootivat wrote:
> >> There are some panel that
> >> (1) does not support display backlight enable via AUX
> >> (2) support display backlight adjustment via AUX
> >> (3) support display backlight enable via eDP BL_ENABLE pin
> >>
> >> The current driver required that (1) must be support to enable (2).
> >> This patch drops that requirement.
> >>
> >
> > You sent this version before I finished my follow-up questions, copying
> > the conversation here for context.
>
> Puthikorn, please don't send new versions before the review is
> addressed.
>
Sorry I thought I was explained it clear enough.
>
> Pushed patches 1, 2, 5, and 7. Thanks for the patches and review.
>
> BR,
> Jani.
>
> > DK: Won't DP_EDP_BACKLIGHT_AUX_ENABLE_CAP be 1 always? The code below,
> > in
> > intel_dp_aux_display_control_capable(), makes sure
> > DP_EDP_BACKLIGHT_PIN_ENABLE_CAP=0. The spec says at least one of these
> > has to be 1.
> >
> > Puthikorn: We will drop the DP_EDP_BACKLIGHT_PIN_ENABLE_CAP != 0 check
> > in next patch set.
> > This patch adds check here to prepare for that.
> >
> >
> > 1) So, this patch does not really fix what the commit message claims
> > because it is dependent on the following patch. Does it make sense to
> > remove this check in this patch? That way, this patch by itself is the
> > fix that the commit message says.
> >
> > - !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP)
> >
>
Sure. I can remove this here and adds it in next patch instead.
>
> > 2) If a panel supports backlight enable via AUX and BL_ENABLE pin, this
> > patch (along with the next) enables backlight twice, doesn't it?
> > _intel_edp_backlight_on(intel_dp) in intel_dp.c is called
> > unconditionally after intel_dp_aux_enable_backlight(). I don't know how
> > likely this configuration is or if it's alright to enable via both AUX
> > and BL_ENABLE pin.
> >
>
The eDP spec did not mention this case explicitly.
But it should not hurt to enable backlight twice as we want the backlight
to be enabled anyway.
> >
> >
> >
> >> Signed-off-by: Puthikorn Voravootivat
> >> ---
> >> drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 5 -
> >> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >> index 870c03fc0f3a..c22712762957 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >> @@ -28,6 +28,10 @@ static void set_aux_backlight_enable(struct
> intel_dp *intel_dp, bool enable)
> >> {
> >> uint8_t reg_val = 0;
> >>
> >> + /* Early return when display use other mechanism to enable
> backlight. */
> >> +if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))
> >> +return;
> >> +
> >> if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_
> REGISTER,
> >>®_val) < 0) {
> >> DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
> >> @@ -164,7 +168,6 @@ intel_dp_aux_display_control_capable(struct
> intel_connector *connector)
> >> * the panel can support backlight control over the aux channel
> >> */
> >> if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP
> &&
> >> -(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> >> (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)
> &&
> >> !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) ||
> >>(intel_dp->edp_dpcd[2] &
> >> DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP)))
> {
> >
>
> --
> Jani Nikula, Intel Open Source Technology Center
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Exclude top-page for ppgtt as well as ggtt
On Fri, May 12, 2017 at 06:49:09PM +0100, Chris Wilson wrote: > We have always excluded the top-page of the Global GTT to prevent > prefetching past the end of the address space. We have been lax about > applying this restriction to the per-process GTT, but there is no reason > to believe that the hw restriction is any less severe. Other than empirical, but the note in bspec is still there and who knows they may start to be strict again in future. -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: Exclude top-page for ppgtt as well as ggtt
We have always excluded the top-page of the Global GTT to prevent
prefetching past the end of the address space. We have been lax about
applying this restriction to the per-process GTT, but there is no reason
to believe that the hw restriction is any less severe.
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 5332d8cb1c53..f18ca11a8da6 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1860,7 +1860,8 @@ static void i915_address_space_init(struct
i915_address_space *vm,
{
i915_gem_timeline_init(dev_priv, &vm->timeline, name);
- drm_mm_init(&vm->mm, 0, vm->total);
+ /* Always exclude the top page to avoid prefetches past the end. */
+ drm_mm_init(&vm->mm, 0, vm->total - I915_GTT_PAGE_SIZE);
vm->mm.head_node.color = I915_COLOR_UNEVICTABLE;
INIT_LIST_HEAD(&vm->active_list);
@@ -2390,7 +2391,7 @@ static void i915_gtt_color_adjust(const struct
drm_mm_node *node,
* GTT boundary.
*/
node = list_next_entry(node, node_list);
- if (node->color != color)
+ if (node->allocated && node->color != color)
*end -= I915_GTT_PAGE_SIZE;
}
--
2.11.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 0/3] Engine utilization tracking
On 5/10/2017 12:45 PM, Daniel Vetter wrote: On Wed, May 10, 2017 at 10:38 AM, Tvrtko Ursulin wrote: On 09/05/2017 19:11, Dmitry Rogozhkin wrote: On 5/9/2017 8:51 AM, Tvrtko Ursulin wrote: On 09/05/2017 16:29, Chris Wilson wrote: On Tue, May 09, 2017 at 04:16:41PM +0100, Tvrtko Ursulin wrote: On 09/05/2017 15:26, Chris Wilson wrote: On Tue, May 09, 2017 at 03:09:33PM +0100, Tvrtko Ursulin wrote: From: Tvrtko Ursulin By popular customer demand here is the prototype for cheap engine utilization tracking. customer and debugfs? Well I did write in one of the following paragraphs on this topic. Perhaps I should have put it in procfs. :) Sysfs API looks restrictive or perhaps I missed a way to get low level (fops) access to it. It uses static branches so in the default off case it really should be cheap. Not as cheap (for the off case) as simply sampling RING_HEAD/RING_TAIL Off case are three no-op instructions in three places in the irq tasklet. And a little bit of object size growth, if you worry about that aspect? It's just how the snowball begins. We should be able to control it. We also have to consider which one is lighter for this particular use case. which looks to be the same level of detail. I wrapped all this up in a perf interface once up a time... How does that work? Via periodic sampling? Accuracy sounds like it would be proportionate to the sampling frequency, no? Right, and the sampling frequency is under user control (via perf) with a default of around 1000, gives a small systematic error when dealing with % I included power, interrupts, rc6, frequency (and the statistics but I never used those and dropped them once oa landed), as well as utilisation, just for the convenience of having sane interface :) Can you resurrect those patches? Don't have to rebase and all but I would like to see them at least. Mind that the idea behind the requested kind of stats is primary usage by the customers in the _product_ environment to track GPU occupancy and predict based on this stats whether they can execute something else. Which means that 1) debugfs and any kind of debug-like infrastructure is Yeah I acknowledged in the cover letter debugfs is not ideal. I could implement it in sysfs I suppose by doing time based transitions as opposed to having explicit open/release hooks. It wouldn't make a fundamental different to this RFC from the overhead point of view. But most importantly we need to see in detail how does Chris' perf based idea looks like and does it fit your requirements. +1 on perf pmu, that sounds much more like the userspace interface you're looking for. If it's not that, then perhaps hand-rolled like the i915 OA stuff we now have (but starting out with a perf pmu sounds much better, at least for anything global which doesn't need to be per-context or per-batch). -Daniel You know, thinking once more time which interface I would like to see as a user, I would say the following. As a user I expect to have easy access to the basic GPU information and current characteristics. This information includes: 1. GPU frequency characteristics including: current running frequency, min/max SW limits, min/max HW limits, boost frequency settings (if any), driver power/performance preset (if any) 2. Basic information of GPU high level structure, I specifically mean engines capable to work in parallel: number of VDBOX engines, number of VEBOX engines, etc. 3. High level metric to understand how GPU was busy over time: each engine busy clocks I would assume that there will be users who will simply log to the system and want to quickly get the above info with the cat /sysfs file. I would assume that some programmatic usages are possible to parse sysfs and take certain actions if, for example, current GPU support single VDBOX only (for example, run some operation as SW decoding, rather than HW). So, I would suggest to have /sysfs files for the information above. Perf subsystem indeed looks attractive to expose such a metrics, but I think we need to target lower level metrics with Perf. From my perspective right now i915 misses exposure of certain key information which is natively expected by any user and developer. Using perf to expose it will force users to use special tools or write own programs to query them - this will simply reduce usability. After all, why you expose /sys/class/drm/card0/power/rc6_residency_ms and you do not expose how much time GPGPU or VDBOX did its job?! Honestly, RC6 is a second level of details for the significant part of the users and customers. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
On Fri, 12 May 2017 06:56:03 + "Chen, Xiaoguang" wrote: > Hi Gerd, > > >-Original Message- > >From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On > >Behalf Of Gerd Hoffmann > >Sent: Thursday, May 11, 2017 9:28 PM > >To: Chen, Xiaoguang > >Cc: Tian, Kevin ; intel-gfx@lists.freedesktop.org; > >linux- > >ker...@vger.kernel.org; zhen...@linux.intel.com; Alex Williamson > >; Lv, Zhiyuan ; intel-gvt- > >d...@lists.freedesktop.org; Wang, Zhi A > >Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf > > > > Hi, > > > >> While read the framebuffer region we have to tell the vendor driver which > >framebuffer we want to read? There are two framebuffers now in KVMGT that is > >primary and cursor. > >> There are two methods to implement this: > >> 1) write the plane id first and then read the framebuffer. > >> 2) create 2 vfio regions one for primary and one for cursor. > > > >(3) Place information for both planes into one vfio region. > >Which allows to fetch both with a single read() syscall. > That works too. Then using the ioctl to get the dmabuf fd if needed. And > plane id can be ioctl's parameter. > > How about method 2 primary plane and cursor plane are different and should > generate different dmabuf for each of them. > > > > >The question is how you'll get the file descriptor then. If the ioctl > >returns the > >dma-buf fd only you have a racy interface: Things can change between > >read(vfio- > >region) and ioctl(need-dmabuf-fd). > You are right. So when creating the dmabuf we may have to decode the > framebuffer and create the dmabuf using the latest framebuffer information > and we must return the framebuffer information together with the dmabuf fd. > > In the current implementation I saved the framebuffer information while user > querying the framebuffer and generate the dmabuf using the saved information > no error found yet but in theory there are sync problems. > > > > >ioctl(need-dma-buf) could return both dmabuf fd and plane info to fix the > >race, > >but then it is easier to go with ioctl only interface (simliar to the > >orginal one from > >dec last year) I think. > Yes. ioctl works for it. > But based on the mail last week. If I understand correctly Alex hope to query > the framebuffer information by reading the vfio device region and then get > the dmabuf fd using ioctl. No, I was explaining that I had questioned whether we could use a vfio region in place of a separate dmabuf fd. We can't. I have no particular desire to use a vfio region just for querying framebuffer info. I prefer the dmabuf manager fd idea that Gerd suggested. Thanks, Alex ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
On Fri, 12 May 2017 11:12:05 +0200 Gerd Hoffmann wrote: > Hi, > > > If the contents of the framebuffer change or if the parameters of the > > framebuffer change? I can't image that creating a new dmabuf fd for > > every visual change within the framebuffer would be efficient, but I > > don't have any concept of what a dmabuf actually does. > > Ok, some background: > > The drm subsystem has the concept of planes. The most important plane > is the primary framebuffer (i.e. what gets scanned out to the physical > display). The cursor is a plane too, and there can be additional > overlay planes for stuff like video playback. > > Typically there are multiple planes in a system and only one of them > gets scanned out to the crtc, i.e. the fbdev emulation creates one plane > for the framebuffer console. The X-Server creates a plane too, and when > you switch between X-Server and framebuffer console via ctrl-alt-fn the > intel driver just reprograms the encoder to scan out the one or the > other plane to the crtc. > > The dma-buf handed out by gvt is a reference to a plane. I think on the > host side gvt can see only the active plane (from encoder/crtc register > programming) not the inactive ones. > > The dma-buf can be imported as opengl texture and then be used to render > the guest display to a host window. I think it is even possible to use > the dma-buf as plane in the host drm driver and scan it out directly to > a physical display. The actual framebuffer content stays in gpu memory > all the time, the cpu never has to touch it. > > It is possible to cache the dma-buf handles, i.e. when the guest boots > you'll get the first for the fbcon plane, when the x-server starts the > second for the x-server framebuffer, and when the user switches to the > text console via ctrl-alt-fn you can re-use the fbcon dma-buf you > already have. > > The caching becomes more important for good performance when the guest > uses pageflipping (wayland does): define two planes, render into one > while displaying the other, then flip the two for a atomic display > update. > > The caching also makes it a bit difficult to create a good interface. > So, the current patch set creates: > > (a) A way to query the active planes (ioctl > INTEL_VGPU_QUERY_DMABUF added by patch 5/6 of this series). > (b) A way to create a dma-buf for the active plane (ioctl > INTEL_VGPU_GENERATE_DMABUF). > > Typical userspace workflow is to first query the plane, then check if it > already has a dma-buf for it, and if not create one. Thank you! This is immensely helpful! > > What changes to the framebuffer require a new dmabuf fd? Shouldn't the > > user query the parameters of the framebuffer through a dmabuf fd and > > shouldn't the dmabuf fd have some signaling mechanism to the user > > (eventfd perhaps) to notify the user to re-evaluate the parameters? > > dma-bufs don't support that, they are really just a handle to a piece of > memory, all metadata (format, size) most be communicated by other means. > > > Otherwise are you imagining that the user polls the vfio region? > > Hmm, notification support would probably a good reason to have a > separate file handle to manage the dma-bufs (instead of using > driver-specific ioctls on the vfio fd), because the driver could also > use the management fd for notifications then. I like this idea of a separate control fd for dmabufs, it provides not only a central management point, but also a nice abstraction for the vfio device specific interface. We potentially only need a single VFIO_DEVICE_GET_DMABUF_MGR_FD() ioctl to get a dmabuf management fd (perhaps with a type parameter, ex. GFX) where maybe we could have vfio-core incorporate this reference into the group lifecycle, so the vendor driver only needs to fdget/put this manager fd for the various plane dmabuf fds spawned in order to get core-level reference counting. The dmabuf manager fd can be separately versioned from vfio or make use of some vendor magic if there are portions that are vendor specific (the above examples for query/get ioctls really don't seem vendor specific). > I'm not sure how useful notification support actually is though. > Notifications when another plane gets mapped to the crtc should be easy. > But I'm not sure it is possible to get notifications when the plane > content changes, especially in case the guest does software rendering so > the display is updated without gvt seeing guest activity on the > rendering pipeline. Without the later qemu needs a timer for display > updates _anyway_ ... Seems like it has benefits even if we don't have an initial use for notification mechanisms. Thanks, Alex ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] drm/i915/guc: Introduce buffer based cmd transport
On Fri, May 12, 2017 at 03:02:59PM +, Michal Wajdeczko wrote:
> Buffer based command transport can replace MMIO based mechanism.
> It may be used to perform host-2-guc and guc-to-host communication.
Hmm, sad to see a ringbuffer used for synchronous comms.
> Portions of this patch are based on work by:
> Michel Thierry
> Robert Beckett
> Daniele Ceraolo Spurio
>
> Signed-off-by: Michal Wajdeczko
> Cc: Daniele Ceraolo Spurio
> Cc: Oscar Mateo
> Cc: Joonas Lahtinen
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/i915_drv.c | 2 +
> drivers/gpu/drm/i915/i915_drv.h | 2 +
> drivers/gpu/drm/i915/i915_utils.h | 4 +
> drivers/gpu/drm/i915/intel_guc_ct.c | 440
> ++
> drivers/gpu/drm/i915/intel_guc_ct.h | 92 +++
> drivers/gpu/drm/i915/intel_guc_fwif.h | 42
> drivers/gpu/drm/i915/intel_uc.c | 25 +-
> drivers/gpu/drm/i915/intel_uc.h | 4 +-
> 9 files changed, 610 insertions(+), 2 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/intel_guc_ct.c
> create mode 100644 drivers/gpu/drm/i915/intel_guc_ct.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 7b05fb8..16dccf5 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -58,6 +58,7 @@ i915-y += i915_cmd_parser.o \
>
> # general-purpose microcontroller (GuC) support
> i915-y += intel_uc.o \
> + intel_guc_ct.o \
> intel_guc_log.o \
> intel_guc_loader.o \
> intel_huc.o \
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 72fb47a..71f7915 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -869,6 +869,7 @@ static int i915_driver_init_early(struct drm_i915_private
> *dev_priv,
> i915_workqueues_cleanup(dev_priv);
> err_engines:
> i915_engines_cleanup(dev_priv);
> + intel_uc_cleanup(dev_priv);
> return ret;
> }
>
> @@ -883,6 +884,7 @@ static void i915_driver_cleanup_early(struct
> drm_i915_private *dev_priv)
> intel_irq_fini(dev_priv);
> i915_workqueues_cleanup(dev_priv);
> i915_engines_cleanup(dev_priv);
> + intel_uc_cleanup(dev_priv);
> }
>
> static int i915_mmio_setup(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ff3574a..ec2d45f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -712,6 +712,7 @@ struct intel_csr {
> func(has_gmbus_irq); \
> func(has_gmch_display); \
> func(has_guc); \
> + func(has_guc_ct); \
> func(has_hotplug); \
> func(has_l3_dpf); \
> func(has_llc); \
> @@ -2831,6 +2832,7 @@ intel_info(const struct drm_i915_private *dev_priv)
> * properties, so we have separate macros to test them.
> */
> #define HAS_GUC(dev_priv)((dev_priv)->info.has_guc)
> +#define HAS_GUC_CT(dev_priv) ((dev_priv)->info.has_guc_ct)
> #define HAS_GUC_UCODE(dev_priv) (HAS_GUC(dev_priv))
> #define HAS_GUC_SCHED(dev_priv) (HAS_GUC(dev_priv))
> #define HAS_HUC_UCODE(dev_priv) (HAS_GUC(dev_priv))
> diff --git a/drivers/gpu/drm/i915/i915_utils.h
> b/drivers/gpu/drm/i915/i915_utils.h
> index f9d6607..cd4a0d9 100644
> --- a/drivers/gpu/drm/i915/i915_utils.h
> +++ b/drivers/gpu/drm/i915/i915_utils.h
> @@ -86,6 +86,10 @@
>
> #define ptr_offset(ptr, member) offsetof(typeof(*(ptr)), member)
>
> +#define arr_offset(arr, index, member) ({\
> + (index) * sizeof((arr)[0]) + ptr_offset(&(arr)[0], member); \
> +})
> +
> #define fetch_and_zero(ptr) ({
> \
> typeof(*ptr) __T = *(ptr); \
> *(ptr) = (typeof(*ptr))0; \
> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c
> b/drivers/gpu/drm/i915/intel_guc_ct.c
> new file mode 100644
> index 000..6eb46e0
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_guc_ct.c
> @@ -0,0 +1,440 @@
> +/*
> + * Copyright © 2016-2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
[Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/guc: Disable send function on fini
== Series Details == Series: series starting with [1/3] drm/i915/guc: Disable send function on fini URL : https://patchwork.freedesktop.org/series/24363/ State : success == Summary == Series 24363v1 Series without cover letter https://patchwork.freedesktop.org/api/1.0/series/24363/revisions/1/mbox/ fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time:453s fi-bdw-gvtdvmtotal:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:432s fi-bsw-n3050 total:278 pass:242 dwarn:0 dfail:0 fail:0 skip:36 time:588s fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:498s fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time:488s fi-byt-n2820 total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:482s fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:414s fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:416s fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:425s fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:494s fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:468s fi-kbl-7500u total:278 pass:255 dwarn:5 dfail:0 fail:0 skip:18 time:449s fi-kbl-7560u total:278 pass:263 dwarn:5 dfail:0 fail:0 skip:10 time:560s fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:446s fi-skl-6700hqtotal:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time:572s fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time:455s fi-skl-6770hqtotal:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:489s fi-skl-gvtdvmtotal:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time:423s fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:541s fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time:415s fi-bxt-t5700 failed to collect. IGT log at Patchwork_4685/fi-bxt-t5700/igt.log 38d26beef9f31fa7ca983563cf86d4131defd94a drm-tip: 2017y-05m-12d-13h-15m-01s UTC integration manifest 169b7b8 HAX Enable GuC loading & submission 2d34ce6 drm/i915/guc: Introduce buffer based cmd transport 4ab208a drm/i915/guc: Disable send function on fini == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4685/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915/guc: Disable send function on fini
On Fri, May 12, 2017 at 04:17:00PM +0100, Chris Wilson wrote:
> On Fri, May 12, 2017 at 03:02:58PM +, Michal Wajdeczko wrote:
> > In earlier patch 789a625 we were enabling send function only
> > after successful init. For completeness, we should make sure
> > that we disable it on fini.
> >
> > Signed-off-by: Michal Wajdeczko
> > Cc: Joonas Lahtinen
> > Cc: Daniele Ceraolo Spurio
> > Cc: Chris Wilson
> > ---
> > drivers/gpu/drm/i915/intel_uc.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_uc.c
> > b/drivers/gpu/drm/i915/intel_uc.c
> > index 07c5658..940a3c9 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > @@ -412,8 +412,11 @@ void intel_uc_fini_hw(struct drm_i915_private
> > *dev_priv)
> >
> > if (i915.enable_guc_submission) {
> > i915_guc_submission_disable(dev_priv);
> > + guc_disable_communication(&dev_priv->guc);
> > gen9_disable_guc_interrupts(dev_priv);
> > i915_guc_submission_fini(dev_priv);
> > + } else {
> > + guc_disable_communication(&dev_priv->guc);
> > }
>
> Hmm, is the order that sensitive? Do we initialise it in a different
> order depending on guc submission? Seems dubious.
Order is the same, but some steps are omitted.
With submission enabled:
1) i915_ggtt_enable_guc(dev_priv);
2) i915_guc_submission_init(dev_priv);
3) intel_guc_init_hw(&dev_priv->guc);
4) guc_enable_communication(guc);
5) gen9_enable_guc_interrupts(dev_priv);
6) i915_guc_submission_enable(dev_priv);
without:
1) i915_ggtt_enable_guc(dev_priv);
3) intel_guc_init_hw(&dev_priv->guc);
4) guc_enable_communication(guc);
Alternatively, we can split "if (i915.enable_guc_submission)" into
two parts, and put guc_disable_communication() in the middle.
-Michal
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915/guc: Disable send function on fini
On Fri, May 12, 2017 at 03:02:58PM +, Michal Wajdeczko wrote:
> In earlier patch 789a625 we were enabling send function only
> after successful init. For completeness, we should make sure
> that we disable it on fini.
>
> Signed-off-by: Michal Wajdeczko
> Cc: Joonas Lahtinen
> Cc: Daniele Ceraolo Spurio
> Cc: Chris Wilson
> ---
> drivers/gpu/drm/i915/intel_uc.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 07c5658..940a3c9 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -412,8 +412,11 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>
> if (i915.enable_guc_submission) {
> i915_guc_submission_disable(dev_priv);
> + guc_disable_communication(&dev_priv->guc);
> gen9_disable_guc_interrupts(dev_priv);
> i915_guc_submission_fini(dev_priv);
> + } else {
> + guc_disable_communication(&dev_priv->guc);
> }
Hmm, is the order that sensitive? Do we initialise it in a different
order depending on guc submission? Seems dubious.
-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 3/3] HAX Enable GuC loading & submission
This is just for CI testing, *** DO NOT MERGE ***
Signed-off-by: Michal Wajdeczko
---
drivers/gpu/drm/i915/i915_params.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_params.c
b/drivers/gpu/drm/i915/i915_params.c
index b6a7e36..abd2894 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -56,8 +56,8 @@ struct i915_params i915 __read_mostly = {
.verbose_state_checks = 1,
.nuclear_pageflip = 0,
.edp_vswing = 0,
- .enable_guc_loading = 0,
- .enable_guc_submission = 0,
+ .enable_guc_loading = 1,
+ .enable_guc_submission = 1,
.guc_log_level = -1,
.guc_firmware_path = NULL,
.huc_firmware_path = NULL,
@@ -221,12 +221,12 @@ MODULE_PARM_DESC(edp_vswing,
module_param_named_unsafe(enable_guc_loading, i915.enable_guc_loading, int,
0400);
MODULE_PARM_DESC(enable_guc_loading,
"Enable GuC firmware loading "
- "(-1=auto, 0=never [default], 1=if available, 2=required)");
+ "(-1=auto, 0=never, 1=if available [default], 2=required)");
module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission,
int, 0400);
MODULE_PARM_DESC(enable_guc_submission,
"Enable GuC submission "
- "(-1=auto, 0=never [default], 1=if available, 2=required)");
+ "(-1=auto, 0=never, 1=if available [default], 2=required)");
module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
MODULE_PARM_DESC(guc_log_level,
--
2.7.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/3] drm/i915/guc: Introduce buffer based cmd transport
Buffer based command transport can replace MMIO based mechanism.
It may be used to perform host-2-guc and guc-to-host communication.
Portions of this patch are based on work by:
Michel Thierry
Robert Beckett
Daniele Ceraolo Spurio
Signed-off-by: Michal Wajdeczko
Cc: Daniele Ceraolo Spurio
Cc: Oscar Mateo
Cc: Joonas Lahtinen
---
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/i915_drv.c | 2 +
drivers/gpu/drm/i915/i915_drv.h | 2 +
drivers/gpu/drm/i915/i915_utils.h | 4 +
drivers/gpu/drm/i915/intel_guc_ct.c | 440 ++
drivers/gpu/drm/i915/intel_guc_ct.h | 92 +++
drivers/gpu/drm/i915/intel_guc_fwif.h | 42
drivers/gpu/drm/i915/intel_uc.c | 25 +-
drivers/gpu/drm/i915/intel_uc.h | 4 +-
9 files changed, 610 insertions(+), 2 deletions(-)
create mode 100644 drivers/gpu/drm/i915/intel_guc_ct.c
create mode 100644 drivers/gpu/drm/i915/intel_guc_ct.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 7b05fb8..16dccf5 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -58,6 +58,7 @@ i915-y += i915_cmd_parser.o \
# general-purpose microcontroller (GuC) support
i915-y += intel_uc.o \
+ intel_guc_ct.o \
intel_guc_log.o \
intel_guc_loader.o \
intel_huc.o \
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 72fb47a..71f7915 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -869,6 +869,7 @@ static int i915_driver_init_early(struct drm_i915_private
*dev_priv,
i915_workqueues_cleanup(dev_priv);
err_engines:
i915_engines_cleanup(dev_priv);
+ intel_uc_cleanup(dev_priv);
return ret;
}
@@ -883,6 +884,7 @@ static void i915_driver_cleanup_early(struct
drm_i915_private *dev_priv)
intel_irq_fini(dev_priv);
i915_workqueues_cleanup(dev_priv);
i915_engines_cleanup(dev_priv);
+ intel_uc_cleanup(dev_priv);
}
static int i915_mmio_setup(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ff3574a..ec2d45f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -712,6 +712,7 @@ struct intel_csr {
func(has_gmbus_irq); \
func(has_gmch_display); \
func(has_guc); \
+ func(has_guc_ct); \
func(has_hotplug); \
func(has_l3_dpf); \
func(has_llc); \
@@ -2831,6 +2832,7 @@ intel_info(const struct drm_i915_private *dev_priv)
* properties, so we have separate macros to test them.
*/
#define HAS_GUC(dev_priv) ((dev_priv)->info.has_guc)
+#define HAS_GUC_CT(dev_priv) ((dev_priv)->info.has_guc_ct)
#define HAS_GUC_UCODE(dev_priv)(HAS_GUC(dev_priv))
#define HAS_GUC_SCHED(dev_priv)(HAS_GUC(dev_priv))
#define HAS_HUC_UCODE(dev_priv)(HAS_GUC(dev_priv))
diff --git a/drivers/gpu/drm/i915/i915_utils.h
b/drivers/gpu/drm/i915/i915_utils.h
index f9d6607..cd4a0d9 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -86,6 +86,10 @@
#define ptr_offset(ptr, member) offsetof(typeof(*(ptr)), member)
+#define arr_offset(arr, index, member) ({ \
+ (index) * sizeof((arr)[0]) + ptr_offset(&(arr)[0], member); \
+})
+
#define fetch_and_zero(ptr) ({ \
typeof(*ptr) __T = *(ptr); \
*(ptr) = (typeof(*ptr))0; \
diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c
b/drivers/gpu/drm/i915/intel_guc_ct.c
new file mode 100644
index 000..6eb46e0
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_guc_ct.c
@@ -0,0 +1,440 @@
+/*
+ * Copyright © 2016-2017 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
[Intel-gfx] [PATCH 1/3] drm/i915/guc: Disable send function on fini
In earlier patch 789a625 we were enabling send function only
after successful init. For completeness, we should make sure
that we disable it on fini.
Signed-off-by: Michal Wajdeczko
Cc: Joonas Lahtinen
Cc: Daniele Ceraolo Spurio
Cc: Chris Wilson
---
drivers/gpu/drm/i915/intel_uc.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 07c5658..940a3c9 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -412,8 +412,11 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
if (i915.enable_guc_submission) {
i915_guc_submission_disable(dev_priv);
+ guc_disable_communication(&dev_priv->guc);
gen9_disable_guc_interrupts(dev_priv);
i915_guc_submission_fini(dev_priv);
+ } else {
+ guc_disable_communication(&dev_priv->guc);
}
i915_ggtt_disable_guc(dev_priv);
}
--
2.7.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: Compute the fw_domain id from the mask
== Series Details == Series: series starting with [1/2] drm/i915: Compute the fw_domain id from the mask URL : https://patchwork.freedesktop.org/series/24359/ State : failure == Summary == Series 24359v1 Series without cover letter https://patchwork.freedesktop.org/api/1.0/series/24359/revisions/1/mbox/ Test kms_pipe_crc_basic: Subgroup suspend-read-crc-pipe-b: skip -> INCOMPLETE (fi-bsw-n3050) fdo#100989 Test pm_rpm: Subgroup basic-pci-d3-state: pass -> DMESG-WARN (fi-byt-j1900) pass -> DMESG-WARN (fi-byt-n2820) pass -> INCOMPLETE (fi-skl-6700k) pass -> INCOMPLETE (fi-kbl-7500u) pass -> DMESG-WARN (fi-kbl-7560u) Subgroup basic-rte: pass -> DMESG-WARN (fi-byt-j1900) pass -> DMESG-WARN (fi-byt-n2820) pass -> INCOMPLETE (fi-skl-6700hq) pass -> INCOMPLETE (fi-kbl-7560u) Test prime_vgem: Subgroup basic-fence-flip: pass -> DMESG-WARN (fi-byt-j1900) pass -> DMESG-WARN (fi-byt-n2820) fdo#100989 https://bugs.freedesktop.org/show_bug.cgi?id=100989 fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time:449s fi-bdw-gvtdvmtotal:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:432s fi-bsw-n3050 total:236 pass:204 dwarn:0 dfail:0 fail:0 skip:31 fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:516s fi-byt-j1900 total:278 pass:251 dwarn:3 dfail:0 fail:0 skip:24 time:493s fi-byt-n2820 total:278 pass:247 dwarn:3 dfail:0 fail:0 skip:28 time:487s fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:418s fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:408s fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:418s fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:506s fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:466s fi-kbl-7500u total:241 pass:218 dwarn:5 dfail:0 fail:0 skip:17 fi-kbl-7560u total:242 pass:227 dwarn:6 dfail:0 fail:0 skip:8 fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:458s fi-skl-6700hqtotal:242 pass:226 dwarn:0 dfail:0 fail:0 skip:15 fi-skl-6700k total:241 pass:223 dwarn:0 dfail:0 fail:0 skip:17 fi-skl-6770hqtotal:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:504s fi-skl-gvtdvmtotal:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time:436s fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:537s fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time:398s fi-bxt-t5700 failed to collect. IGT log at Patchwork_4684/fi-bxt-t5700/igt.log 38d26beef9f31fa7ca983563cf86d4131defd94a drm-tip: 2017y-05m-12d-13h-15m-01s UTC integration manifest c88076a932 drm/i915: Keep the forcewake timer alive for 1ms past the most recent use 19ec9ce drm/i915: Compute the fw_domain id from the mask == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4684/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] dma-buf/sync-file: Defer creation of sync_file->name
Hi Chris,
Thanks for the patch!
2017-05-12 Chris Wilson :
> Constructing the name takes the majority of the time for allocating a
> sync_file to wrap a fence, and the name is very rarely used (only via
> the sync_file status user interface). To reduce the impact on the common
> path (that of creating sync_file to pass around), defer the construction
> of the name until it is first used.
>
> Signed-off-by: Chris Wilson
> Cc: Sumit Semwal
> Cc: Gustavo Padovan
> ---
> drivers/dma-buf/sync_file.c | 18 +++---
> include/linux/sync_file.h | 2 +-
> 2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 2321035f6204..2cccfa834778 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -82,11 +82,6 @@ struct sync_file *sync_file_create(struct dma_fence *fence)
>
> sync_file->fence = dma_fence_get(fence);
>
> - snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
> - fence->ops->get_driver_name(fence),
> - fence->ops->get_timeline_name(fence), fence->context,
> - fence->seqno);
> -
> return sync_file;
> }
> EXPORT_SYMBOL(sync_file_create);
> @@ -268,7 +263,7 @@ static struct sync_file *sync_file_merge(const char
> *name, struct sync_file *a,
> goto err;
> }
>
> - strlcpy(sync_file->name, name, sizeof(sync_file->name));
> + strlcpy(sync_file->user_name, name, sizeof(sync_file->user_name));
> return sync_file;
>
> err:
> @@ -422,7 +417,16 @@ static long sync_file_ioctl_fence_info(struct sync_file
> *sync_file,
> }
>
> no_fences:
> - strlcpy(info.name, sync_file->name, sizeof(info.name));
> + if (!sync_file->user_name[0]) {
> + scnprintf(sync_file->user_name,
> + sizeof(sync_file->user_name),
> + "%s-%s%llu-%d",
> +
> sync_file->fence->ops->get_driver_name(sync_file->fence),
> +
> sync_file->fence->ops->get_timeline_name(sync_file->fence),
> + sync_file->fence->context,
> + sync_file->fence->seqno);
> + }
> + strlcpy(info.name, sync_file->user_name, sizeof(info.name));
> info.status = dma_fence_is_signaled(sync_file->fence);
> info.num_fences = num_fences;
>
> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
> index 3e3ab84fc4cd..0cbeff29f510 100644
> --- a/include/linux/sync_file.h
> +++ b/include/linux/sync_file.h
> @@ -34,7 +34,7 @@
> struct sync_file {
> struct file *file;
> struct kref kref;
> - charname[32];
> + charuser_name[32];
Looks good to me, but please re-spin it fixing the issue reported by the
test bot then I can apply it.
Gustavo
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] drm/i915: Compute the fw_domain id from the mask
Storing both the mask and the id is redundant as we can trivially
compute one from the other. As the mask is more frequently used, remove
the id.
Signed-off-by: Chris Wilson
Cc: Mika Kuoppala
Cc: Tvrtko Ursulin
---
drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
drivers/gpu/drm/i915/intel_uncore.c | 12 ++--
drivers/gpu/drm/i915/intel_uncore.h | 6 ++
3 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index bd9abef40c66..a2c9c3e792e1 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1468,7 +1468,7 @@ static int i915_forcewake_domains(struct seq_file *m,
void *data)
for_each_fw_domain(fw_domain, i915, tmp)
seq_printf(m, "%s.wake_count = %u\n",
- intel_uncore_forcewake_domain_to_str(fw_domain->id),
+ intel_uncore_forcewake_domain_to_str(fw_domain),
READ_ONCE(fw_domain->wake_count));
return 0;
diff --git a/drivers/gpu/drm/i915/intel_uncore.c
b/drivers/gpu/drm/i915/intel_uncore.c
index 08d7d08438c0..7eaa592aed26 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -40,8 +40,10 @@ static const char * const forcewake_domain_names[] = {
};
const char *
-intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id)
+intel_uncore_forcewake_domain_to_str(const struct
intel_uncore_forcewake_domain *d)
{
+ unsigned int id = ilog2(d->mask);
+
BUILD_BUG_ON(ARRAY_SIZE(forcewake_domain_names) != FW_DOMAIN_ID_COUNT);
if (id >= 0 && id < FW_DOMAIN_ID_COUNT)
@@ -77,7 +79,7 @@ fw_domain_wait_ack_clear(const struct drm_i915_private *i915,
FORCEWAKE_KERNEL) == 0,
FORCEWAKE_ACK_TIMEOUT_MS))
DRM_ERROR("%s: timed out waiting for forcewake ack to clear.\n",
- intel_uncore_forcewake_domain_to_str(d->id));
+ intel_uncore_forcewake_domain_to_str(d));
}
static inline void
@@ -95,7 +97,7 @@ fw_domain_wait_ack(const struct drm_i915_private *i915,
FORCEWAKE_KERNEL),
FORCEWAKE_ACK_TIMEOUT_MS))
DRM_ERROR("%s: timed out waiting for forcewake ack request.\n",
- intel_uncore_forcewake_domain_to_str(d->id));
+ intel_uncore_forcewake_domain_to_str(d));
}
static inline void
@@ -209,7 +211,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
struct intel_uncore_forcewake_domain *domain =
container_of(timer, struct intel_uncore_forcewake_domain, timer);
struct drm_i915_private *dev_priv =
- container_of(domain, struct drm_i915_private,
uncore.fw_domain[domain->id]);
+ container_of(domain, struct drm_i915_private,
uncore.fw_domain[ilog2(domain->mask)]);
unsigned long irqflags;
assert_rpm_device_not_suspended(dev_priv);
@@ -1149,8 +1151,6 @@ static void fw_domain_init(struct drm_i915_private
*dev_priv,
d->reg_set = reg_set;
d->reg_ack = reg_ack;
- d->id = domain_id;
-
BUILD_BUG_ON(FORCEWAKE_RENDER != (1 << FW_DOMAIN_ID_RENDER));
BUILD_BUG_ON(FORCEWAKE_BLITTER != (1 << FW_DOMAIN_ID_BLITTER));
BUILD_BUG_ON(FORCEWAKE_MEDIA != (1 << FW_DOMAIN_ID_MEDIA));
diff --git a/drivers/gpu/drm/i915/intel_uncore.h
b/drivers/gpu/drm/i915/intel_uncore.h
index ff6fe2bb0ccf..5fec5fd4346c 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -93,8 +93,7 @@ struct intel_uncore {
u32 fw_reset;
struct intel_uncore_forcewake_domain {
- enum forcewake_domain_id id;
- enum forcewake_domains mask;
+ unsigned int mask;
unsigned int wake_count;
struct hrtimer timer;
i915_reg_t reg_set;
@@ -123,8 +122,7 @@ void intel_uncore_resume_early(struct drm_i915_private
*dev_priv);
u64 intel_uncore_edram_size(struct drm_i915_private *dev_priv);
void assert_forcewakes_inactive(struct drm_i915_private *dev_priv);
-const char *intel_uncore_forcewake_domain_to_str(const enum
forcewake_domain_id id);
-
+const char *intel_uncore_forcewake_domain_to_str(const struct
intel_uncore_forcewake_domain *domain);
enum forcewake_domains
intel_uncore_forcewake_for_reg(struct drm_i915_private *dev_priv,
i915_reg_t reg, unsigned int op);
--
2.11.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: Keep the forcewake timer alive for 1ms past the most recent use
Currently the timer is armed for 1ms after the first use and is killed
immediately, dropping the forcewake as early as possible. However, for
very frequent operations the forcewake dance has a large impact on
latency and keeping the timer alive until we are idle is preferred. To
achieve this, if we call intel_uncore_forcewake_get whilst the timer is
alive (repeated use), then set a flag to restart the timer on expiry
rather than drop the forcewake usage count. The timer is racy, the
consequence of the race is to expire the timer earlier than is now
desired but does not impact on correct behaviour. The offset the race
slightly, we set the active flag again on intel_uncore_forcewake_put.
Signed-off-by: Chris Wilson
Cc: Mika Kuoppala
Cc: Tvrtko Ursulin
---
drivers/gpu/drm/i915/intel_uncore.c | 14 +++---
drivers/gpu/drm/i915/intel_uncore.h | 1 +
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c
b/drivers/gpu/drm/i915/intel_uncore.c
index 7eaa592aed26..26b6c0c14441 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -216,6 +216,9 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
assert_rpm_device_not_suspended(dev_priv);
+ if (xchg(&domain->active, false))
+ return HRTIMER_RESTART;
+
spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
if (WARN_ON(domain->wake_count == 0))
domain->wake_count++;
@@ -453,9 +456,12 @@ static void __intel_uncore_forcewake_get(struct
drm_i915_private *dev_priv,
fw_domains &= dev_priv->uncore.fw_domains;
- for_each_fw_domain_masked(domain, fw_domains, dev_priv, tmp)
- if (domain->wake_count++)
+ for_each_fw_domain_masked(domain, fw_domains, dev_priv, tmp) {
+ if (domain->wake_count++) {
fw_domains &= ~domain->mask;
+ domain->active = true;
+ }
+ }
if (fw_domains)
dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_domains);
@@ -520,8 +526,10 @@ static void __intel_uncore_forcewake_put(struct
drm_i915_private *dev_priv,
if (WARN_ON(domain->wake_count == 0))
continue;
- if (--domain->wake_count)
+ if (--domain->wake_count) {
+ domain->active = true;
continue;
+ }
fw_domain_arm_timer(domain);
}
diff --git a/drivers/gpu/drm/i915/intel_uncore.h
b/drivers/gpu/drm/i915/intel_uncore.h
index 5fec5fd4346c..dfead585835c 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -95,6 +95,7 @@ struct intel_uncore {
struct intel_uncore_forcewake_domain {
unsigned int mask;
unsigned int wake_count;
+ bool active;
struct hrtimer timer;
i915_reg_t reg_set;
i915_reg_t reg_ack;
--
2.11.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/11] drm/i915/skl+: Watermark calculation cleanup
Hi,
On Friday 12 May 2017 05:53 AM, Matt Roper wrote:
On Mon, May 08, 2017 at 05:18:59PM +0530, Mahesh Kumar wrote:
This patch cleanup/reorganises the watermark calculation functions.
This patch also make use of already available macro
"drm_atomic_crtc_state_for_each_plane_state" to walk through
plane_state list instead of calculating plane_state in function itself.
Now we iterate over WM levels in skl_compute_wm_level function instead
of "skl_build_pipe_wm" function.
I'd split the minor cleanup (const-ifying,
drm_atomic_crtc_state_for_each_plane_state usage, etc.) into a separate
patch from the code flow change (looping over levels in
skl_compute_wm_level instead of skl_build_pipe_wm). You'll probably
also want to rename the function to skl_compute_wm_level*s* (plural) if
its going to calculate all levels now instead of just one.
const-ifying is required by drm_atomic_crtc_state_for_each_plane_state
Macro,
dividing it into 2 patches one to use the the macro & another one to
move level iteration in separate
skl_compute_wm_level*s *function.
-Mahesh
Matt
This restructuring will help later patch for new DDB allocation
algorithm to do only algo related changes.
Signed-off-by: Mahesh Kumar
---
drivers/gpu/drm/i915/intel_pm.c | 89 +
1 file changed, 37 insertions(+), 52 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0ace94d67432..9d0225862a7e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3732,7 +3732,7 @@ static uint_fixed_16_16_t skl_wm_method2(uint32_t
pixel_rate,
}
static uint32_t skl_adjusted_plane_pixel_rate(const struct intel_crtc_state *cstate,
- struct intel_plane_state *pstate)
+ const struct intel_plane_state
*pstate)
{
uint64_t adjusted_pixel_rate;
uint_fixed_16_16_t downscale_amount;
@@ -3754,7 +3754,7 @@ static uint32_t skl_adjusted_plane_pixel_rate(const
struct intel_crtc_state *cst
static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
struct intel_crtc_state *cstate,
- struct intel_plane_state *intel_pstate,
+ const struct intel_plane_state *intel_pstate,
uint16_t ddb_allocation,
int level,
uint16_t *out_blocks, /* out */
@@ -3762,8 +3762,8 @@ static int skl_compute_plane_wm(const struct
drm_i915_private *dev_priv,
bool *enabled /* out */)
{
struct intel_plane *plane = to_intel_plane(intel_pstate->base.plane);
- struct drm_plane_state *pstate = &intel_pstate->base;
- struct drm_framebuffer *fb = pstate->fb;
+ const struct drm_plane_state *pstate = &intel_pstate->base;
+ const struct drm_framebuffer *fb = pstate->fb;
uint32_t latency = dev_priv->wm.skl_latency[level];
uint_fixed_16_16_t method1, method2;
uint_fixed_16_16_t plane_blocks_per_line;
@@ -3918,52 +3918,36 @@ static int
skl_compute_wm_level(const struct drm_i915_private *dev_priv,
struct skl_ddb_allocation *ddb,
struct intel_crtc_state *cstate,
-struct intel_plane *intel_plane,
-int level,
-struct skl_wm_level *result)
+const struct intel_plane_state *intel_pstate,
+struct skl_plane_wm *wm)
{
- struct drm_atomic_state *state = cstate->base.state;
struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
- struct drm_plane *plane = &intel_plane->base;
- struct intel_plane_state *intel_pstate = NULL;
+ struct drm_plane *plane = intel_pstate->base.plane;
+ struct intel_plane *intel_plane = to_intel_plane(plane);
uint16_t ddb_blocks;
enum pipe pipe = intel_crtc->pipe;
+ int level, max_level = ilk_wm_max_level(dev_priv);
int ret;
- if (state)
- intel_pstate =
- intel_atomic_get_existing_plane_state(state,
- intel_plane);
-
- /*
-* Note: If we start supporting multiple pending atomic commits against
-* the same planes/CRTC's in the future, plane->state will no longer be
-* the correct pre-state to use for the calculations here and we'll
-* need to change where we get the 'unchanged' plane data from.
-*
-* For now this is fine because we only allow one queued commit against
-* a CRTC. Even if the plane isn't modified by this transaction and we
-* don't have a plane lock, we still have the CRTC's lock, so we know
-* that no other transactions are racing with us to update it.
-*/
-
Re: [Intel-gfx] [PATCH 07/11] drm/i915/skl+: Fail the flip if ddb min requirement exceeds pipe allocation
On Friday 12 May 2017 05:53 AM, Matt Roper wrote:
On Mon, May 08, 2017 at 05:18:58PM +0530, Mahesh Kumar wrote:
DDB minimum requirement may exceed the allocated DDB for CRTC/Pipe. This
patch make changes to fail the flip if minimum requirement for pipe
exceeds the total ddb allocated to the pipe.
Previously it succeeded but making alloc_size a negative value. Which
will make later calculations for plane ddb allocation bogus & may lead
to screen corruption or system hang.
I think originally the bspec defined the initial minimum DDB allocation
for each plane to just be '8' in all cases. So even with 4 planes on a
pipe, your initial pipe minimum would only be 32, so the code here never
had a risk of overflowing (note that the initial minimum is different
than the WM0 minimum requirement we ultimately wind up calculating).
But then the bspec was updated to suggest a higher initial minimum for
y-tiled buffers (calculated based on the width of the buffer), so our
initial assumptions here don't really hold anymore. We should still be
safe if we're not using y-tile, but even a single 4k y-tiled plane would
have an initial minimum of something like 250 blocks iirc., so we can't
assume we're always safe anymore.
Signed-off-by: Mahesh Kumar
---
drivers/gpu/drm/i915/intel_pm.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2a4e9d89cd6f..0ace94d67432 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3591,6 +3591,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
int num_active;
unsigned plane_data_rate[I915_MAX_PLANES] = {};
unsigned plane_y_data_rate[I915_MAX_PLANES] = {};
+ uint16_t total_min_blocks = 0;
/* Clear the partitioning for disabled planes. */
memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
@@ -3618,10 +3619,18 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
*/
for_each_plane_id_on_crtc(intel_crtc, plane_id) {
- alloc_size -= minimum[plane_id];
- alloc_size -= y_minimum[plane_id];
+ total_min_blocks += minimum[plane_id];
+ total_min_blocks += y_minimum[plane_id];
}
+ if ((total_min_blocks > alloc_size)) {
+ DRM_DEBUG_KMS("Requested display configuration exceeds system DDB
limitations");
+ DRM_DEBUG_KMS("minimum required %d/%d\n", total_min_blocks,
+ alloc_size);
+ return-EINVAL;
I think you're missing a space here?
With that fixed,
I think I already fixed this in newer version of patch. :),
anyway resubmitting complete series again with other comments addressed.
thanks,
-Mahesh
Reviewed-by: Matt Roper
+ }
+
+ alloc_size -= total_min_blocks;
ddb->plane[pipe][PLANE_CURSOR].start = alloc->end -
minimum[PLANE_CURSOR];
ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
--
2.11.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] tests/initial_state: Add a test to capture the state of the GPU
On Fri, May 12, 2017 at 04:08:48PM +0300, Martin Peres wrote:
> The commit name "Add a test to capture the state of the GPU" should
> likely be renamed to "Add a test that checks the current state of
> the driver".
>
> On 12/05/17 14:07, Marta Lofstedt wrote:
> >When running testlist for CI iot would be good to know if
>
> iot -> it
>
> >the state of the GPU is as expected. This adds a subtest to check
> >if the GPU is wedged.
>
> This is not mandatory, but how about adding:
>
> "When coupled with piglit's abort-on feature, this makes the piglit
> results contain the reason for the abort when the driver's initial
> state is unacceptable, improving debuggability"
>
> With the commit title and the typo fixed, the patch is:
> Acked-by: Martin Peres
>
> >
> >Signed-off-by: Marta Lofstedt
> >---
> > tests/Makefile.sources| 1 +
> > tests/initial_state.c | 52
> > +++
> > tests/intel-ci/extended.testlist | 1 +
> > tests/intel-ci/fast-feedback.testlist | 1 +
> > 4 files changed, 55 insertions(+)
> > create mode 100644 tests/initial_state.c
> >
> >diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> >index 9553e4d9..32431a05 100644
> >--- a/tests/Makefile.sources
> >+++ b/tests/Makefile.sources
> >@@ -152,6 +152,7 @@ TESTS_progs_M = \
> > vgem_basic \
> > vgem_slow \
> > meta_test \
> >+initial_state \
> > $(NULL)
> > if HAVE_CHAMELIUM
> >diff --git a/tests/initial_state.c b/tests/initial_state.c
> >new file mode 100644
> >index ..1c5d9a74
> >--- /dev/null
> >+++ b/tests/initial_state.c
> >@@ -0,0 +1,52 @@
> >+/*
> >+ * Copyright © 2017 Intel Corporation
> >+ *
> >+ * Permission is hereby granted, free of charge, to any person obtaining a
> >+ * copy of this software and associated documentation files (the
> >"Software"),
> >+ * to deal in the Software without restriction, including without limitation
> >+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> >+ * and/or sell copies of the Software, and to permit persons to whom the
> >+ * Software is furnished to do so, subject to the following conditions:
> >+ *
> >+ * The above copyright notice and this permission notice (including the next
> >+ * paragraph) shall be included in all copies or substantial portions of the
> >+ * Software.
> >+ *
> >+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> >OR
> >+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> >+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> >+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> >OTHER
> >+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> >+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> >DEALINGS
> >+ * IN THE SOFTWARE.
> >+ *
> >+ */
> >+
> >+#include
> >+#include
> >+#include "igt.h"
> >+
> >+/*
> >+ * The purpose of this test is to capture the
> >+ * state of the GPU before running a testlist.
> >+ *
> >+ * Note, this test currently only checks if
> >+ * i915 is wedged. But, it could be extended to report
> >+ * on other bad initial states.
> >+ */
> >+
> >+static void is_wedged(void)
> >+{
> >+char buf[128];
> >+int fd = drm_open_driver(DRIVER_INTEL);
> >+
> >+igt_debugfs_read(fd, "i915_wedged", buf);
The ABI for testing whether the device is wedged before use is
gem_throttle(). This is encapsulated into igt_require_gem() and any test
that depends upon execution on the GPU should be checking for GEM first.
This test should just be igt_gem_require(drm_open_driver(DRIVER_INTEL));
-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 v7 3/9] drm/i915: Drop AUX backlight enable check for backlight control
On Fri, 12 May 2017, "Pandiyan, Dhinakaran"
wrote:
> On Thu, 2017-05-11 at 16:02 -0700, Puthikorn Voravootivat wrote:
>> There are some panel that
>> (1) does not support display backlight enable via AUX
>> (2) support display backlight adjustment via AUX
>> (3) support display backlight enable via eDP BL_ENABLE pin
>>
>> The current driver required that (1) must be support to enable (2).
>> This patch drops that requirement.
>>
>
> You sent this version before I finished my follow-up questions, copying
> the conversation here for context.
Puthikorn, please don't send new versions before the review is
addressed.
Pushed patches 1, 2, 5, and 7. Thanks for the patches and review.
BR,
Jani.
> DK: Won't DP_EDP_BACKLIGHT_AUX_ENABLE_CAP be 1 always? The code below,
> in
> intel_dp_aux_display_control_capable(), makes sure
> DP_EDP_BACKLIGHT_PIN_ENABLE_CAP=0. The spec says at least one of these
> has to be 1.
>
> Puthikorn: We will drop the DP_EDP_BACKLIGHT_PIN_ENABLE_CAP != 0 check
> in next patch set.
> This patch adds check here to prepare for that.
>
>
> 1) So, this patch does not really fix what the commit message claims
> because it is dependent on the following patch. Does it make sense to
> remove this check in this patch? That way, this patch by itself is the
> fix that the commit message says.
>
> - !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP)
>
>
> 2) If a panel supports backlight enable via AUX and BL_ENABLE pin, this
> patch (along with the next) enables backlight twice, doesn't it?
> _intel_edp_backlight_on(intel_dp) in intel_dp.c is called
> unconditionally after intel_dp_aux_enable_backlight(). I don't know how
> likely this configuration is or if it's alright to enable via both AUX
> and BL_ENABLE pin.
>
>
>
>
>> Signed-off-by: Puthikorn Voravootivat
>> ---
>> drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 5 -
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> index 870c03fc0f3a..c22712762957 100644
>> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> @@ -28,6 +28,10 @@ static void set_aux_backlight_enable(struct intel_dp
>> *intel_dp, bool enable)
>> {
>> uint8_t reg_val = 0;
>>
>> + /* Early return when display use other mechanism to enable
>> backlight. */
>> +if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))
>> +return;
>> +
>> if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
>>®_val) < 0) {
>> DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
>> @@ -164,7 +168,6 @@ intel_dp_aux_display_control_capable(struct
>> intel_connector *connector)
>> * the panel can support backlight control over the aux channel
>> */
>> if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP &&
>> -(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
>> (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) &&
>> !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) ||
>>(intel_dp->edp_dpcd[2] &
>> DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))) {
>
--
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] tests/initial_state: Add a test to capture the state of the GPU
The commit name "Add a test to capture the state of the GPU" should
likely be renamed to "Add a test that checks the current state of the
driver".
On 12/05/17 14:07, Marta Lofstedt wrote:
When running testlist for CI iot would be good to know if
iot -> it
the state of the GPU is as expected. This adds a subtest to check
if the GPU is wedged.
This is not mandatory, but how about adding:
"When coupled with piglit's abort-on feature, this makes the piglit
results contain the reason for the abort when the driver's initial state
is unacceptable, improving debuggability"
With the commit title and the typo fixed, the patch is:
Acked-by: Martin Peres
Signed-off-by: Marta Lofstedt
---
tests/Makefile.sources| 1 +
tests/initial_state.c | 52 +++
tests/intel-ci/extended.testlist | 1 +
tests/intel-ci/fast-feedback.testlist | 1 +
4 files changed, 55 insertions(+)
create mode 100644 tests/initial_state.c
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 9553e4d9..32431a05 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -152,6 +152,7 @@ TESTS_progs_M = \
vgem_basic \
vgem_slow \
meta_test \
+ initial_state \
$(NULL)
if HAVE_CHAMELIUM
diff --git a/tests/initial_state.c b/tests/initial_state.c
new file mode 100644
index ..1c5d9a74
--- /dev/null
+++ b/tests/initial_state.c
@@ -0,0 +1,52 @@
+/*
+ * Copyright © 2017 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include
+#include
+#include "igt.h"
+
+/*
+ * The purpose of this test is to capture the
+ * state of the GPU before running a testlist.
+ *
+ * Note, this test currently only checks if
+ * i915 is wedged. But, it could be extended to report
+ * on other bad initial states.
+ */
+
+static void is_wedged(void)
+{
+ char buf[128];
+ int fd = drm_open_driver(DRIVER_INTEL);
+
+ igt_debugfs_read(fd, "i915_wedged", buf);
+ igt_assert(!strstr(buf, "1"));
+}
+
+igt_main
+{
+
+ igt_subtest("is_wedged")
+ is_wedged();
+}
diff --git a/tests/intel-ci/extended.testlist b/tests/intel-ci/extended.testlist
index a16c9c84..2ec08b55 100644
--- a/tests/intel-ci/extended.testlist
+++ b/tests/intel-ci/extended.testlist
@@ -1,3 +1,4 @@
+igt@initial_state@is_wedged
igt@core_auth@many-magics
igt@core_getclient
igt@core_get_client_auth@master-drop
diff --git a/tests/intel-ci/fast-feedback.testlist
b/tests/intel-ci/fast-feedback.testlist
index 5ffa2cef..a6a0a810 100644
--- a/tests/intel-ci/fast-feedback.testlist
+++ b/tests/intel-ci/fast-feedback.testlist
@@ -1,3 +1,4 @@
+igt@initial_state@is_wedged
igt@core_auth@basic-auth
igt@core_prop_blob@basic
igt@drv_getparams_basic@basic-eu-total
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] dma-buf/sync-file: Defer creation of sync_file->name
Hi Chris, [auto build test WARNING on linus/master] [also build test WARNING on v4.11 next-20170512] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Chris-Wilson/dma-buf-sync-file-Defer-creation-of-sync_file-name/20170512-193751 reproduce: make htmldocs All warnings (new ones prefixed by >>): WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org) arch/x86/include/asm/uaccess_32.h:1: warning: no structured comments found include/linux/init.h:1: warning: no structured comments found include/linux/mod_devicetable.h:686: warning: Excess struct/union/enum/typedef member 'ver_major' description in 'fsl_mc_device_id' include/linux/mod_devicetable.h:686: warning: Excess struct/union/enum/typedef member 'ver_minor' description in 'fsl_mc_device_id' kernel/sched/core.c:2088: warning: No description found for parameter 'rf' kernel/sched/core.c:2088: warning: Excess function parameter 'cookie' description in 'try_to_wake_up_local' include/linux/kthread.h:26: warning: Excess function parameter '...' description in 'kthread_create' kernel/sys.c:1: warning: no structured comments found include/linux/device.h:969: warning: No description found for parameter 'dma_ops' drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found >> include/linux/sync_file.h:47: warning: No description found for parameter >> 'user_name' >> include/linux/sync_file.h:47: warning: Excess struct/union/enum/typedef >> member 'name' description in 'sync_file' include/linux/iio/iio.h:597: warning: No description found for parameter 'trig_readonly' include/linux/iio/trigger.h:151: warning: No description found for parameter 'indio_dev' include/linux/iio/trigger.h:151: warning: No description found for parameter 'trig' include/linux/device.h:970: warning: No description found for parameter 'dma_ops' include/linux/usb/gadget.h:230: warning: No description found for parameter 'claimed' include/linux/usb/gadget.h:230: warning: No description found for parameter 'enabled' include/linux/usb/gadget.h:408: warning: No description found for parameter 'quirk_altset_not_supp' include/linux/usb/gadget.h:408: warning: No description found for parameter 'quirk_stall_not_supp' include/linux/usb/gadget.h:408: warning: No description found for parameter 'quirk_zlp_not_supp' include/drm/drm_drv.h:524: warning: No description found for parameter 'set_busid' include/drm/drm_drv.h:524: warning: No description found for parameter 'irq_handler' include/drm/drm_drv.h:524: warning: No description found for parameter 'irq_preinstall' include/drm/drm_drv.h:524: warning: No description found for parameter 'irq_postinstall' include/drm/drm_drv.h:524: warning: No description found for parameter 'irq_uninstall' include/drm/drm_drv.h:524: warning: No description found for parameter 'debugfs_init' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_open_object' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_close_object' include/drm/drm_drv.h:524: warning: No description found for parameter 'prime_handle_to_fd' include/drm/drm_drv.h:524: warning: No description found for parameter 'prime_fd_to_handle' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_export' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_import' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_pin' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_unpin' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_res_obj' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_get_sg_table' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_import_sg_table' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_vmap' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_vunmap' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_mmap' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_vm_ops' include/drm/drm_drv.h:524: warning: No description found for parameter &#x
Re: [Intel-gfx] [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations
Hi Lv, I am trying to reduce the number of parallel discussion we have on the same subject, but there is something here I can't let you have. On Fri, May 12, 2017 at 8:08 AM, Zheng, Lv wrote: > Hi, > > If my previous reply is not persuasive enough. > Let me do that in a different way. > >> From: linux-acpi-ow...@vger.kernel.org >> [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Zheng, >> Lv >> Subject: RE: [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() >> invocations >> >> Hi, >> >> > From: Benjamin Tissoires [mailto:benjamin.tissoi...@gmail.com] >> > Subject: Re: [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() >> > invocations >> > >> > On Tue, May 9, 2017 at 9:02 AM, Lv Zheng wrote: >> > > Since notification side has been changed to always notify kernel >> > > listeners >> > > using _LID returning value. Now listeners needn't invoke acpi_lid_open(), >> > > it should use a spec suggested control method lid device usage model: >> > > register lid notification and use the notified value instead, which is >> > > the >> > > only way to ensure the value is correct for >> > > "button.lid_init_state=ignore" >> > > mode or other modes with "button.lid_fake_events=N" specified. >> > > >> > > This patch fixes i915 driver to drop acpi_lid_open() user. It's not >> > > possible to change nouveau_connector.c user using a simple way now. So >> > > this >> > > patch only marks acpi_lid_open() as deprecated to accelerate this process >> > > by indicating this change to the nouveau developers. >> > >> > If the only 2 users of acpi_lid_open() are intel and nouveau, and if >> > they should rely on the value stored in the input node, not the one >> > exported by the _LID method (which can be wrong on some platforms), >> > how about simply changing the implementation of acpi_lid_open() to >> > fetch the value from the input node directly? > > If acpi_lid_open() returns stored value in input node, > then it actually returns the value we notified to the input layer. > While i915 and nouveau explicitly do not trust that value and invoke > acpi_lid_open(). > > For button.lid_init_state=method, PATCH 4/5 is useless as the values are same. > > However in my reply of PATCH 2. > I explained the difference of method/close, for the same reason, we can also > sense the difference of method/open. > The difference then can explain the usefulness of open/close modes. > > Given open/close modes are all useful: > button.lid_init_state=open > 1. button driver sends open to input layer after boot/resume > 2. i915/nouveau uses _LID return value after boot/resume > If _LID return value after boot/resume is "close", they are different. > > button.lid_init_state=close > 1. button driver sends close to input layer after boot/resume > 2. i915/nouveau uses _LID return value after boot/resume > If _LID return value after boot/resume is "open", they are different. > > The only way to be consistent to old i915/nouveau behavior is: > button.lid_init_state=open/close But these two modes are wrong in the absolute case: - a laptop has no reasons for not being powered up with the lid physically closed (wake on lan?) -> so open is an approximation already made on good assumption that there is not a high chance of the users powering/resuming the laptop with the lid closed - in the "close" case, this setting works for docked laptops, but if the laptop can be docked, it can also be undocked. And if you boot with button.lid_init_state=close, undock your laptop, go into suspend, resume -> the lid state is now "closed" while it should be opened. So no, these are just workarounds. i915/nouveau expect the acpi/button state to be reliable, or they should ignore it. But you can't fake events when you are not absolutely sure of what is happening. > 1. button driver sends open/close to input layer after boot/resume > 2. button driver sends _LID return value to i915 after boot/resume (PATCH 4) > So that i915 can just use the notified value in this patch (PATCH 5). > For nouveau, no change has been made for now, and as a concequence, > acpi_lid_open() is still kept but marked as deprecated. > >> >> See my reply of PATCH 4. >> It seems they trust _LID return value more than what we send to them. >> >> We can actually send faked "open/close" to them when >> button.lid_init_state=open/close is specified. >> >> So these 2 patches [PATCH 4-5/5] are used to do a small cleanup for lid >> event notifier APIs. >> I think they are not strictly related to the lid issues we are talking about. > > See Documentation/acpi/acpi-lid.txt: > The _LID control method is described to return the "current" lid state. > However the word of "current" has ambiguity, some buggy AML tables return > the lid state upon the last lid notification instead of returning the lid > state upon the last _LID evaluation. > > In order to have acpi lid event notifier API compliant to the above mentioned > both "buggy" and "nonbuggy" implementation. > The ensured lid event mod
Re: [Intel-gfx] [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations
On Tue, May 9, 2017 at 9:02 AM, Lv Zheng wrote:
> Since notification side has been changed to always notify kernel listeners
> using _LID returning value. Now listeners needn't invoke acpi_lid_open(),
> it should use a spec suggested control method lid device usage model:
> register lid notification and use the notified value instead, which is the
> only way to ensure the value is correct for "button.lid_init_state=ignore"
> mode or other modes with "button.lid_fake_events=N" specified.
>
> This patch fixes i915 driver to drop acpi_lid_open() user. It's not
> possible to change nouveau_connector.c user using a simple way now. So this
> patch only marks acpi_lid_open() as deprecated to accelerate this process
> by indicating this change to the nouveau developers.
If the only 2 users of acpi_lid_open() are intel and nouveau, and if
they should rely on the value stored in the input node, not the one
exported by the _LID method (which can be wrong on some platforms),
how about simply changing the implementation of acpi_lid_open() to
fetch the value from the input node directly?
Cheers,
Benjamin
>
> Cc:
> Cc:
> Signed-off-by: Lv Zheng
> ---
> drivers/acpi/button.c | 7 ++-
> drivers/gpu/drm/i915/intel_lvds.c | 2 +-
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 7ff3a75..50d7898 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -348,7 +348,12 @@ int acpi_lid_notifier_unregister(struct notifier_block
> *nb)
> }
> EXPORT_SYMBOL(acpi_lid_notifier_unregister);
>
> -int acpi_lid_open(void)
> +/*
> + * The intentional usage model is to register a lid notifier and use the
> + * notified value instead. Directly evaluating _LID without seeing a
> + * Notify(lid, 0x80) is not reliable.
> + */
> +int __deprecated acpi_lid_open(void)
> {
> if (!lid_device)
> return -ENODEV;
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c
> b/drivers/gpu/drm/i915/intel_lvds.c
> index 9ca4dc4..8ca9080 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -548,7 +548,7 @@ static int intel_lid_notify(struct notifier_block *nb,
> unsigned long val,
> /* Don't force modeset on machines where it causes a GPU lockup */
> if (dmi_check_system(intel_no_modeset_on_lid))
> goto exit;
> - if (!acpi_lid_open()) {
> + if (!val) {
> /* do modeset on next lid open event */
> dev_priv->modeset_restore = MODESET_ON_LID_OPEN;
> goto exit;
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ✗ Fi.CI.BAT: warning for drm/i915: don't do allocate_va_range again on pin update
On Fri, May 12, 2017 at 11:01:02AM -, Patchwork wrote: > == Series Details == > > Series: drm/i915: don't do allocate_va_range again on pin update > URL : https://patchwork.freedesktop.org/series/24342/ > State : warning > > == Summary == > > Series 24342v1 drm/i915: don't do allocate_va_range again on pin update > https://patchwork.freedesktop.org/api/1.0/series/24342/revisions/1/mbox/ > > Test gem_exec_suspend: > Subgroup basic-s4-devices: > pass -> DMESG-WARN (fi-kbl-7560u) fdo#100125 > Test kms_force_connector_basic: > Subgroup prune-stale-modes: > pass -> SKIP (fi-snb-2520m) > > fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125 Thanks for the fix Matthew, pushed it to dinq so we can get into the fixes queue promptly. -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/gen9: Reintroduce WaEnableYV12BugFixInHalfSliceChicken7
Arkadiusz Hiler writes:
> This basically reverts commit 465418c6064c
> ("drm/i915/gen9: Remove WaEnableYV12BugFixInHalfSliceChicken7")
> with small addition - marking it as affecting KBL as well.
s/KBL/GLK
Reviewed-by: Mika Kuoppala
>
> It was incorrectly considered fixed in production steppings.
>
> References: HSD#2126385, HSD#2131381, HSDES#1504433555, BSID#0764
> Cc: Mika Kuoppala
> Cc: Jeff McGee
> Signed-off-by: Arkadiusz Hiler
> ---
> drivers/gpu/drm/i915/intel_engine_cs.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c
> b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 483ed76..49c2315 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -851,8 +851,10 @@ static int gen9_init_workarounds(struct intel_engine_cs
> *engine)
>*/
> }
>
> + /* WaEnableYV12BugFixInHalfSliceChicken7:skl,bxt,kbl,glk */
> /* WaEnableSamplerGPGPUPreemptionSupport:skl,bxt,kbl */
> WA_SET_BIT_MASKED(GEN9_HALF_SLICE_CHICKEN7,
> + GEN9_ENABLE_YV12_BUGFIX |
> GEN9_ENABLE_GPGPU_PREEMPTION);
>
> /* Wa4x4STCOptimizationDisable:skl,bxt,kbl,glk */
> --
> 2.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/12] drm/i915: Remove kref from i915_sw_fence
Chris Wilson writes:
> My original intention was for i915_sw_fence to be the base class and
> provide the reference count for the container. This was from starting
> with a design to handle async_work. In practice, for i915 we embed
> fences into structs which have their own independent reference counting,
> making the i915_sw_fence.kref duplicitous. If we remove the kref, we
> remove the i915_sw_fence's ability to free itself and its independence,
> it can only exist within a container and must be supplied with a
> callback to handle its release.
>
> Signed-off-by: Chris Wilson
Reviewed-by: Mika Kuoppala
> ---
> drivers/gpu/drm/i915/i915_sw_fence.c | 55
>
> drivers/gpu/drm/i915/i915_sw_fence.h | 1 -
> 2 files changed, 11 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c
> b/drivers/gpu/drm/i915/i915_sw_fence.c
> index a277f8eb7beb..a0a690d6627e 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.c
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> @@ -120,34 +120,6 @@ void i915_sw_fence_fini(struct i915_sw_fence *fence)
> }
> #endif
>
> -static void i915_sw_fence_release(struct kref *kref)
> -{
> - struct i915_sw_fence *fence = container_of(kref, typeof(*fence), kref);
> -
> - WARN_ON(atomic_read(&fence->pending) > 0);
> - debug_fence_destroy(fence);
> -
> - if (fence->flags & I915_SW_FENCE_MASK) {
> - __i915_sw_fence_notify(fence, FENCE_FREE);
> - } else {
> - i915_sw_fence_fini(fence);
> - kfree(fence);
> - }
> -}
> -
> -static void i915_sw_fence_put(struct i915_sw_fence *fence)
> -{
> - debug_fence_assert(fence);
> - kref_put(&fence->kref, i915_sw_fence_release);
> -}
> -
> -static struct i915_sw_fence *i915_sw_fence_get(struct i915_sw_fence *fence)
> -{
> - debug_fence_assert(fence);
> - kref_get(&fence->kref);
> - return fence;
> -}
> -
> static void __i915_sw_fence_wake_up_all(struct i915_sw_fence *fence,
> struct list_head *continuation)
> {
> @@ -202,13 +174,15 @@ static void __i915_sw_fence_complete(struct
> i915_sw_fence *fence,
>
> debug_fence_set_state(fence, DEBUG_FENCE_IDLE, DEBUG_FENCE_NOTIFY);
>
> - if (fence->flags & I915_SW_FENCE_MASK &&
> - __i915_sw_fence_notify(fence, FENCE_COMPLETE) != NOTIFY_DONE)
> + if (__i915_sw_fence_notify(fence, FENCE_COMPLETE) != NOTIFY_DONE)
> return;
>
> debug_fence_set_state(fence, DEBUG_FENCE_NOTIFY, DEBUG_FENCE_IDLE);
>
> __i915_sw_fence_wake_up_all(fence, continuation);
> +
> + debug_fence_destroy(fence);
> + __i915_sw_fence_notify(fence, FENCE_FREE);
> }
>
> static void i915_sw_fence_complete(struct i915_sw_fence *fence)
> @@ -232,33 +206,26 @@ void __i915_sw_fence_init(struct i915_sw_fence *fence,
> const char *name,
> struct lock_class_key *key)
> {
> - BUG_ON((unsigned long)fn & ~I915_SW_FENCE_MASK);
> + BUG_ON(!fn || (unsigned long)fn & ~I915_SW_FENCE_MASK);
>
> debug_fence_init(fence);
>
> __init_waitqueue_head(&fence->wait, name, key);
> - kref_init(&fence->kref);
> atomic_set(&fence->pending, 1);
> fence->flags = (unsigned long)fn;
> }
>
> -static void __i915_sw_fence_commit(struct i915_sw_fence *fence)
> -{
> - i915_sw_fence_complete(fence);
> - i915_sw_fence_put(fence);
> -}
> -
> void i915_sw_fence_commit(struct i915_sw_fence *fence)
> {
> debug_fence_activate(fence);
> - __i915_sw_fence_commit(fence);
> + i915_sw_fence_complete(fence);
> }
>
> static int i915_sw_fence_wake(wait_queue_t *wq, unsigned mode, int flags,
> void *key)
> {
> list_del(&wq->task_list);
> __i915_sw_fence_complete(wq->private, key);
> - i915_sw_fence_put(wq->private);
> +
> if (wq->flags & I915_SW_FENCE_FLAG_ALLOC)
> kfree(wq);
> return 0;
> @@ -353,7 +320,7 @@ static int __i915_sw_fence_await_sw_fence(struct
> i915_sw_fence *fence,
> INIT_LIST_HEAD(&wq->task_list);
> wq->flags = pending;
> wq->func = i915_sw_fence_wake;
> - wq->private = i915_sw_fence_get(fence);
> + wq->private = fence;
>
> i915_sw_fence_await(fence);
>
> @@ -402,7 +369,7 @@ static void timer_i915_sw_fence_wake(unsigned long data)
> dma_fence_put(cb->dma);
> cb->dma = NULL;
>
> - __i915_sw_fence_commit(cb->fence);
> + i915_sw_fence_complete(cb->fence);
> cb->timer.function = NULL;
> }
>
> @@ -413,7 +380,7 @@ static void dma_i915_sw_fence_wake(struct dma_fence *dma,
>
> del_timer_sync(&cb->timer);
> if (cb->timer.function)
> - __i915_sw_fence_commit(cb->fence);
> + i915_sw_fence_complete(cb->fence);
> dma_fence_put(cb->dma);
>
> kfree(cb);
> @@ -440,7 +407,7 @@ int i915_sw_fence_await_dma_fence(struct i915_sw_fence
> *fence,
> return dma_fenc
[Intel-gfx] [RFC i-g-t] igt/media-bench.pl: Media workload analyzer
From: Tvrtko Ursulin
The high level goal of this script is to programatically analyze
the simulated media workloads (gem_wsim) by finding an optimal
load balancing strategy, and also detecting any possible
shortcomings of the same.
When run without command line arguments script will run through
both of its phases.
In the first phase it will be running all the known balancers
against the all the known workloads, and for each combination
look for a point where aggregated system throughput cannot be
increased by running more parallel workload instances.
At that point each balancer gets a score proportional to the
throughput achieved, which is added to the running total for the
complete phase.
Several different score boards are kept - total throughput, per
client throughput and combined (total + per client). Weighted
scoreboards are also kept where scores are weighted based on the
total variance detected for a single workload. This means scores
for workloads which respond well to being balanced will be worth
more than of the ones which do not balance well in neither of
the configurations.
Based on the first phase a "best" balancing strategy will be
selected based on the combined weighted scoreboard.
Second phase will then proceed to profile all the selected
workloads with this balancer and look at potential problems with
GPU engines not being completely saturated.
If none of the active engine is saturated the workload will be
flagged, as it will if the only saturated engine is one of the
ones which can be balanced, but the other one in the same class
is under-utilized.
Flagged workloads then need to be analyzed which can be achieved
by looking at the html of the engine timelines which are
generated during this phase. (These files are all put in the
current working directory.)
It is quite possible that something flagged by the script as
suspect is completely fine and just a consequence of the
workload in question being fundementally unbalanced.
It is possible to skip directly to the second phase of the
evaluation by using the -b command line option. This option must
contain a string exactly as understood by gem_wsim's -b option.
For example '-b "-b rtavg -R"'.
Apart from being run with no arguments, script also supports a
selection of command line switches to enable fine tuning.
For example, also including the complete output from the script
in order to be more illustrative:
-8<---
+ scripts/media-bench.pl -n 642317 -r 2 -B rand,rtavg -W
media_load_balance_hd12.wsim,media_load_balance_fhd26u7.wsim
Workloads:
media_load_balance_hd12.wsim
media_load_balance_fhd26u7.wsim
Balancers: rand,rtavg,
Target workload duration is 2s.
Calibration tolerance is 0.01.
Nop calibration is 642317.
Evaluating 'media_load_balance_hd12.wsim'... 2s is 990 workloads.
(error=0.00756)
Finding saturation points for 'media_load_balance_hd12.wsim'...
rand balancer ('-b rand'): 6 clients (1412.576 wps, 235.4293
wps/client).
rand balancer ('-b rand -R'): 6 clients (1419.639 wps, 236.6065 wps/client).
rtavg balancer ('-b rtavg'): 5 clients (1430.143 wps, 286.0286 wps/client).
rtavg balancer ('-b rtavg -H'): 5 clients (1339.775 wps, 267.955
wps/client).
rtavg balancer ('-b rtavg -R'): 5 clients (1386.384 wps, 277.2768
wps/client).
rtavg balancer ('-b rtavg -R -H'): 6 clients (1365.943 wps,
227.65716667 wps/client).
Best balancer is '-b rtavg'.
Evaluating 'media_load_balance_fhd26u7.wsim'... 2s is 52 workloads.
(error=0.002)
Finding saturation points for 'media_load_balance_fhd26u7.wsim'...
rand balancer ('-b rand'): 3 clients (46.532 wps, 15.51067
wps/client).
rand balancer ('-b rand -R'): 3 clients (46.242 wps, 15.414 wps/client).
rtavg balancer ('-b rtavg'): 6 clients (61.232 wps, 10.20533
wps/client).
rtavg balancer ('-b rtavg -H'): 4 clients (57.608 wps, 14.402 wps/client).
rtavg balancer ('-b rtavg -R'): 6 clients (61.793 wps, 10.29883
wps/client).
rtavg balancer ('-b rtavg -R -H'): 7 clients (60.697 wps, 8.671 wps/client).
Best balancer is '-b rtavg -R'.
Total wps rank:
===
1: '-b rtavg' (1)
2: '-b rtavg -R' (0.989191465637926)
3: '-b rtavg -R -H' (0.973103630772601)
4: '-b rtavg -H' (0.938804458876241)
5: '-b rand -R' (0.874465740398305)
6: '-b rand' (0.874342391093453)
Total weighted wps rank:
1: '-b rtavg -R' (1)
2: '-b rtavg' (0.998877134022041)
3: '-b rtavg -R -H' (0.982849160383224)
4: '-b rtavg -H' (0.938950446314292)
5: '-b rand' (0.80507369080098)
6: '-b rand -R' (0.80229656623594)
Per client wps rank:
1: '-b rtavg -H' (1)
2: '-b rand' (0.977356849770376)
3: '-b rand -R' (0.976222085591368)
4: '-b rtavg' (0.25068013012)
5: '-b rtavg -R' (0.875653417817828)
6: '-b rtavg -R -H' (0.726389466714194)
Per client weighted wps rank:
=
1: '-b rand' (1)
2: '-b rand -R' (0.99
[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/gen9: Reintroduce WaEnableYV12BugFixInHalfSliceChicken7
== Series Details == Series: drm/i915/gen9: Reintroduce WaEnableYV12BugFixInHalfSliceChicken7 URL : https://patchwork.freedesktop.org/series/24351/ State : success == Summary == Series 24351v1 drm/i915/gen9: Reintroduce WaEnableYV12BugFixInHalfSliceChicken7 https://patchwork.freedesktop.org/api/1.0/series/24351/revisions/1/mbox/ Test gem_exec_suspend: Subgroup basic-s4-devices: pass -> DMESG-WARN (fi-kbl-7560u) fdo#100125 fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125 fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time:438s fi-bdw-gvtdvmtotal:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:432s fi-bsw-n3050 total:278 pass:242 dwarn:0 dfail:0 fail:0 skip:36 time:575s fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:516s fi-bxt-t5700 total:278 pass:258 dwarn:0 dfail:0 fail:0 skip:20 time:554s fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time:493s fi-byt-n2820 total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:484s fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:418s fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:409s fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:421s fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:499s fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:503s fi-kbl-7500u total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:463s fi-kbl-7560u total:278 pass:267 dwarn:1 dfail:0 fail:0 skip:10 time:575s fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:464s fi-skl-6700hqtotal:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time:575s fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time:475s fi-skl-6770hqtotal:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:499s fi-skl-gvtdvmtotal:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time:438s fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:534s fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time:409s 51350ea90d1a5c8bb7e6532eb61c64a57e83ab9b drm-tip: 2017y-05m-11d-13h-05m-06s UTC integration manifest c0a9178 drm/i915/gen9: Reintroduce WaEnableYV12BugFixInHalfSliceChicken7 == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4682/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: set initialised only when init_context callback is NULL (rev2)
On Fri, May 12, 2017 at 11:34:21AM -, Patchwork wrote: > == Series Details == > > Series: drm/i915: set initialised only when init_context callback is NULL > (rev2) > URL : https://patchwork.freedesktop.org/series/24286/ > State : success > > == Summary == > > Series 24286v2 drm/i915: set initialised only when init_context callback is > NULL > https://patchwork.freedesktop.org/api/1.0/series/24286/revisions/2/mbox/ > > Test gem_exec_suspend: > Subgroup basic-s4-devices: > pass -> DMESG-WARN (fi-kbl-7560u) fdo#100125 Thanks for the patch and pushed. -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] ✓ Fi.CI.BAT: success for drm/i915: set initialised only when init_context callback is NULL (rev2)
== Series Details == Series: drm/i915: set initialised only when init_context callback is NULL (rev2) URL : https://patchwork.freedesktop.org/series/24286/ State : success == Summary == Series 24286v2 drm/i915: set initialised only when init_context callback is NULL https://patchwork.freedesktop.org/api/1.0/series/24286/revisions/2/mbox/ Test gem_exec_suspend: Subgroup basic-s4-devices: pass -> DMESG-WARN (fi-kbl-7560u) fdo#100125 fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125 fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time:438s fi-bdw-gvtdvmtotal:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:437s fi-bsw-n3050 total:278 pass:242 dwarn:0 dfail:0 fail:0 skip:36 time:576s fi-bxt-t5700 total:278 pass:258 dwarn:0 dfail:0 fail:0 skip:20 time:562s fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time:497s fi-byt-n2820 total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:481s fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:411s fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:415s fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:414s fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:500s fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:478s fi-kbl-7500u total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:468s fi-kbl-7560u total:278 pass:267 dwarn:1 dfail:0 fail:0 skip:10 time:572s fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:459s fi-skl-6700hqtotal:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time:582s fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time:461s fi-skl-6770hqtotal:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:502s fi-skl-gvtdvmtotal:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time:440s fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:536s fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time:419s fi-bxt-j4205 failed to collect. IGT log at Patchwork_4681/fi-bxt-j4205/igt.log 51350ea90d1a5c8bb7e6532eb61c64a57e83ab9b drm-tip: 2017y-05m-11d-13h-05m-06s UTC integration manifest fdaa20d drm/i915: set initialised only when init_context callback is NULL == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4681/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] dma-buf/sync-file: Defer creation of sync_file->name
Constructing the name takes the majority of the time for allocating a
sync_file to wrap a fence, and the name is very rarely used (only via
the sync_file status user interface). To reduce the impact on the common
path (that of creating sync_file to pass around), defer the construction
of the name until it is first used.
Signed-off-by: Chris Wilson
Cc: Sumit Semwal
Cc: Gustavo Padovan
---
drivers/dma-buf/sync_file.c | 18 +++---
include/linux/sync_file.h | 2 +-
2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 2321035f6204..2cccfa834778 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -82,11 +82,6 @@ struct sync_file *sync_file_create(struct dma_fence *fence)
sync_file->fence = dma_fence_get(fence);
- snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
-fence->ops->get_driver_name(fence),
-fence->ops->get_timeline_name(fence), fence->context,
-fence->seqno);
-
return sync_file;
}
EXPORT_SYMBOL(sync_file_create);
@@ -268,7 +263,7 @@ static struct sync_file *sync_file_merge(const char *name,
struct sync_file *a,
goto err;
}
- strlcpy(sync_file->name, name, sizeof(sync_file->name));
+ strlcpy(sync_file->user_name, name, sizeof(sync_file->user_name));
return sync_file;
err:
@@ -422,7 +417,16 @@ static long sync_file_ioctl_fence_info(struct sync_file
*sync_file,
}
no_fences:
- strlcpy(info.name, sync_file->name, sizeof(info.name));
+ if (!sync_file->user_name[0]) {
+ scnprintf(sync_file->user_name,
+ sizeof(sync_file->user_name),
+ "%s-%s%llu-%d",
+
sync_file->fence->ops->get_driver_name(sync_file->fence),
+
sync_file->fence->ops->get_timeline_name(sync_file->fence),
+ sync_file->fence->context,
+ sync_file->fence->seqno);
+ }
+ strlcpy(info.name, sync_file->user_name, sizeof(info.name));
info.status = dma_fence_is_signaled(sync_file->fence);
info.num_fences = num_fences;
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index 3e3ab84fc4cd..0cbeff29f510 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -34,7 +34,7 @@
struct sync_file {
struct file *file;
struct kref kref;
- charname[32];
+ charuser_name[32];
#ifdef CONFIG_DEBUG_FS
struct list_headsync_file_list;
#endif
--
2.11.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915/gen9: Reintroduce WaEnableYV12BugFixInHalfSliceChicken7
This basically reverts commit 465418c6064c
("drm/i915/gen9: Remove WaEnableYV12BugFixInHalfSliceChicken7")
with small addition - marking it as affecting KBL as well.
It was incorrectly considered fixed in production steppings.
References: HSD#2126385, HSD#2131381, HSDES#1504433555, BSID#0764
Cc: Mika Kuoppala
Cc: Jeff McGee
Signed-off-by: Arkadiusz Hiler
---
drivers/gpu/drm/i915/intel_engine_cs.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c
b/drivers/gpu/drm/i915/intel_engine_cs.c
index 483ed76..49c2315 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -851,8 +851,10 @@ static int gen9_init_workarounds(struct intel_engine_cs
*engine)
*/
}
+ /* WaEnableYV12BugFixInHalfSliceChicken7:skl,bxt,kbl,glk */
/* WaEnableSamplerGPGPUPreemptionSupport:skl,bxt,kbl */
WA_SET_BIT_MASKED(GEN9_HALF_SLICE_CHICKEN7,
+ GEN9_ENABLE_YV12_BUGFIX |
GEN9_ENABLE_GPGPU_PREEMPTION);
/* Wa4x4STCOptimizationDisable:skl,bxt,kbl,glk */
--
2.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✓ Fi.CI.BAT: success for Enable guest linux i915 full ppgtt functionality (rev2)
== Series Details == Series: Enable guest linux i915 full ppgtt functionality (rev2) URL : https://patchwork.freedesktop.org/series/24271/ State : success == Summary == Series 24271v2 Enable guest linux i915 full ppgtt functionality https://patchwork.freedesktop.org/api/1.0/series/24271/revisions/2/mbox/ Test gem_exec_suspend: Subgroup basic-s4-devices: pass -> DMESG-WARN (fi-kbl-7560u) fdo#100125 fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125 fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time:437s fi-bdw-gvtdvmtotal:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:434s fi-bsw-n3050 total:278 pass:242 dwarn:0 dfail:0 fail:0 skip:36 time:580s fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:516s fi-bxt-t5700 total:278 pass:258 dwarn:0 dfail:0 fail:0 skip:20 time:561s fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time:502s fi-byt-n2820 total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:488s fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:412s fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:410s fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:423s fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:489s fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:497s fi-kbl-7500u total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:460s fi-kbl-7560u total:278 pass:267 dwarn:1 dfail:0 fail:0 skip:10 time:578s fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:467s fi-skl-6700hqtotal:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time:581s fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time:471s fi-skl-6770hqtotal:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:496s fi-skl-gvtdvmtotal:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time:444s fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:538s fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time:417s 51350ea90d1a5c8bb7e6532eb61c64a57e83ab9b drm-tip: 2017y-05m-11d-13h-05m-06s UTC integration manifest c8b3314 drm/i915: enable guest full ppgtt when device model supports 4b4151c drm/i915/gvt: provide full ppgtt capability for guest 9a9e0ba drm/i915: introduce vgt_caps to pvinfo 40e3bd6 drm/i915/gvt: reorder the shadow ppgtt update process by adding entry first == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4680/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: set initialised only when init_context callback is NULL
Thanks Chris! Have a nice weekend. :) > -Original Message- > From: Chris Wilson [mailto:ch...@chris-wilson.co.uk] > Sent: Friday, May 12, 2017 6:12 PM > To: intel-gfx@lists.freedesktop.org > Cc: Dong, Chuanxiao ; Chris Wilson > > Subject: [PATCH v2] drm/i915: set initialised only when init_context callback > is NULL > > From: Chuanxiao Dong > > During execlist_context_deferred_alloc() we presumed that the context is > uninitialised (we only just allocated the state object for it!) and chose to > optimise away the later call to engine->init_context() if > engine->init_context were NULL. This breaks with GVT's contexts that are > marked as pre-initialised to avoid us annoyingly calling > engine->init_context(). The fix is to not override ce->initialised if it > is already true. > > Cc: Chris Wilson > Signed-off-by: Chuanxiao Dong > Link: http://patchwork.freedesktop.org/patch/msgid/1494497262-24855-1- > git-send-email-chuanxiao.d...@intel.com > Reviewed-by: Chris Wilson > Signed-off-by: Chris Wilson > --- > drivers/gpu/drm/i915/intel_lrc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c > b/drivers/gpu/drm/i915/intel_lrc.c > index 319d9a8f37ca..9a1192d61538 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1956,7 +1956,7 @@ static int execlists_context_deferred_alloc(struct > i915_gem_context *ctx, > > ce->ring = ring; > ce->state = vma; > - ce->initialised = engine->init_context == NULL; > + ce->initialised |= engine->init_context == NULL; > > return 0; > > -- > 2.11.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t] tests/initial_state: Add a test to capture the state of the GPU
When running testlist for CI iot would be good to know if
the state of the GPU is as expected. This adds a subtest to check
if the GPU is wedged.
Signed-off-by: Marta Lofstedt
---
tests/Makefile.sources| 1 +
tests/initial_state.c | 52 +++
tests/intel-ci/extended.testlist | 1 +
tests/intel-ci/fast-feedback.testlist | 1 +
4 files changed, 55 insertions(+)
create mode 100644 tests/initial_state.c
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 9553e4d9..32431a05 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -152,6 +152,7 @@ TESTS_progs_M = \
vgem_basic \
vgem_slow \
meta_test \
+ initial_state \
$(NULL)
if HAVE_CHAMELIUM
diff --git a/tests/initial_state.c b/tests/initial_state.c
new file mode 100644
index ..1c5d9a74
--- /dev/null
+++ b/tests/initial_state.c
@@ -0,0 +1,52 @@
+/*
+ * Copyright © 2017 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include
+#include
+#include "igt.h"
+
+/*
+ * The purpose of this test is to capture the
+ * state of the GPU before running a testlist.
+ *
+ * Note, this test currently only checks if
+ * i915 is wedged. But, it could be extended to report
+ * on other bad initial states.
+ */
+
+static void is_wedged(void)
+{
+ char buf[128];
+ int fd = drm_open_driver(DRIVER_INTEL);
+
+ igt_debugfs_read(fd, "i915_wedged", buf);
+ igt_assert(!strstr(buf, "1"));
+}
+
+igt_main
+{
+
+ igt_subtest("is_wedged")
+ is_wedged();
+}
diff --git a/tests/intel-ci/extended.testlist b/tests/intel-ci/extended.testlist
index a16c9c84..2ec08b55 100644
--- a/tests/intel-ci/extended.testlist
+++ b/tests/intel-ci/extended.testlist
@@ -1,3 +1,4 @@
+igt@initial_state@is_wedged
igt@core_auth@many-magics
igt@core_getclient
igt@core_get_client_auth@master-drop
diff --git a/tests/intel-ci/fast-feedback.testlist
b/tests/intel-ci/fast-feedback.testlist
index 5ffa2cef..a6a0a810 100644
--- a/tests/intel-ci/fast-feedback.testlist
+++ b/tests/intel-ci/fast-feedback.testlist
@@ -1,3 +1,4 @@
+igt@initial_state@is_wedged
igt@core_auth@basic-auth
igt@core_prop_blob@basic
igt@drv_getparams_basic@basic-eu-total
--
2.11.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ Fi.CI.BAT: warning for drm/i915: don't do allocate_va_range again on pin update
== Series Details == Series: drm/i915: don't do allocate_va_range again on pin update URL : https://patchwork.freedesktop.org/series/24342/ State : warning == Summary == Series 24342v1 drm/i915: don't do allocate_va_range again on pin update https://patchwork.freedesktop.org/api/1.0/series/24342/revisions/1/mbox/ Test gem_exec_suspend: Subgroup basic-s4-devices: pass -> DMESG-WARN (fi-kbl-7560u) fdo#100125 Test kms_force_connector_basic: Subgroup prune-stale-modes: pass -> SKIP (fi-snb-2520m) fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125 fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time:442s fi-bdw-gvtdvmtotal:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:442s fi-bsw-n3050 total:278 pass:242 dwarn:0 dfail:0 fail:0 skip:36 time:581s fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:515s fi-bxt-t5700 total:278 pass:258 dwarn:0 dfail:0 fail:0 skip:20 time:561s fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time:492s fi-byt-n2820 total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:492s fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:412s fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:412s fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:425s fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:489s fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:474s fi-kbl-7500u total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:468s fi-kbl-7560u total:278 pass:267 dwarn:1 dfail:0 fail:0 skip:10 time:575s fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:461s fi-skl-6700hqtotal:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time:587s fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time:467s fi-skl-6770hqtotal:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:497s fi-skl-gvtdvmtotal:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time:438s fi-snb-2520m total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time:541s fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time:414s 51350ea90d1a5c8bb7e6532eb61c64a57e83ab9b drm-tip: 2017y-05m-11d-13h-05m-06s UTC integration manifest 5cfd581 drm/i915: don't do allocate_va_range again on pin update == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4679/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: don't do allocate_va_range again on pin update
On 12 May 2017 at 10:31, Chris Wilson wrote:
> On Fri, May 12, 2017 at 10:14:23AM +0100, Chris Wilson wrote:
>> From: Matthew Auld
>>
>> If a vma is already bound to a ppgtt, we incorrectly call
>> allocate_va_range again when doing a PIN_UPDATE, which will result in
>> over accounting within our paging structures, such that when we do
>> unbind something we don't actually destroy the structures and end up
>> inadvertently recycling them. In reality this probably isn't too bad,
>> but once we start touching PDEs and PDPEs for 64K/2M/1G pages this
>> apparent recycling will manifest into lots of really, really subtle
>> bugs.
>>
>> v2: Fix the testing of vma->flags for aliasing_ppgtt_bind_vma
>>
>> Fixes: ff685975d97f ("drm/i915: Move allocate_va_range to GTT")
>> Signed-off-by: Matthew Auld
>> Cc: Chris Wilson
>> Cc: Joonas Lahtinen
>> Reviewed-by: Chris Wilson
>
> So, we are missing coverage of PIN_UPDATE and set-cache-level from the
> kselftests. Ideas?
>
> Matthew, do you have any clue how to reproduce those subtle errors?
The errors I encountered were only visible with huge-pages, for
example hitting the 4K PTE insertion path while the PDE still points
to a stale 2M page and not a page-table.
I wanted to add a GEM_BUG_ON(pt->used_ptes > GEN8_PTES) as part of
this patch but the appgtt would make that a pain iiuc. Although I
don't really understand why we bother with
allocate_va_range/clear_range for the appgtt bind/unbind given that we
preallocate it anyway...
> -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 mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: set initialised only when init_context callback is NULL
> -Original Message- > From: Chris Wilson [mailto:ch...@chris-wilson.co.uk] > Sent: Friday, May 12, 2017 6:11 PM > To: Dong, Chuanxiao ; intel- > g...@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915: set initialised only when > init_context callback is NULL > > On Fri, May 12, 2017 at 10:49:24AM +0100, Chris Wilson wrote: > > On Thu, May 11, 2017 at 06:07:42PM +0800, Chuanxiao Dong wrote: > > > initialised is fixup by the GVT shadow context as true to avoid the > > > init from the host because it cannot take the settings from the > > > host. Add a check to let host driver only overwrite it when the init > > > callback is NULL > > > > During execlist_context_deferred_alloc() we presumed that the context > > is uninitialised (we only just allocated the state object for it!) and > > chose to optimise away the later call to engine->init_context() if > > engine->init_context were NULL. This breaks with GVT's contexts that > > engine->are > > marked as pre-initialised to avoid us annoyingly calling > > engine->init_context(). The fix is to not override ce->initialised if > > engine->it > > is already true. Thanks Chris, will use this as commit message > > > > > Cc: Chris Wilson > > > Signed-off-by: Chuanxiao Dong > > > --- > > > drivers/gpu/drm/i915/intel_lrc.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c > > > b/drivers/gpu/drm/i915/intel_lrc.c > > > index 319d9a8..d0e9b61 100644 > > > --- a/drivers/gpu/drm/i915/intel_lrc.c > > > +++ b/drivers/gpu/drm/i915/intel_lrc.c > > > @@ -1956,7 +1956,8 @@ static int > > > execlists_context_deferred_alloc(struct i915_gem_context *ctx, > > > > > > ce->ring = ring; > > > ce->state = vma; > > > - ce->initialised = engine->init_context == NULL; > > > + if (!engine->init_context) > > > + ce->initialised = true; > > > > Does the compiler generate a cmov? Just get the assembly code of this change, seems no cmov. if (!engine->init_context) 8156b6f3: 49 83 bf c0 01 00 00cmpq $0x0,0x1c0(%r15) 8156b6fa: 00 8156b6fb: 0f 84 e5 01 00 00 je 8156b8e6 . ce->initialised = true; 8156b8e6: 4d 6b ed 28 imul $0x28,%r13,%r13 8156b8ea: 43 c6 44 2e 7c 01 movb $0x1,0x7c(%r14,%r13,1) 8156b8f0: e9 0c fe ff ff jmpq 8156b701 > > Test and jump, may as well just use ce->initialised |= init_context == NULL; - > Chris This is the new assembly code for this new change: ce->initialised |= engine->init_context == NULL; 8156b6f3: 49 83 bf c0 01 00 00cmpq $0x0,0x1c0(%r15) 8156b6fa: 00 8156b6fb: 0f 94 c2sete %dl 8156b6fe: 08 50 7cor %dl,0x7c(%rax) looks your change is better because the compiler doesn't generate a jump. :) Will send out v2 with your idea. Thanks Chuanxiao ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: set initialised only when init_context callback is NULL
On Fri, May 12, 2017 at 10:49:24AM +0100, Chris Wilson wrote: > On Thu, May 11, 2017 at 06:07:42PM +0800, Chuanxiao Dong wrote: > > initialised is fixup by the GVT shadow context as true to avoid the init > > from the host because it cannot take the settings from the host. Add a > > check to let host driver only overwrite it when the init callback is NULL > > During execlist_context_deferred_alloc() we presumed that the context is > uninitialised (we only just allocated the state object for it!) and > chose to optimise away the later call to engine->init_context() if > engine->init_context were NULL. This breaks with GVT's contexts that are > marked as pre-initialised to avoid us annoyingly calling > engine->init_context(). The fix is to not override ce->initialised if it > is already true. > > > Cc: Chris Wilson > > Signed-off-by: Chuanxiao Dong > > --- > > drivers/gpu/drm/i915/intel_lrc.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c > > b/drivers/gpu/drm/i915/intel_lrc.c > > index 319d9a8..d0e9b61 100644 > > --- a/drivers/gpu/drm/i915/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/intel_lrc.c > > @@ -1956,7 +1956,8 @@ static int execlists_context_deferred_alloc(struct > > i915_gem_context *ctx, > > > > ce->ring = ring; > > ce->state = vma; > > - ce->initialised = engine->init_context == NULL; > > + if (!engine->init_context) > > + ce->initialised = true; > > Does the compiler generate a cmov? Test and jump, may as well just use ce->initialised |= init_context == NULL; -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 v2] drm/i915: set initialised only when init_context callback is NULL
From: Chuanxiao Dong During execlist_context_deferred_alloc() we presumed that the context is uninitialised (we only just allocated the state object for it!) and chose to optimise away the later call to engine->init_context() if engine->init_context were NULL. This breaks with GVT's contexts that are marked as pre-initialised to avoid us annoyingly calling engine->init_context(). The fix is to not override ce->initialised if it is already true. Cc: Chris Wilson Signed-off-by: Chuanxiao Dong Link: http://patchwork.freedesktop.org/patch/msgid/1494497262-24855-1-git-send-email-chuanxiao.d...@intel.com Reviewed-by: Chris Wilson Signed-off-by: Chris Wilson --- drivers/gpu/drm/i915/intel_lrc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 319d9a8f37ca..9a1192d61538 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1956,7 +1956,7 @@ static int execlists_context_deferred_alloc(struct i915_gem_context *ctx, ce->ring = ring; ce->state = vma; - ce->initialised = engine->init_context == NULL; + ce->initialised |= engine->init_context == NULL; return 0; -- 2.11.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: set initialised only when init_context callback is NULL
On Thu, May 11, 2017 at 06:07:42PM +0800, Chuanxiao Dong wrote: > initialised is fixup by the GVT shadow context as true to avoid the init > from the host because it cannot take the settings from the host. Add a > check to let host driver only overwrite it when the init callback is NULL During execlist_context_deferred_alloc() we presumed that the context is uninitialised (we only just allocated the state object for it!) and chose to optimise away the later call to engine->init_context() if engine->init_context were NULL. This breaks with GVT's contexts that are marked as pre-initialised to avoid us annoyingly calling engine->init_context(). The fix is to not override ce->initialised if it is already true. > Cc: Chris Wilson > Signed-off-by: Chuanxiao Dong > --- > drivers/gpu/drm/i915/intel_lrc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c > b/drivers/gpu/drm/i915/intel_lrc.c > index 319d9a8..d0e9b61 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1956,7 +1956,8 @@ static int execlists_context_deferred_alloc(struct > i915_gem_context *ctx, > > ce->ring = ring; > ce->state = vma; > - ce->initialised = engine->init_context == NULL; > + if (!engine->init_context) > + ce->initialised = true; Does the compiler generate a cmov? -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 v2 0/4] Enable guest linux i915 full ppgtt functionality
This patchset fixs guest i915 full ppgtt block issue in device model and enables the full ppgtt functionality to guest i915 driver when device model with the fixed patch can support this capability. Changes since last version: - Refine code with coding style. - Rewrite the guest vgpu full ppgtt checking logic. Tina Zhang (4): drm/i915/gvt: reorder the shadow ppgtt update process by adding entry first drm/i915: introduce vgt_caps to pvinfo, v2 drm/i915/gvt: provide full ppgtt capability for guest, v2 drm/i915: enable guest full ppgtt when device model supports, v2 drivers/gpu/drm/i915/gvt/gtt.c | 45 +++-- drivers/gpu/drm/i915/gvt/vgpu.c | 1 + drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem_gtt.c | 11 + drivers/gpu/drm/i915/i915_pvinfo.h | 8 ++- drivers/gpu/drm/i915/i915_vgpu.c| 7 ++ drivers/gpu/drm/i915/i915_vgpu.h| 3 +++ 7 files changed, 52 insertions(+), 24 deletions(-) -- 2.7.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 2/4] drm/i915: introduce vgt_caps to pvinfo
vgt_caps is used for guest i915 driver to get the vgpu capabilities from
the device model. VGT_CPAS_FULL_PPGTT is one of the capabilities type to
let guest i915 dirver know that the guest i915 full ppgtt is supported by
device model.
Changes since v1:
- Use u32 instead of uint32_t (Joonas)
- Move VGT_CAPS_FULL_PPGTT introduction to this patch and use #define instead
of enum (Joonas)
Signed-off-by: Tina Zhang
---
drivers/gpu/drm/i915/i915_pvinfo.h | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_pvinfo.h
b/drivers/gpu/drm/i915/i915_pvinfo.h
index c0cb297..8dc0664 100644
--- a/drivers/gpu/drm/i915/i915_pvinfo.h
+++ b/drivers/gpu/drm/i915/i915_pvinfo.h
@@ -53,12 +53,18 @@ enum vgt_g2v_type {
VGT_G2V_MAX,
};
+/*
+ * VGT capabilities type
+ */
+#define VGT_CAPS_FULL_PPGTTBIT(2)
+
struct vgt_if {
u64 magic; /* VGT_MAGIC */
uint16_t version_major;
uint16_t version_minor;
u32 vgt_id; /* ID of vGT instance */
- u32 rsv1[12]; /* pad to offset 0x40 */
+ u32 vgt_caps; /* VGT capabilities */
+ u32 rsv1[11]; /* pad to offset 0x40 */
/*
* Data structure to describe the balooning info of resources.
* Each VM can only have one portion of continuous area for now.
--
2.7.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 1/4] drm/i915/gvt: reorder the shadow ppgtt update process by adding entry first
Guest i915 driver may update the ppgtt table just before this workload
is going to be submitted to the hardware by device model. This case
wasn't handled well by device model before, due to the small time
window between removing old ppgtt entry and adding the new one. Errors
occur when the workload is executed by hardware during that small time
window. This patch is to remove this time window by adding the new ppgtt
entry first and then remove the old one.
Signed-off-by: Tina Zhang
---
drivers/gpu/drm/i915/gvt/gtt.c | 45 ++
1 file changed, 28 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
index c6f0077..8caad86 100644
--- a/drivers/gpu/drm/i915/gvt/gtt.c
+++ b/drivers/gpu/drm/i915/gvt/gtt.c
@@ -975,29 +975,26 @@ static int ppgtt_populate_shadow_page(struct
intel_vgpu_ppgtt_spt *spt)
}
static int ppgtt_handle_guest_entry_removal(struct intel_vgpu_guest_page *gpt,
- unsigned long index)
+ struct intel_gvt_gtt_entry *se, unsigned long index)
{
struct intel_vgpu_ppgtt_spt *spt = guest_page_to_ppgtt_spt(gpt);
struct intel_vgpu_shadow_page *sp = &spt->shadow_page;
struct intel_vgpu *vgpu = spt->vgpu;
struct intel_gvt_gtt_pte_ops *ops = vgpu->gvt->gtt.pte_ops;
- struct intel_gvt_gtt_entry e;
int ret;
- ppgtt_get_shadow_entry(spt, &e, index);
-
- trace_gpt_change(spt->vgpu->id, "remove", spt, sp->type, e.val64,
+ trace_gpt_change(spt->vgpu->id, "remove", spt, sp->type, se->val64,
index);
- if (!ops->test_present(&e))
+ if (!ops->test_present(se))
return 0;
- if (ops->get_pfn(&e) == vgpu->gtt.scratch_pt[sp->type].page_mfn)
+ if (ops->get_pfn(se) == vgpu->gtt.scratch_pt[sp->type].page_mfn)
return 0;
- if (gtt_type_is_pt(get_next_pt_type(e.type))) {
+ if (gtt_type_is_pt(get_next_pt_type(se->type))) {
struct intel_vgpu_ppgtt_spt *s =
- ppgtt_find_shadow_page(vgpu, ops->get_pfn(&e));
+ ppgtt_find_shadow_page(vgpu, ops->get_pfn(se));
if (!s) {
gvt_vgpu_err("fail to find guest page\n");
ret = -ENXIO;
@@ -1007,12 +1004,10 @@ static int ppgtt_handle_guest_entry_removal(struct
intel_vgpu_guest_page *gpt,
if (ret)
goto fail;
}
- ops->set_pfn(&e, vgpu->gtt.scratch_pt[sp->type].page_mfn);
- ppgtt_set_shadow_entry(spt, &e, index);
return 0;
fail:
gvt_vgpu_err("fail: shadow page %p guest entry 0x%llx type %d\n",
- spt, e.val64, e.type);
+ spt, se->val64, se->type);
return ret;
}
@@ -1232,22 +1227,37 @@ static int ppgtt_handle_guest_write_page_table(
{
struct intel_vgpu_ppgtt_spt *spt = guest_page_to_ppgtt_spt(gpt);
struct intel_vgpu *vgpu = spt->vgpu;
+ int type = spt->shadow_page.type;
struct intel_gvt_gtt_pte_ops *ops = vgpu->gvt->gtt.pte_ops;
+ struct intel_gvt_gtt_entry se;
int ret;
int new_present;
new_present = ops->test_present(we);
- ret = ppgtt_handle_guest_entry_removal(gpt, index);
- if (ret)
- goto fail;
+ /*
+* Adding the new entry first and then removing the old one, that can
+* guarantee the ppgtt table is validated during the window between
+* adding and removal.
+*/
+ ppgtt_get_shadow_entry(spt, &se, index);
if (new_present) {
ret = ppgtt_handle_guest_entry_add(gpt, we, index);
if (ret)
goto fail;
}
+
+ ret = ppgtt_handle_guest_entry_removal(gpt, &se, index);
+ if (ret)
+ goto fail;
+
+ if (!new_present) {
+ ops->set_pfn(&se, vgpu->gtt.scratch_pt[type].page_mfn);
+ ppgtt_set_shadow_entry(spt, &se, index);
+ }
+
return 0;
fail:
gvt_vgpu_err("fail: shadow page %p guest entry 0x%llx type %d.\n",
@@ -1319,7 +1329,7 @@ static int ppgtt_handle_guest_write_page_table_bytes(void
*gp,
struct intel_vgpu *vgpu = spt->vgpu;
struct intel_gvt_gtt_pte_ops *ops = vgpu->gvt->gtt.pte_ops;
const struct intel_gvt_device_info *info = &vgpu->gvt->device_info;
- struct intel_gvt_gtt_entry we;
+ struct intel_gvt_gtt_entry we, se;
unsigned long index;
int ret;
@@ -1335,7 +1345,8 @@ static int ppgtt_handle_guest_write_page_table_bytes(void
*gp,
return ret;
} else {
if (!test_bit(index, spt->post_shadow_bitmap)) {
- ret = ppgtt_handle_guest_entry_removal(gpt, index);
+ ppgtt_get_shadow_entry(spt, &se, index);
+ ret = ppgtt_handle_gu
[Intel-gfx] [PATCH v2 4/4] drm/i915: enable guest full ppgtt when device model supports
Add full ppgtt capability check in guest i915 driver and enable the full
ppgtt in guest only when device mode supports.
Changes since v1:
- Use u32 instead of uint32_t. (Joonas)
- Rewrite the vgpu full ppgtt capability checking logic. (Joonas)
- Some coding style refine. (Joonas)
Signed-off-by: Tina Zhang
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_gem_gtt.c | 11 +--
drivers/gpu/drm/i915/i915_vgpu.c| 7 +++
drivers/gpu/drm/i915/i915_vgpu.h| 3 +++
4 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 357b6c6..2e34927 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1909,6 +1909,7 @@ struct i915_workarounds {
struct i915_virtual_gpu {
bool active;
+ u32 caps;
};
/* used in computing the new watermarks state */
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 8bab4ae..3f110aa 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -139,13 +139,12 @@ int intel_sanitize_enable_ppgtt(struct drm_i915_private
*dev_priv,
has_aliasing_ppgtt = dev_priv->info.has_aliasing_ppgtt;
has_full_ppgtt = dev_priv->info.has_full_ppgtt;
- has_full_48bit_ppgtt = dev_priv->info.has_full_48bit_ppgtt;
- if (intel_vgpu_active(dev_priv)) {
- /* emulation is too hard */
- has_full_ppgtt = false;
- has_full_48bit_ppgtt = false;
- }
+ if (intel_vgpu_active(dev_priv))
+ has_full_ppgtt = intel_vgpu_has_full_ppgtt(dev_priv);
+
+ has_full_48bit_ppgtt = has_full_ppgtt &&
+ dev_priv->info.has_full_48bit_ppgtt;
if (!has_aliasing_ppgtt)
return 0;
diff --git a/drivers/gpu/drm/i915/i915_vgpu.c b/drivers/gpu/drm/i915/i915_vgpu.c
index 4ab8a97..adc4eda 100644
--- a/drivers/gpu/drm/i915/i915_vgpu.c
+++ b/drivers/gpu/drm/i915/i915_vgpu.c
@@ -77,10 +77,17 @@ void i915_check_vgpu(struct drm_i915_private *dev_priv)
return;
}
+ dev_priv->vgpu.caps = __raw_i915_read32(dev_priv, vgtif_reg(vgt_caps));
+
dev_priv->vgpu.active = true;
DRM_INFO("Virtual GPU for Intel GVT-g detected.\n");
}
+bool intel_vgpu_has_full_ppgtt(struct drm_i915_private *dev_priv)
+{
+ return dev_priv->vgpu.caps & VGT_CAPS_FULL_PPGTT;
+}
+
struct _balloon_info_ {
/*
* There are up to 2 regions per mappable/unmappable graphic
diff --git a/drivers/gpu/drm/i915/i915_vgpu.h b/drivers/gpu/drm/i915/i915_vgpu.h
index 3c3b2d2..b4e04eb 100644
--- a/drivers/gpu/drm/i915/i915_vgpu.h
+++ b/drivers/gpu/drm/i915/i915_vgpu.h
@@ -27,6 +27,9 @@
#include "i915_pvinfo.h"
void i915_check_vgpu(struct drm_i915_private *dev_priv);
+
+bool intel_vgpu_has_full_ppgtt(struct drm_i915_private *dev_priv);
+
int intel_vgt_balloon(struct drm_i915_private *dev_priv);
void intel_vgt_deballoon(struct drm_i915_private *dev_priv);
--
2.7.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 3/4] drm/i915/gvt: provide full ppgtt capability for guest
This patch is to provide full ppgtt capability for guest i915 driver. Changes since v1: - Move VGT_CAPS_FULL_PPGTT introduction to patch 2/4. (Joonas) Signed-off-by: Tina Zhang --- drivers/gpu/drm/i915/gvt/vgpu.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c index 6e3cbd8..23578c1 100644 --- a/drivers/gpu/drm/i915/gvt/vgpu.c +++ b/drivers/gpu/drm/i915/gvt/vgpu.c @@ -43,6 +43,7 @@ void populate_pvinfo_page(struct intel_vgpu *vgpu) vgpu_vreg(vgpu, vgtif_reg(version_minor)) = 0; vgpu_vreg(vgpu, vgtif_reg(display_ready)) = 0; vgpu_vreg(vgpu, vgtif_reg(vgt_id)) = vgpu->id; + vgpu_vreg(vgpu, vgtif_reg(vgt_caps)) = VGT_CAPS_FULL_PPGTT; vgpu_vreg(vgpu, vgtif_reg(avail_rs.mappable_gmadr.base)) = vgpu_aperture_gmadr_base(vgpu); vgpu_vreg(vgpu, vgtif_reg(avail_rs.mappable_gmadr.size)) = -- 2.7.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: don't do allocate_va_range again on pin update
On Fri, May 12, 2017 at 10:14:23AM +0100, Chris Wilson wrote:
> From: Matthew Auld
>
> If a vma is already bound to a ppgtt, we incorrectly call
> allocate_va_range again when doing a PIN_UPDATE, which will result in
> over accounting within our paging structures, such that when we do
> unbind something we don't actually destroy the structures and end up
> inadvertently recycling them. In reality this probably isn't too bad,
> but once we start touching PDEs and PDPEs for 64K/2M/1G pages this
> apparent recycling will manifest into lots of really, really subtle
> bugs.
>
> v2: Fix the testing of vma->flags for aliasing_ppgtt_bind_vma
>
> Fixes: ff685975d97f ("drm/i915: Move allocate_va_range to GTT")
> Signed-off-by: Matthew Auld
> Cc: Chris Wilson
> Cc: Joonas Lahtinen
> Reviewed-by: Chris Wilson
So, we are missing coverage of PIN_UPDATE and set-cache-level from the
kselftests. Ideas?
Matthew, do you have any clue how to reproduce those subtle errors?
-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: don't do allocate_va_range again on pin update
From: Matthew Auld
If a vma is already bound to a ppgtt, we incorrectly call
allocate_va_range again when doing a PIN_UPDATE, which will result in
over accounting within our paging structures, such that when we do
unbind something we don't actually destroy the structures and end up
inadvertently recycling them. In reality this probably isn't too bad,
but once we start touching PDEs and PDPEs for 64K/2M/1G pages this
apparent recycling will manifest into lots of really, really subtle
bugs.
v2: Fix the testing of vma->flags for aliasing_ppgtt_bind_vma
Fixes: ff685975d97f ("drm/i915: Move allocate_va_range to GTT")
Signed-off-by: Matthew Auld
Cc: Chris Wilson
Cc: Joonas Lahtinen
Reviewed-by: Chris Wilson
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 62871cd50605..6354fe78238f 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -193,9 +193,12 @@ static int ppgtt_bind_vma(struct i915_vma *vma,
u32 pte_flags;
int ret;
- ret = vma->vm->allocate_va_range(vma->vm, vma->node.start, vma->size);
- if (ret)
- return ret;
+ if (!(vma->flags & I915_VMA_LOCAL_BIND)) {
+ ret = vma->vm->allocate_va_range(vma->vm, vma->node.start,
+vma->size);
+ if (ret)
+ return ret;
+ }
vma->pages = vma->obj->mm.pages;
@@ -2304,7 +2307,8 @@ static int aliasing_gtt_bind_vma(struct i915_vma *vma,
if (flags & I915_VMA_LOCAL_BIND) {
struct i915_hw_ppgtt *appgtt = i915->mm.aliasing_ppgtt;
- if (appgtt->base.allocate_va_range) {
+ if (!(vma->flags & I915_VMA_LOCAL_BIND) &&
+ appgtt->base.allocate_va_range) {
ret = appgtt->base.allocate_va_range(&appgtt->base,
vma->node.start,
vma->node.size);
--
2.11.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf
Hi, > If the contents of the framebuffer change or if the parameters of the > framebuffer change? I can't image that creating a new dmabuf fd for > every visual change within the framebuffer would be efficient, but I > don't have any concept of what a dmabuf actually does. Ok, some background: The drm subsystem has the concept of planes. The most important plane is the primary framebuffer (i.e. what gets scanned out to the physical display). The cursor is a plane too, and there can be additional overlay planes for stuff like video playback. Typically there are multiple planes in a system and only one of them gets scanned out to the crtc, i.e. the fbdev emulation creates one plane for the framebuffer console. The X-Server creates a plane too, and when you switch between X-Server and framebuffer console via ctrl-alt-fn the intel driver just reprograms the encoder to scan out the one or the other plane to the crtc. The dma-buf handed out by gvt is a reference to a plane. I think on the host side gvt can see only the active plane (from encoder/crtc register programming) not the inactive ones. The dma-buf can be imported as opengl texture and then be used to render the guest display to a host window. I think it is even possible to use the dma-buf as plane in the host drm driver and scan it out directly to a physical display. The actual framebuffer content stays in gpu memory all the time, the cpu never has to touch it. It is possible to cache the dma-buf handles, i.e. when the guest boots you'll get the first for the fbcon plane, when the x-server starts the second for the x-server framebuffer, and when the user switches to the text console via ctrl-alt-fn you can re-use the fbcon dma-buf you already have. The caching becomes more important for good performance when the guest uses pageflipping (wayland does): define two planes, render into one while displaying the other, then flip the two for a atomic display update. The caching also makes it a bit difficult to create a good interface. So, the current patch set creates: (a) A way to query the active planes (ioctl INTEL_VGPU_QUERY_DMABUF added by patch 5/6 of this series). (b) A way to create a dma-buf for the active plane (ioctl INTEL_VGPU_GENERATE_DMABUF). Typical userspace workflow is to first query the plane, then check if it already has a dma-buf for it, and if not create one. > What changes to the framebuffer require a new dmabuf fd? Shouldn't the > user query the parameters of the framebuffer through a dmabuf fd and > shouldn't the dmabuf fd have some signaling mechanism to the user > (eventfd perhaps) to notify the user to re-evaluate the parameters? dma-bufs don't support that, they are really just a handle to a piece of memory, all metadata (format, size) most be communicated by other means. > Otherwise are you imagining that the user polls the vfio region? Hmm, notification support would probably a good reason to have a separate file handle to manage the dma-bufs (instead of using driver-specific ioctls on the vfio fd), because the driver could also use the management fd for notifications then. I'm not sure how useful notification support actually is though. Notifications when another plane gets mapped to the crtc should be easy. But I'm not sure it is possible to get notifications when the plane content changes, especially in case the guest does software rendering so the display is updated without gvt seeing guest activity on the rendering pipeline. Without the later qemu needs a timer for display updates _anyway_ ... cheers, Gerd ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/11] drm/i915: Add more wrapper for fixed_point_16_16 operations
Hi,
On Friday 12 May 2017 05:52 AM, Matt Roper wrote:
On Mon, May 08, 2017 at 05:18:53PM +0530, Mahesh Kumar wrote:
This patch adds few wrapper to perform fixed_point_16_16 operations
mul_u32_fixed_16_16_round_up : Multiplies u32 and fixed_16_16_t
variables & returns u32 result with
rounding-off.
mul_fixed_16_16 : Multiplies two fixed_16_16_t variable & returns
fixed_16_16
div_round_up_fixed_16_16 : Perform division operation on fixed_16_16_t
variables & return u32 result with round-off
fixed_16_16_div_round_up_u64 : devide uint32_t variable by fixed_16_16
variable and round_up the result to uint32_t.
Minor nitpick, but I'd say "rounding-up" rather than "rounding-off" in
your descriptions here.
Will update.
These wrappers will be used by later patches in the series.
The name on this last one (fixed_16_16_div_round_up_u64) doesn't seem to
match what it does. You're using u64 internally, but the actual
operands are a u32 and a fixed point value. Wouldn't it make more sense
to call it div_round_up_u32_fixed_16_16 (that also keeps the dividend
first and the divisor second, which seems more natural to me).
Actually the naming of all the fixed point math functions is a bit
confusing/inconsistent. Some of them are "op_type[_type][_round_up],"
some of them are "op[_round_up]_type," some of them are
"type_op[_round_up]," and some of them are "type_op[_round_up]_type."
Also, none of them specify the return value type, but I guess that's not
too much of a problem since the use of a fixed point structure will
ensure the compiler warns if someone tries to use them assuming the
wrong kind of result.
Maybe we could standardize the names a bit to help avoid confusion? I'd
suggest "op[_round_up]_type1_type2." If both types are the same, we'd
still repeat the type for clarity, but maybe we could just use "fixed16"
or "fix16" to keep the overall names from getting too long. And for
division we'd always keep the dividend as the first type, divisor as
second.
E.g.,
mul_round_up_u32_fixed16()
div_round_up_u32_fixed16()
mul_fixed16_fixed16()
div_round_up_fixed16_fixed16()
will use name "mul_fixed16 / div_round_up_fixed16" (assuming same type
for both the operand if only one type is there in function name), hope
that is fine with you.
And presumably the existing fixed point operations could be renamed in
the same manner.
thanks for suggestion/review, It sounds logical & more consistent, will
fix it,
In this patch will update only these 4 wrappers, will create a new patch
at end of the series to address naming for other wrappers.
thanks,
-Mahesh
Matt
Signed-off-by: Mahesh Kumar
---
drivers/gpu/drm/i915/i915_drv.h | 43 +
1 file changed, 43 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5804734d30a3..5ff0091707c0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -152,6 +152,38 @@ static inline uint_fixed_16_16_t
max_fixed_16_16(uint_fixed_16_16_t max1,
return max;
}
+static inline uint32_t div_round_up_fixed_16_16(uint_fixed_16_16_t val,
+ uint_fixed_16_16_t d)
+{
+ return DIV_ROUND_UP(val.val, d.val);
+}
+
+static inline uint32_t mul_u32_fixed_16_16_round_up(uint32_t val,
+ uint_fixed_16_16_t mul)
+{
+ uint64_t intermediate_val;
+ uint32_t result;
+
+ intermediate_val = (uint64_t) val * mul.val;
+ intermediate_val = DIV_ROUND_UP_ULL(intermediate_val, 1 << 16);
+ WARN_ON(intermediate_val >> 32);
+ result = clamp_t(uint32_t, intermediate_val, 0, ~0);
+ return result;
+}
+
+static inline uint_fixed_16_16_t mul_fixed_16_16(uint_fixed_16_16_t val,
+uint_fixed_16_16_t mul)
+{
+ uint64_t intermediate_val;
+ uint_fixed_16_16_t fp;
+
+ intermediate_val = (uint64_t) val.val * mul.val;
+ intermediate_val = intermediate_val >> 16;
+ WARN_ON(intermediate_val >> 32);
+ fp.val = clamp_t(uint32_t, intermediate_val, 0, ~0);
+ return fp;
+}
+
static inline uint_fixed_16_16_t fixed_16_16_div(uint32_t val, uint32_t d)
{
uint_fixed_16_16_t fp, res;
@@ -174,6 +206,17 @@ static inline uint_fixed_16_16_t
fixed_16_16_div_u64(uint32_t val, uint32_t d)
return res;
}
+static inline uint32_t fixed_16_16_div_round_up_u64(uint32_t val,
+ uint_fixed_16_16_t d)
+{
+ uint64_t interm_val;
+
+ interm_val = (uint64_t)val << 16;
+ interm_val = DIV_ROUND_UP_ULL(interm_val, d.val);
+ WARN_ON(interm_val >> 32);
+ return clamp_t(uint32_t, interm_val, 0, ~0);
+}
+
static inlin
Re: [Intel-gfx] [PATCH 00/11] Implement DDB algorithm and WM cleanup
Hi Matt, Thanks for review, On Friday 12 May 2017 05:51 AM, Matt Roper wrote: On Mon, May 08, 2017 at 05:18:51PM +0530, Mahesh Kumar wrote: This series implements new DDB allocation algorithm to solve the cases, where we have sufficient DDB available to enable multiple planes, But due to the current algorithm not dividing it properly among planes, we end-up failing the flip. It also takes care of enabling same watermark level for each plane, for efficient power saving. Series also fixes/cleans-up few bug in present code. There are two steps in current WM programming. 1. Calculate minimum number of blocks required for a WM level to be enabled. For 1440x2560 panel we need 41 blocks as minimum number of blocks to enable WM0. This is the step which doesn't use vertical size. It only depends on Pipe drain rate and plane horizontal size as per the current Bspec algorithm. So all the plane below have minimum number of blocks required to enable WM0 as 41 Plane 1 - 1440x2560-Min blocks to enable WM0 = 41 Plane 2 - 1440x2560-Min blocks to enable WM0 = 41 Plane 3 - 1440x48 -Min blocks to enable WM0 = 41 Plane 4 - 1440x96 -Min blocks to enable WM0 = 41 2. Number of blocks allotted by the driver Driver allocates 12 for Plane 3 & 16 for plane 4 Total Dbuf Available = 508 Dbuf Available after 32 blocks for cursor = 508 - (32) = 476 Given the dbuf size of 508, I assume this example is for Broxton hardware, right? In that case, you wouldn't actually be able to use the cursor plane since Plane 4 (1440x96) is mutually exclusive with the cursor, so there wouldn't be a need to reserve these 32 blocks. I guess there's also the issue that the upstream driver can't actually expose/use Plane 4 at all today. yes, this example is for Broxton. During this writeup only optimization of "not to use cursor plane instead use 4th plane" was there, but code was still allocating DDB for cursor. True, upstream doesn't expose 4th plane, this was as per the local optimization for Broxton. Regards, -Mahesh That said, your overall example here still gets the important points across and is very much appreciated. allocate minimum blocks for each plane 8 * 4 = 32 remaining blocks = 476 - 32 = 444 Relative Data Rate for Planes Plane 1 = 1440 * 2560 * 3 = 11059200 Plane 2 = 1440 * 2560 * 3 = 11059200 Plane 3 = 1440 * 48 * 3 = 207360 Plane 4 = 1440 * 96 * 3 = 414720 Total Relative BW= 22740480 - Allocate Buffer buffer allocation = (Plane relative data rate / total data rate) * total remaming DDB + minimum plane DDB Plane 1 buffer allocation = (11059200 / 22740480) * 444 + 8 = 223 Plane 2 buffer allocation = (11059200 / 22740480) * 444 + 8 = 223 Plane 3 buffer allocation = (207360 / 22740480) * 444 + 8 = 12 Plane 4 buffer allocation = (414720 / 22740480) * 444 + 8 = 16 In this case it forced driver to disable Plane 3 & 4. Driver need to use more efficient way to allocate buffer that is optimum for power. New Algorithm suggested by HW team is: 1. Calculate minimum buffer allocations for each plane and for each watermark level 2. Add minimum buffer allocations required for enabling WM7 for all the planes Level 0 = 41 + 41 + 41 + 41 = 164 Level 1 = 42 + 42 + 42 + 42 = 168 Level 2 = 42 + 42 + 42 + 42 = 168 Level 3 = 94 + 94 + 94 + 94 = 376 Level 4 = 94 + 94 + 94 + 94 = 376 Level 5 = 94 + 94 + 94 + 94 = 376 Level 6 = 94 + 94 + 94 + 94 = 376 Level 7 = 94 + 94 + 94 + 94 = 376 3. Check to see how many buffer allocation are left and enable the best case. In this case since we have 476 blocks we can enable WM0-7 on all 4 planes. Let's say if we have only 200 block available then the best cases allocation is to enable Level2 which requires 168 blocks It's probably worth noting that the use cases that are most likely to benefit from this are those with large differences in the height of the 'shortest' plane vs the height of the 'tallest' plane. It's the blind proportional distribution of remaining blocks in the current algorithm that prevents 'short' planes from reaching their minimum block requirements for various watermark levels (and if they can't even reach the WM0 minimum, then the plane can't be used at all). There will certainly still be cases where the overall display configuration (with lots of pipes and planes in use) simply requires more blocks than the hardware has to even reach WM0, no matter how we slice up the limited DDB size, but the changes here will definitely help prevent us from rejecting atomic commits for some configurations we actually could handle. Matt Mahesh Kumar (11): drm/i915: fix naming of fixed_16_16 wrapper. drm/i915: Add more wrapper for fixed_point_16_16 operations drm/i915: Use fixed_16_16 wrapper for division operation
Re: [Intel-gfx] [PATCH 01/12] drm/i915: Remove kref from i915_sw_fence
On Fri, May 12, 2017 at 10:43:31AM +0300, Mika Kuoppala wrote:
> Chris Wilson writes:
>
> > My original intention was for i915_sw_fence to be the base class and
> > provide the reference count for the container. This was from starting
> > with a design to handle async_work. In practice, for i915 we embed
> > fences into structs which have their own independent reference counting,
> > making the i915_sw_fence.kref duplicitous. If we remove the kref, we
> > remove the i915_sw_fence's ability to free itself and its independence,
> > it can only exist within a container and must be supplied with a
> > callback to handle its release.
> >
> > Signed-off-by: Chris Wilson
> > ---
> > drivers/gpu/drm/i915/i915_sw_fence.c | 55
> >
> > drivers/gpu/drm/i915/i915_sw_fence.h | 1 -
> > 2 files changed, 11 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c
> > b/drivers/gpu/drm/i915/i915_sw_fence.c
> > index a277f8eb7beb..a0a690d6627e 100644
> > --- a/drivers/gpu/drm/i915/i915_sw_fence.c
> > +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> > @@ -120,34 +120,6 @@ void i915_sw_fence_fini(struct i915_sw_fence *fence)
> > }
> > #endif
> >
> > -static void i915_sw_fence_release(struct kref *kref)
> > -{
> > - struct i915_sw_fence *fence = container_of(kref, typeof(*fence), kref);
> > -
> > - WARN_ON(atomic_read(&fence->pending) > 0);
> > - debug_fence_destroy(fence);
> > -
> > - if (fence->flags & I915_SW_FENCE_MASK) {
> > - __i915_sw_fence_notify(fence, FENCE_FREE);
>
> Is the current design so that the callback equals embeddding?
To embed, in 99.999% of cases you really do have to employ the callback
to resolve lifetime. Yes, it is possible by explicitly calling wait
that you do know its state; and that is useful for on-stack fences.
However, the current usage of debugobjects negects that (on-stack
objects are banned), so we do a need constructor for
i915_sw_fence_init_onstack(), so mandating a callback simplifies
everyone's lives.
-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 07/12] drm/i915: Use a define for the default priority [0]
Chris Wilson writes:
> Explicitly assign the default priority, and give it a name. After much
> discussion, we have chosen to call it I915_PRIORITY_NORMAL!
>
> Signed-off-by: Chris Wilson
Reviewed-by: Mika Kuoppala
> ---
> drivers/gpu/drm/i915/i915_gem_context.c | 1 +
> drivers/gpu/drm/i915/i915_gem_request.h | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index 31a73c39239f..c5d1666d7071 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -199,6 +199,7 @@ __create_hw_context(struct drm_i915_private *dev_priv,
> kref_init(&ctx->ref);
> list_add_tail(&ctx->link, &dev_priv->context_list);
> ctx->i915 = dev_priv;
> + ctx->priority = I915_PRIORITY_NORMAL;
>
> /* Default context will never have a file_priv */
> ret = DEFAULT_CONTEXT_HANDLE;
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.h
> b/drivers/gpu/drm/i915/i915_gem_request.h
> index 4ccab5affd3c..0ecfc5e2d707 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.h
> +++ b/drivers/gpu/drm/i915/i915_gem_request.h
> @@ -70,6 +70,7 @@ struct i915_priotree {
> struct rb_node node;
> int priority;
> #define I915_PRIORITY_MAX 1024
> +#define I915_PRIORITY_NORMAL 0
> #define I915_PRIORITY_MIN (-I915_PRIORITY_MAX)
> };
>
> --
> 2.11.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/dp: start a DPCD based DP sink/branch device quirk database
On Thu, 11 May 2017, Daniel Vetter wrote:
> On Thu, May 11, 2017 at 12:57:20PM +0300, Jani Nikula wrote:
>> Face the fact, there are Display Port sink and branch devices out there
>> in the wild that don't follow the Display Port specifications, or they
>> have bugs, or just otherwise require special treatment. Start a common
>> quirk database the drivers can query based on OUI (with the option of
>> expanding to device identification string and hardware/firmware
>> revisions later). At least for now, we leave the workarounds for the
>> drivers to implement as they see fit.
>>
>> For starters, add a branch device that can't handle full 24-bit main
>> link M and N attributes properly. Naturally, the workaround of reducing
>> main link attributes for all devices ended up in regressions for other
>> devices. So here we are.
>>
>> Cc: Ville Syrjälä
>> Cc: Dhinakaran Pandiyan
>> Cc: Clint Taylor
>> Cc: Adam Jackson
>> Cc: Harry Wentland
>> Signed-off-by: Jani Nikula
>
> A few small bikesheds below. Ack in principle.
> -Daniel
>
>> ---
>> drivers/gpu/drm/drm_dp_helper.c | 61
>> +
>> include/drm/drm_dp_helper.h | 7 +
>> 2 files changed, 68 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_helper.c
>> b/drivers/gpu/drm/drm_dp_helper.c
>> index 3e5f52110ea1..58b2ced33151 100644
>> --- a/drivers/gpu/drm/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_helper.c
>> @@ -1208,3 +1208,64 @@ int drm_dp_stop_crc(struct drm_dp_aux *aux)
>> return 0;
>> }
>> EXPORT_SYMBOL(drm_dp_stop_crc);
>> +
>> +/*
>> + * Display Port sink and branch devices in the wild have a variety of bugs,
>> try
>> + * to collect them here. The quirks are shared, but it's up to the drivers
>> to
>> + * implement workarounds for them.
>> + */
>
> I'd stash this in the kerneldoc for drm_dp_get_quirks().
>
>> +
>> +/*
>> + * FIXME: Add support for device identification and hardware/firmware
>> revision.
>> + */
>> +struct dpcd_quirk {
>> +u8 oui[3];
>> +bool is_branch;
>> +u32 quirks;
>> +};
>> +
>> +#define OUI(first, second, third) { (first), (second), (third) }
>> +
>> +static const struct dpcd_quirk dpcd_quirk_list[] = {
>> +/* Analogix 7737 needs reduced M and N at HBR2 link rates */
>> +{ OUI(0x00, 0x22, 0xb9), true, DP_DPCD_QUIRK_LIMITED_M_N },
>> +};
>> +
>> +#undef OUI
>> +
>> +/**
>> + * drm_dp_get_quirks() - get quirks for the sink/branch device
>> + * @oui: pointer to len bytes of DPCD at 0x400 (sink) or 0x500 (branch)
>> + * @len: 3-12 bytes of DPCD data
>> + * @is_branch: true for branch devices, false for sink devices
>> + *
>> + * Get a bit mask of DPCD quirks for the sink/branch device. The quirk data
>> is
>> + * shared but it's up to the drivers to act on the data.
>> + *
>> + * For now, only the OUI (first three bytes) is used, but this may be
>> extended
>> + * to device identification string and hardware/firmware revisions later.
>> + */
>> +u32 drm_dp_get_quirks(const u8 *oui, unsigned int len, bool is_branch)
>
> Should we either base this on drm_dp_aux (so that this function would
> query for quirks), or on struct drm_dp_link (which is kinda something
> tegra and msm started to use to cache some sink properties)?
>
> This is with an eye towards handling your FIXME, and maybe even converging
> the dp helpers a bit more again.
>
> Personally (aka my own bikeshed right now) I'm leaning towards struct
> drm_dp_link and extending that.
Except we don't use drm_dp_link in i915, and I don't see starting to use
that would help us in any way. What we do with the source/sink/link
properties is much more complicated.
As to drm_dp_aux, I think that's the wrong level of abstraction, and I'd
much prefer the drivers would be in control of the DPCD access.
I guess we could move struct intel_dp_desc and intel_dp_read_desc() to
drm helpers, and store the quirks in struct intel_dp_desc upon reading.
>
>> +{
>> +const struct dpcd_quirk *quirk;
>> +u32 quirks = 0;
>> +int i;
>> +
>> +if (WARN_ON_ONCE(len < 3))
>> +return 0;
>> +
>> +for (i = 0; i < ARRAY_SIZE(dpcd_quirk_list); i++) {
>> +quirk = &dpcd_quirk_list[i];
>> +
>> +if (quirk->is_branch != is_branch)
>> +continue;
>> +
>> +if (memcmp(quirk->oui, oui, 3) != 0)
>> +continue;
>> +
>> +quirks |= quirk->quirks;
>> +}
>> +
>> +return quirks;
>> +}
>> +EXPORT_SYMBOL(drm_dp_get_quirks);
>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>> index f7007e544f29..8abe9897fe59 100644
>> --- a/include/drm/drm_dp_helper.h
>> +++ b/include/drm/drm_dp_helper.h
>> @@ -1079,4 +1079,11 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux);
>> int drm_dp_start_crc(struct drm_dp_aux *aux, struct drm_crtc *crtc);
>> int drm_dp_stop_crc(struct drm_dp_aux *aux);
>>
>> +/* Display Port sink and branch device quirks. */
>> +
>> +/* The sink is limited to small li
Re: [Intel-gfx] [PATCH 01/12] drm/i915: Remove kref from i915_sw_fence
Chris Wilson writes:
> My original intention was for i915_sw_fence to be the base class and
> provide the reference count for the container. This was from starting
> with a design to handle async_work. In practice, for i915 we embed
> fences into structs which have their own independent reference counting,
> making the i915_sw_fence.kref duplicitous. If we remove the kref, we
> remove the i915_sw_fence's ability to free itself and its independence,
> it can only exist within a container and must be supplied with a
> callback to handle its release.
>
> Signed-off-by: Chris Wilson
> ---
> drivers/gpu/drm/i915/i915_sw_fence.c | 55
>
> drivers/gpu/drm/i915/i915_sw_fence.h | 1 -
> 2 files changed, 11 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c
> b/drivers/gpu/drm/i915/i915_sw_fence.c
> index a277f8eb7beb..a0a690d6627e 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.c
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> @@ -120,34 +120,6 @@ void i915_sw_fence_fini(struct i915_sw_fence *fence)
> }
> #endif
>
> -static void i915_sw_fence_release(struct kref *kref)
> -{
> - struct i915_sw_fence *fence = container_of(kref, typeof(*fence), kref);
> -
> - WARN_ON(atomic_read(&fence->pending) > 0);
> - debug_fence_destroy(fence);
> -
> - if (fence->flags & I915_SW_FENCE_MASK) {
> - __i915_sw_fence_notify(fence, FENCE_FREE);
Is the current design so that the callback equals embeddding?
-Mika
> - } else {
> - i915_sw_fence_fini(fence);
> - kfree(fence);
> - }
> -}
> -
> -static void i915_sw_fence_put(struct i915_sw_fence *fence)
> -{
> - debug_fence_assert(fence);
> - kref_put(&fence->kref, i915_sw_fence_release);
> -}
> -
> -static struct i915_sw_fence *i915_sw_fence_get(struct i915_sw_fence *fence)
> -{
> - debug_fence_assert(fence);
> - kref_get(&fence->kref);
> - return fence;
> -}
> -
> static void __i915_sw_fence_wake_up_all(struct i915_sw_fence *fence,
> struct list_head *continuation)
> {
> @@ -202,13 +174,15 @@ static void __i915_sw_fence_complete(struct
> i915_sw_fence *fence,
>
> debug_fence_set_state(fence, DEBUG_FENCE_IDLE, DEBUG_FENCE_NOTIFY);
>
> - if (fence->flags & I915_SW_FENCE_MASK &&
> - __i915_sw_fence_notify(fence, FENCE_COMPLETE) != NOTIFY_DONE)
> + if (__i915_sw_fence_notify(fence, FENCE_COMPLETE) != NOTIFY_DONE)
> return;
>
> debug_fence_set_state(fence, DEBUG_FENCE_NOTIFY, DEBUG_FENCE_IDLE);
>
> __i915_sw_fence_wake_up_all(fence, continuation);
> +
> + debug_fence_destroy(fence);
> + __i915_sw_fence_notify(fence, FENCE_FREE);
> }
>
> static void i915_sw_fence_complete(struct i915_sw_fence *fence)
> @@ -232,33 +206,26 @@ void __i915_sw_fence_init(struct i915_sw_fence *fence,
> const char *name,
> struct lock_class_key *key)
> {
> - BUG_ON((unsigned long)fn & ~I915_SW_FENCE_MASK);
> + BUG_ON(!fn || (unsigned long)fn & ~I915_SW_FENCE_MASK);
>
> debug_fence_init(fence);
>
> __init_waitqueue_head(&fence->wait, name, key);
> - kref_init(&fence->kref);
> atomic_set(&fence->pending, 1);
> fence->flags = (unsigned long)fn;
> }
>
> -static void __i915_sw_fence_commit(struct i915_sw_fence *fence)
> -{
> - i915_sw_fence_complete(fence);
> - i915_sw_fence_put(fence);
> -}
> -
> void i915_sw_fence_commit(struct i915_sw_fence *fence)
> {
> debug_fence_activate(fence);
> - __i915_sw_fence_commit(fence);
> + i915_sw_fence_complete(fence);
> }
>
> static int i915_sw_fence_wake(wait_queue_t *wq, unsigned mode, int flags,
> void *key)
> {
> list_del(&wq->task_list);
> __i915_sw_fence_complete(wq->private, key);
> - i915_sw_fence_put(wq->private);
> +
> if (wq->flags & I915_SW_FENCE_FLAG_ALLOC)
> kfree(wq);
> return 0;
> @@ -353,7 +320,7 @@ static int __i915_sw_fence_await_sw_fence(struct
> i915_sw_fence *fence,
> INIT_LIST_HEAD(&wq->task_list);
> wq->flags = pending;
> wq->func = i915_sw_fence_wake;
> - wq->private = i915_sw_fence_get(fence);
> + wq->private = fence;
>
> i915_sw_fence_await(fence);
>
> @@ -402,7 +369,7 @@ static void timer_i915_sw_fence_wake(unsigned long data)
> dma_fence_put(cb->dma);
> cb->dma = NULL;
>
> - __i915_sw_fence_commit(cb->fence);
> + i915_sw_fence_complete(cb->fence);
> cb->timer.function = NULL;
> }
>
> @@ -413,7 +380,7 @@ static void dma_i915_sw_fence_wake(struct dma_fence *dma,
>
> del_timer_sync(&cb->timer);
> if (cb->timer.function)
> - __i915_sw_fence_commit(cb->fence);
> + i915_sw_fence_complete(cb->fence);
> dma_fence_put(cb->dma);
>
> kfree(cb);
> @@ -440,7 +407,7 @@ int i915_sw_fence_await_dma_fence(struct i915_sw_fence
Re: [Intel-gfx] [PATCH 12/12] drm/i915: Don't force serialisation on marking up execlists irq posted
Chris Wilson writes:
> Since we coordinate with the execlists tasklet using a locked schedule
> operation that ensures that after we set the engine->irq_posted we
> always have an invocation of the tasklet, we do not need to use a locked
> operation to set the engine->irq_posted itself.
>
> Signed-off-by: Chris Wilson
This has r-b from Tvrtko.
-Mika
> ---
> drivers/gpu/drm/i915/i915_irq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index a58152dd7021..7b7f55a28eec 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1324,7 +1324,7 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32
> iir, int test_shift)
>
> if (iir & (GT_CONTEXT_SWITCH_INTERRUPT << test_shift)) {
> if (port_count(&engine->execlist_port[0])) {
> - set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> + __set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> tasklet = true;
> }
> }
> --
> 2.11.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
