Re: [Intel-gfx] [PATCH v2] drm/i915/dp: Update connector status for DP MST hotplugs

2016-11-13 Thread Daniel Vetter 
On Fri, Nov 11, 2016 at 10:21:39PM +0100, Daniel Vetter wrote:
> On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote:
> > Hotplugging a monitor in DP MST configuration triggers short pulses.
> > Although the short pulse handling path ends up in the MST hotplug function,
> > we do not perform a detect before sending the hotplug uvent. This leads to
> > the connector status not being updated when the userspace calls
> > DRM_IOCTL_MODE_GETCONNECTOR.
> > 
> > So, let's call the connector function ->detect() to update the connector
> > status before sending the uevent.
> > 
> > v2: Update connector status inside mode_config mutex (Ville)
> > 
> > Signed-off-by: Dhinakaran Pandiyan 
> 
> quick comments from me on irc:
> 
>  dhnkrn, really tricky I think
>  dhnkrn, also feels wrong from an entire design pov
>  I'd expect the topology manager could tell us from which exact 
> downstream port the short pulse came
>  which means we should be able to get at that connector without 
> taking the global lock
>  i.e. at least avoid the for_each_connector
>  on the detect itself, I need to think
>  but we might need to essentially postpone the ->detect work to the 
> hotplug worker
>  except that it's not a direct hpd from our chip
>  but a downstream one
>  this is going to be fun
>  Hmm .. Does updating the connector status need the lock or is it the 
> detect() ?
>  yup
>  and we might need to do additional dpcd transactions (e.g. for load 
> detect probing on downstream ports)
>  or at least edid reads
>  both recurse back into dp mst helper code like ville said
>  dhnkrn, so instead of doing the detect work from the hotplug code
>  put them on some list
>  need to be careful in case they are already there
>  list only protected with dedicated lock
>  then launch the hpd worker
>  and walk that list from there, with the mode_config.mutex to do the 
> full-on probe
>  we can't call ->detect from dig_port_work, and walking the entire 
> connector list in there is also not a good idea imo
>  dhnkrn, but that's just my preliminary analysis from like 10 minutes 
> of thinking and checking what ville raised
>  needs more thought probably
>  dhnkrn, to clarify your question: both connector->status and calling 
> ->detect need mode_config.mutex
>  that mutex is what protects output probe state ccr: i might 
> have udev configured incorrectly then
> 
> Cheers, Daniel
> > ---
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 22 +-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
> > b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 3ffbd69..8e36a96 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct 
> > drm_dp_mst_topology_mgr *mgr)
> > struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
> > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > struct drm_device *dev = intel_dig_port->base.base.dev;
> > +   struct intel_connector *intel_connector;
> > +   bool changed = false;
> > +   enum drm_connector_status old_status;
> > +   struct drm_mode_config *mode_config = &dev->mode_config;
> > +
> > +   mutex_lock(&mode_config->mutex);
> > +   for_each_intel_connector(dev, intel_connector) {
> > +   struct drm_connector *connector = &(intel_connector->base);
> > +
> > +   if (intel_connector->mst_port != intel_dp)
> > +   continue;
> > +
> > +   old_status = connector->status;
> > +   connector->status = connector->funcs->detect(connector, false);
> > +
> > +   if (old_status != connector->status)
> > +   changed = true;
> > +   }
> > +   mutex_unlock(&mode_config->mutex);
> >  
> > -   drm_kms_helper_hotplug_event(dev);

I just realized that this call here will call down into the fbdev helper,
which already does the above ->detect calls. At least if there's no
compositor running. So I wonder why the deadlock ville pointed out isn't
blowing up already ...
-Daniel

> > +   if (changed)
> > +   drm_kms_helper_hotplug_event(dev);
> >  }
> >  
> >  static const struct drm_dp_mst_topology_cbs mst_cbs = {
> > -- 
> > 2.7.4
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3] drm/i915: start adding dp mst audio

2016-11-13 Thread libin . yang 
From: Libin Yang 

(This patch is developed by Dave Airlie  originally)

This patch adds support for DP MST audio in i915.

Enable audio codec when DP MST is enabled if has_audio flag is set.
Disable audio codec when DP MST is disabled if has_audio flag is set.

Another separated patches to support DP MST audio will be implemented
in audio driver.

v2:
Rebased.
v3:
refine to support updated intel_audio_codec_enable()

Signed-off-by: Libin Yang 
Signed-off-by: Dhinakaran Pandiyan 
Reviewed-by: Lyude 
Signed-off-by: Rodrigo Vivi 
Link: 
http://patchwork.freedesktop.org/patch/msgid/1474334681-22690-6-git-send-email-dhinakaran.pandi...@intel.com
---
 drivers/gpu/drm/i915/i915_debugfs.c | 19 ++-
 drivers/gpu/drm/i915/intel_ddi.c| 20 +++-
 drivers/gpu/drm/i915/intel_dp_mst.c | 18 ++
 drivers/gpu/drm/i915/intel_drv.h|  2 ++
 4 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index bce3880..3442381 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2893,6 +2893,20 @@ static void intel_dp_info(struct seq_file *m,
&intel_dp->aux);
 }
 
+static void intel_dp_mst_info(struct seq_file *m,
+ struct intel_connector *intel_connector)
+{
+   struct intel_encoder *intel_encoder = intel_connector->encoder;
+   struct intel_dp_mst_encoder *intel_mst =
+   enc_to_mst(&intel_encoder->base);
+   struct intel_digital_port *intel_dig_port = intel_mst->primary;
+   struct intel_dp *intel_dp = &intel_dig_port->dp;
+   bool has_audio = drm_dp_mst_port_has_audio(&intel_dp->mst_mgr,
+   intel_connector->port);
+
+   seq_printf(m, "\taudio support: %s\n", yesno(has_audio));
+}
+
 static void intel_hdmi_info(struct seq_file *m,
struct intel_connector *intel_connector)
 {
@@ -2935,7 +2949,10 @@ static void intel_connector_info(struct seq_file *m,
switch (connector->connector_type) {
case DRM_MODE_CONNECTOR_DisplayPort:
case DRM_MODE_CONNECTOR_eDP:
-   intel_dp_info(m, intel_connector);
+   if (intel_encoder->type == INTEL_OUTPUT_DP_MST)
+   intel_dp_mst_info(m, intel_connector);
+   else
+   intel_dp_info(m, intel_connector);
break;
case DRM_MODE_CONNECTOR_LVDS:
if (intel_encoder->type == INTEL_OUTPUT_LVDS)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 0ad4e16..b1e3442 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1951,6 +1951,19 @@ void intel_ddi_prepare_link_retrain(struct intel_dp 
*intel_dp)
udelay(600);
 }
 
+bool intel_ddi_is_audio_enabled(struct drm_i915_private *dev_priv,
+struct intel_crtc *intel_crtc)
+{
+   u32 temp;
+
+   if (intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_AUDIO)) {
+   temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
+   if (temp & AUDIO_OUTPUT_ENABLE(intel_crtc->pipe))
+   return true;
+   }
+   return false;
+}
+
 void intel_ddi_get_config(struct intel_encoder *encoder,
  struct intel_crtc_state *pipe_config)
 {
@@ -2016,11 +2029,8 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
break;
}
 
-   if (intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_AUDIO)) {
-   temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
-   if (temp & AUDIO_OUTPUT_ENABLE(intel_crtc->pipe))
-   pipe_config->has_audio = true;
-   }
+   pipe_config->has_audio =
+   intel_ddi_is_audio_enabled(dev_priv, intel_crtc);
 
if (encoder->type == INTEL_OUTPUT_EDP && dev_priv->vbt.edp.bpp &&
pipe_config->pipe_bpp > dev_priv->vbt.edp.bpp) {
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
b/drivers/gpu/drm/i915/intel_dp_mst.c
index 3ffbd69..96ff6a9 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -37,6 +37,8 @@ static bool intel_dp_mst_compute_config(struct intel_encoder 
*encoder,
struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
struct intel_digital_port *intel_dig_port = intel_mst->primary;
struct intel_dp *intel_dp = &intel_dig_port->dp;
+   struct intel_connector *connector =
+   to_intel_connector(conn_state->connector);
struct drm_atomic_state *state;
int bpp;
int lane_count, slots;
@@ -59,6 +61,8 @@ static bool intel_dp_mst_compute_config(struct intel_encoder 
*encoder,
 
state = pipe_config->base.state;
 
+   if (drm_dp_mst_port_has_audio(&intel_dp->mst_mgr, connector->port))
+   pipe_config->ha

Re: [Intel-gfx] [PATCH v3] drm/i915: start adding dp mst audio

2016-11-13 Thread Yang, Libin 
As the dp-mst flicker issue has been fixed by Dhinakaran, let's resend this 
patch to gfx driver to support DP-MST audio.

This patch is also refined to support the new intel_audio_codec_enable() 
parameters.

Regards,
Libin


> -Original Message-
> From: Yang, Libin
> Sent: Monday, November 14, 2016 1:32 PM
> To: intel-gfx@lists.freedesktop.org; jani.nik...@linux.intel.com;
> ville.syrj...@linux.intel.com; Vetter, Daniel ;
> ti...@suse.de
> Cc: Yang, Libin ; Libin Yang
> ; Pandiyan, Dhinakaran
> ; Vivi, Rodrigo 
> Subject: [PATCH v3] drm/i915: start adding dp mst audio
> 
> From: Libin Yang 
> 
> (This patch is developed by Dave Airlie  originally)
> 
> This patch adds support for DP MST audio in i915.
> 
> Enable audio codec when DP MST is enabled if has_audio flag is set.
> Disable audio codec when DP MST is disabled if has_audio flag is set.
> 
> Another separated patches to support DP MST audio will be implemented in
> audio driver.
> 
> v2:
> Rebased.
> v3:
> refine to support updated intel_audio_codec_enable()
> 
> Signed-off-by: Libin Yang 
> Signed-off-by: Dhinakaran Pandiyan 
> Reviewed-by: Lyude 
> Signed-off-by: Rodrigo Vivi 
> Link: http://patchwork.freedesktop.org/patch/msgid/1474334681-22690-6-
> git-send-email-dhinakaran.pandi...@intel.com
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 19 ++-
>  drivers/gpu/drm/i915/intel_ddi.c| 20 +++-
>  drivers/gpu/drm/i915/intel_dp_mst.c | 18 ++
>  drivers/gpu/drm/i915/intel_drv.h|  2 ++
>  4 files changed, 53 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index bce3880..3442381 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2893,6 +2893,20 @@ static void intel_dp_info(struct seq_file *m,
>   &intel_dp->aux);
>  }
> 
> +static void intel_dp_mst_info(struct seq_file *m,
> +   struct intel_connector *intel_connector) {
> + struct intel_encoder *intel_encoder = intel_connector->encoder;
> + struct intel_dp_mst_encoder *intel_mst =
> + enc_to_mst(&intel_encoder->base);
> + struct intel_digital_port *intel_dig_port = intel_mst->primary;
> + struct intel_dp *intel_dp = &intel_dig_port->dp;
> + bool has_audio = drm_dp_mst_port_has_audio(&intel_dp->mst_mgr,
> + intel_connector->port);
> +
> + seq_printf(m, "\taudio support: %s\n", yesno(has_audio)); }
> +
>  static void intel_hdmi_info(struct seq_file *m,
>   struct intel_connector *intel_connector)  { @@ -
> 2935,7 +2949,10 @@ static void intel_connector_info(struct seq_file *m,
>   switch (connector->connector_type) {
>   case DRM_MODE_CONNECTOR_DisplayPort:
>   case DRM_MODE_CONNECTOR_eDP:
> - intel_dp_info(m, intel_connector);
> + if (intel_encoder->type == INTEL_OUTPUT_DP_MST)
> + intel_dp_mst_info(m, intel_connector);
> + else
> + intel_dp_info(m, intel_connector);
>   break;
>   case DRM_MODE_CONNECTOR_LVDS:
>   if (intel_encoder->type == INTEL_OUTPUT_LVDS) diff --git
> a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 0ad4e16..b1e3442 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1951,6 +1951,19 @@ void intel_ddi_prepare_link_retrain(struct
> intel_dp *intel_dp)
>   udelay(600);
>  }
> 
> +bool intel_ddi_is_audio_enabled(struct drm_i915_private *dev_priv,
> +  struct intel_crtc *intel_crtc)
> +{
> + u32 temp;
> +
> + if (intel_display_power_is_enabled(dev_priv,
> POWER_DOMAIN_AUDIO)) {
> + temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> + if (temp & AUDIO_OUTPUT_ENABLE(intel_crtc->pipe))
> + return true;
> + }
> + return false;
> +}
> +
>  void intel_ddi_get_config(struct intel_encoder *encoder,
> struct intel_crtc_state *pipe_config)  { @@ -2016,11
> +2029,8 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>   break;
>   }
> 
> - if (intel_display_power_is_enabled(dev_priv,
> POWER_DOMAIN_AUDIO)) {
> - temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> - if (temp & AUDIO_OUTPUT_ENABLE(intel_crtc->pipe))
> - pipe_config->has_audio = true;
> - }
> + pipe_config->has_audio =
> + intel_ddi_is_audio_enabled(dev_priv, intel_crtc);
> 
>   if (encoder->type == INTEL_OUTPUT_EDP && dev_priv->vbt.edp.bpp
> &&
>   pipe_config->pipe_bpp > dev_priv->vbt.edp.bpp) { diff --git
> a/drivers/gpu/drm/i915/intel_dp_mst.c
> b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 3ffbd69..96ff6a9 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -37,6 +3

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: start adding dp mst audio

2016-11-13 Thread Patchwork 
== Series Details ==

Series: drm/i915: start adding dp mst audio
URL   : https://patchwork.freedesktop.org/series/15245/
State : success

== Summary ==

Series 15245v1 drm/i915: start adding dp mst audio
https://patchwork.freedesktop.org/api/1.0/series/15245/revisions/1/mbox/


fi-bdw-5557u total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050 total:244  pass:204  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700 total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900 total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820 total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770  total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650   total:244  pass:191  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770  total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hqtotal:244  pass:223  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k total:244  pass:222  dwarn:1   dfail:0   fail:0   skip:21 
fi-snb-2520m total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600  total:244  pass:211  dwarn:0   dfail:0   fail:0   skip:33 

48ac4d084b5911d7c88a6b54175731d6a87f1769 drm-intel-nightly: 
2016y-11m-13d-16h-50m-41s UTC integration manifest
46ec54b drm/i915: start adding dp mst audio

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2975/
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC i-g-t 4/4] Add support for hotplug testing with the Chamelium

2016-11-13 Thread Daniel Vetter 
On Mon, Nov 07, 2016 at 07:05:16PM -0500, Lyude wrote:
> For the purpose of testing things such as hotplugging and bad monitors,
> the ChromeOS team ended up designing a neat little device known as the
> Chamelium. More information on this can be found here:
> 
>   https://www.chromium.org/chromium-os/testing/chamelium
> 
> This adds support for a couple of things to intel-gpu-tools:
>  - igt library functions for connecting to udev and monitoring it for
>hotplug events, loosely based off of the unfinished hotplugging
>implementation in testdisplay
>  - Library functions for controlling the chamelium in tests using
>xmlrpc. A couple of RPC calls were ommitted here, mainly because they
>didn't seem very useful for our needs or because they're just plain
>broken
>  - A set of basic tests using the chamelium.
> 
> Because there's no surefire way that I know of where we can map which
> chamelium port belongs to which port on the system being tested (we
> could just use hotplugging, but then we'd be relying on something that
> might be broken on the machine and potentially give false positives for
> certain tests), most of the chamelium tests will figure out whether or
> not a connection happened by counting the number of connectors matching
> the status we're looking for before hotplugging with the chamelium, vs.
> after hotplugging it.
> 
> Tests which require that we know which port belongs to a certain port
> (such as ones where we actually perform a modeset) will unplug all of
> the chamelium ports, plug the desired port, then use the first DRM
> connector with the desired connector type that's marked as connected. In
> order to ensure we don't end up using the wrong connector, these tests
> will skip if they find any connectors with the desired type marked as
> connected before performing the hotplug on the chamelium.
> 
> Running these tests requires (of course) a working Chamelium, along with
> the RPC URL for the chamelium being specified in the environment
> variable CHAMELIUM_HOST. If no URL is specified, the tests will just
> skip on their own. As well, tests for connectors which are not actually
> present on the system or the chamelium will skip on their own as well.
> 
> Signed-off-by: Lyude 
> ---
>  configure.ac   |  13 +
>  lib/Makefile.am|  10 +-
>  lib/igt.h  |   1 +
>  lib/igt_chamelium.c| 628 
> +
>  lib/igt_chamelium.h|  77 ++

Since you typed these nice gtkdocs, please also add it to the .xml in
docs/ and make sure it looks all good (./autogen.sh --enable-gtk-docs).

Wrt the api itself I think all we need is agreement from Tomeu that this
is the right thing for his chamelium use-cases, too. And Tomeu has commit
rights, so can push this stuff for you.
-Daniel


>  lib/igt_kms.c  | 107 +
>  lib/igt_kms.h  |  13 +-
>  scripts/run-tests.sh   |   4 +-
>  tests/Makefile.am  |   5 +-
>  tests/Makefile.sources |   1 +
>  tests/chamelium.c  | 549 ++
>  11 files changed, 1403 insertions(+), 5 deletions(-)
>  create mode 100644 lib/igt_chamelium.c
>  create mode 100644 lib/igt_chamelium.h
>  create mode 100644 tests/chamelium.c
> 
> diff --git a/configure.ac b/configure.ac
> index 735cfd5..88113b2 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -259,6 +259,18 @@ if test "x$with_libunwind" = xyes; then
> AC_MSG_ERROR([libunwind not found. Use 
> --without-libunwind to disable libunwind support.]))
>  fi
>  
> +# enable support for using the chamelium
> +AC_ARG_ENABLE(chamelium,
> +   AS_HELP_STRING([--without-chamelium],
> +  [Build tests without chamelium support]),
> +   [], [with_chamelium=yes])
> +
> +AM_CONDITIONAL(HAVE_CHAMELIUM, [test "x$with_chamelium" = xyes])
> +if test "x$with_chamelium" = xyes; then
> + AC_DEFINE(HAVE_CHAMELIUM, 1, [chamelium suport])
> + PKG_CHECK_MODULES(XMLRPC, xmlrpc_client)
> +fi
> +
>  # enable debug symbols
>  AC_ARG_ENABLE(debug,
> AS_HELP_STRING([--disable-debug],
> @@ -356,6 +368,7 @@ echo "   Assembler  : ${enable_assembler}"
>  echo "   Debugger   : ${enable_debugger}"
>  echo "   Overlay: X: ${enable_overlay_xlib}, Xv: 
> ${enable_overlay_xvlib}"
>  echo "   x86-specific tools : ${build_x86}"
> +echo "   Chamelium support  : ${with_chamelium}"
>  echo ""
>  echo " • API-Documentation  : ${enable_gtk_doc}"
>  echo " • Fail on warnings   : ${enable_werror}"
> diff --git a/lib/Makefile.am b/lib/Makefile.am
> index 4c0893d..aeac43a 100644
> --- a/lib/Makefile.am
> +++ b/lib/Makefile.am
> @@ -22,8 +22,14 @@ if !HAVE_LIBDRM_INTEL
>  stubs/drm/intel_bufmgr.h
>  endif
>  
> +if HAVE_CHAMELIUM
> +libintel_tools_la_SOURCES += \
> + igt_chamelium.c \
> + igt_chamelium.h
> +endif
> +
>  AM_CPPFLAGS = -I$(top

Re: [Intel-gfx] [PATCH] drm/i915: Assuming non-DP++ port if dvo_port is HDMI and there's no AUX ch specified in the VBT

2016-11-13 Thread Daniel Vetter 
On Fri, Nov 11, 2016 at 07:14:24PM +0200, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
> 
> My heuristic for detecting type 1 DVI DP++ adaptors based on the VBT
> port information apparently didn't survive the reality of buggy VBTs.
> In this particular case we have a machine with a natice HDMI port, but
> the VBT telsl us it's a DP++ port based on its capabilities.
> 
> The dvo_port information in VBT does claim that we're dealing with a
> HDMI port though, but we have other machines which do the same even
> when they actually have DP++ ports. So that piece of information alone
> isn't sufficient to tell the two apart.
> 
> After staring at a bunch of VBTs from various machines, I have to
> conclude that the only other semi-reliable clue we can use is the
> presence of the AUX channel in the VBT. On this particular machine
> AUX channel is specified as zero, whereas on some of the other machines
> which listed the DP++ port as HDMI have a non-zero AUX channel.
> 
> I've also seen VBTs which have dvo_port a DP but have a zero AUX
> channel. I believe those we need to treat as DP ports, so we'll limit
> the AUX channel check to just the cases where dvo_port is HDMI.
> 
> If we encounter any more serious failures with this heuristic I think
> we'll have to have to throw it out entirely. But that could mean that
> there is a risk of type 1 DVI dongle users getting greeted by a
> black screen, so I'd rather not go there unless absolutely necessary.
> 
> Cc: Daniel Otero 
> Cc: sta...@vger.kernel.org
> Tested-by: Daniel Otero 
> Fixes: d61992565bd3 ("drm/i915: Determine DP++ type 1 DVI adaptor presence 
> based on VBT")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97994
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/intel_bios.c | 32 
>  drivers/gpu/drm/i915/intel_vbt_defs.h |  3 ++-
>  2 files changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c 
> b/drivers/gpu/drm/i915/intel_bios.c
> index 5ab646ef8c9f..33ed05186810 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1147,7 +1147,7 @@ static void parse_ddi_port(struct drm_i915_private 
> *dev_priv, enum port port,
>   if (!child)
>   return;
>  
> - aux_channel = child->raw[25];
> + aux_channel = child->common.aux_channel;
>   ddc_pin = child->common.ddc_pin;
>  
>   is_dvi = child->common.device_type & DEVICE_TYPE_TMDS_DVI_SIGNALING;
> @@ -1677,7 +1677,8 @@ bool intel_bios_is_port_edp(struct drm_i915_private 
> *dev_priv, enum port port)
>   return false;
>  }
>  
> -bool intel_bios_is_port_dp_dual_mode(struct drm_i915_private *dev_priv, enum 
> port port)
> +static bool child_dev_is_dp_dual_mode(const union child_device_config 
> *p_child,
> +   enum port port)
>  {
>   static const struct {
>   u16 dp, hdmi;
> @@ -1691,22 +1692,37 @@ bool intel_bios_is_port_dp_dual_mode(struct 
> drm_i915_private *dev_priv, enum por
>   [PORT_D] = { DVO_PORT_DPD, DVO_PORT_HDMID, },
>   [PORT_E] = { DVO_PORT_DPE, DVO_PORT_HDMIE, },
>   };
> - int i;
>  
>   if (port == PORT_A || port >= ARRAY_SIZE(port_mapping))
>   return false;
>  
> - if (!dev_priv->vbt.child_dev_num)
> + if ((p_child->common.device_type & DEVICE_TYPE_DP_DUAL_MODE_BITS) !=
> + (DEVICE_TYPE_DP_DUAL_MODE & DEVICE_TYPE_DP_DUAL_MODE_BITS))
> + return false;
> +
> + if (p_child->common.dvo_port == port_mapping[port].dp)
> + return true;
> +
> + /* Only accept a HDMI dvo_port as DP++ if it has an AUX channel */
> + if (p_child->common.dvo_port == port_mapping[port].hdmi &&
> + p_child->common.aux_channel != 0)
> + return true;
> +
> + return false;
> +}
> +
> +bool intel_bios_is_port_dp_dual_mode(struct drm_i915_private *dev_priv, enum 
> port port)
> +{
> + int i;
> +
> + if (port == PORT_A)
>   return false;

We check for PORT_A twice now. Otherwise contains what it says on the tin,
but no idea whether this is the perfect solution either. Also need to
check vbt docs for the right bit, but assuming that checks out (I'll
report on irc), and with the above nitpick fixed:

Reviewed-by: Daniel Vetter 

>  
>   for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
>   const union child_device_config *p_child =
>   &dev_priv->vbt.child_dev[i];
>  
> - if ((p_child->common.dvo_port == port_mapping[port].dp ||
> -  p_child->common.dvo_port == port_mapping[port].hdmi) &&
> - (p_child->common.device_type & 
> DEVICE_TYPE_DP_DUAL_MODE_BITS) ==
> - (DEVICE_TYPE_DP_DUAL_MODE & DEVICE_TYPE_DP_DUAL_MODE_BITS))
> + if (child_dev_is_dp_dual_mode(p_child, port))
>   return true;
>   }
>  
> diff --git a/drivers/gp

Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure during modeset

2016-11-13 Thread Manasi Navare 
On Fri, Nov 11, 2016 at 07:42:16PM +, Cheng, Tony wrote:
> In case of link training failure and requiring user space to set a lower 
> mode, would full mode set address it?  How do we make user mode select lower 
> resolution?
> 
> For example 4k@60Hz monitor, and link training at 4 lane HBR2 fails and 
> fallback to 4 lanes HBR, 4k@60 is no longer doable.  I would think preventing 
> user mode from seeing 4K@60Hz from the start is a easier and more robust 
> solution and requiring user mode to have logic to decide how to fallback.
>

Hi Tony,

So in case of link training failure, we first call mode_valid that will use the 
lower link rate/lane count
values based on the link training fallback to validate the modes and prune the 
modes that are higher than
the available link. After this is done, then we send the uevent to userspace 
indicating that link status is BAD and 
it will redo a probe and trigger a modeset for next possible lower resolution.

Manasi

 
> -Original Message-
> From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com] 
> Sent: Friday, November 11, 2016 11:44 AM
> To: Cheng, Tony 
> Cc: Deucher, Alexander ; 'Jani Nikula' 
> ; Manasi Navare ; 
> dri-de...@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Wentland, 
> Harry ; Peres, Martin 
> Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure during 
> modeset
> 
> On Fri, Nov 11, 2016 at 04:21:58PM +, Cheng, Tony wrote:
> > For HDMI, you can yank the cable, plug back in, HDMI will light up without 
> > user mode or kernel mode doing anything.
> > 
> > For DP this is not possible, someone will have to retrain the link when 
> > plugging back in or DP will not light up.  We see that on Ubuntu if someone 
> > unplug display and plug it back into same connector, we do not get KMS so 
> > in this case.  It appears there is some optimizations somewhere in user 
> > mode stack which I am not familiar with yet.  dal added workaround to 
> > retrain the link to light it back up (after the test training at end of 
> > detection).
> 
> The link should get retrained as the driver should check the link state on 
> HPD and retrain if it's not good. At least that's what we do in i915. Of 
> course if it's not the same display that got plugged back, the retraining 
> might fail. The new property can help with that case as userspace would then 
> attempt a full modeset after the failed link training.
> 
> > 
> > >From user mode perspective I think it make sense to keep CRTC running, so 
> > >vblank is still going so UMD isn't impacted.   As regard to connector and 
> > >encoder does it matter if kernel mode change state without user mode 
> > >knowing?  It seems to me those are more informational to UMD as UMD 
> > >doesn't act on them.
> > 
> > windows does not know link state and all link management is hidden behind 
> > kernel driver.   
> 
> If the user visible state doesn't change in any way, then the kernel could 
> try to manage it all internally.
> 
> > 
> > -Original Message-
> > From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
> > Sent: Friday, November 11, 2016 9:05 AM
> > To: Cheng, Tony 
> > Cc: Deucher, Alexander ; 'Jani Nikula' 
> > ; Manasi Navare 
> > ; dri-de...@lists.freedesktop.org; 
> > intel-gfx@lists.freedesktop.org; Wentland, Harry 
> > ; Peres, Martin 
> > Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure 
> > during modeset
> > 
> > On Thu, Nov 10, 2016 at 06:51:40PM +, Cheng, Tony wrote:
> > > Amdgpu dal implementation will do a test link training at end of 
> > > detection to verify we can achieve the capability reported in DPCD.  We 
> > > then report mode base on result of test training.
> > > 
> > > AMD hardware (at least the generations supported by amdgpu) is able to 
> > > link train without timing being setup (DP encoder and CRTC is decoupled). 
> > >  Do we have limitation from other vendors where you need timing to be 
> > > there before you can link train?
> > 
> > I can't recall the specifics for all of our supported platforms, but at 
> > least I have the recollection that it would be the case yes.
> > 
> > The other problem wiyh this apporach is that even if you don't need the 
> > crtc, you still need the link itself. What happens if the link is still 
> > active since userspace just didn't bother to shut it down when the cable 
> > was yanked? Can you keep the crtc going but stop it from feeding the link 
> > in a way that userspace won't be able to notice? The kms design has always 
> > been pretty much that policy is in userspace, and thus the kernel shouldn't 
> > shut down crtcs unless explicitly asked to do so.
> > 
> > > 
> > > We can also past DP1.2 link layer compliance with this approach.
> > > 
> > > -Original Message-
> > > From: Deucher, Alexander
> > > Sent: Thursday, November 10, 2016 1:43 PM
> > > To: 'Jani Nikula' ; Manasi Navare 
> > > ; dri-de...@lists.freedesktop.org; 
> > > intel-gfx@lists.freedesktop.org; Wen

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Make GPU pages movable

2016-11-13 Thread akash goel 
On Thu, Nov 10, 2016 at 1:00 PM, Goel, Akash  wrote:
>
>
> On 11/10/2016 12:09 PM, Hugh Dickins wrote:
>>
>> On Fri, 4 Nov 2016, akash.g...@intel.com wrote:
>>>
>>> From: Chris Wilson 
>>>
>>> On a long run of more than 2-3 days, physical memory tends to get
>>> fragmented severely, which considerably slows down the system. In such a
>>> scenario, the shrinker is also unable to help as lack of memory is not
>>> the actual problem, since it has been observed that there are enough free
>>> pages of 0 order. This also manifests itself when an indiviual zone in
>>> the mm runs out of pages and if we cannot migrate pages between zones,
>>> the kernel hits an out-of-memory even though there are free pages (and
>>> often all of swap) available.
>>>
>>> To address the issue of external fragementation, kernel does a compaction
>>> (which involves migration of pages) but it's efficacy depends upon how
>>> many pages are marked as MOVABLE, as only those pages can be migrated.
>>>
>>> Currently the backing pages for GPU buffers are allocated from shmemfs
>>> with GFP_RECLAIMABLE flag, in units of 4KB pages.  In the case of limited
>>> swap space, it may not be possible always to reclaim or swap-out pages of
>>> all the inactive objects, to make way for free space allowing formation
>>> of higher order groups of physically-contiguous pages on compaction.
>>>
>>> Just marking the GPU pages as MOVABLE will not suffice, as i915.ko has to
>>> pin the pages if they are in use by GPU, which will prevent their
>>> migration. So the migratepage callback in shmem is also hooked up to get
>>> a notification when kernel initiates the page migration. On the
>>> notification, i915.ko appropriately unpin the pages.  With this we can
>>> effectively mark the GPU pages as MOVABLE and hence mitigate the
>>> fragmentation problem.
>>>
>>> v2:
>>>  - Rename the migration routine to gem_shrink_migratepage, move it to the
>>>shrinker file, and use the existing constructs (Chris)
>>>  - To cleanup, add a new helper function to encapsulate all page
>>> migration
>>>skip conditions (Chris)
>>>  - Add a new local helper function in shrinker file, for dropping the
>>>backing pages, and call the same from gem_shrink() also (Chris)
>>>
>>> v3:
>>>  - Fix/invert the check on the return value of unsafe_drop_pages (Chris)
>>>
>>> v4:
>>>  - Minor tidy
>>>
>>> v5:
>>>  - Fix unsafe usage of unsafe_drop_pages()
>>>  - Rebase onto vmap-notifier
>>>
>>> v6:
>>> - Remove i915_gem_object_get/put across unsafe_drop_pages() as with
>>>   struct_mutex protection object can't disappear. (Chris)
>>>
>>> Testcase: igt/gem_shrink
>>> Bugzilla: (e.g.) https://bugs.freedesktop.org/show_bug.cgi?id=90254
>>> Cc: Hugh Dickins 
>>> Cc: linux...@kvack.org
>>> Signed-off-by: Sourab Gupta 
>>> Signed-off-by: Akash Goel 
>>> Signed-off-by: Chris Wilson 
>>> Reviewed-by: Joonas Lahtinen 
>>> Reviewed-by: Chris Wilson 
>>
>>
>> I'm confused!  But perhaps it's gone around and around between you all,
>> I'm not sure what the rules are then.  I think this sequence implies
>> that Sourab wrote it originally, then Akash and Chris passed it on
>> with refinements - but then Chris wouldn't add Reviewed-by.
>>
> Thank you very much for the review and sorry for all the needless confusion.
>
> Chris actually conceived the patches and prepared an initial version of them
> (hence he is the Author).
> I & Sourab did the further refinements and fixed issues (all those
> page_private stuff).
> Chris then reviewed the final patch and also recently did a rebase for it.
>
>
>>> ---
>>>  drivers/gpu/drm/i915/i915_drv.h  |   2 +
>>>  drivers/gpu/drm/i915/i915_gem.c  |   9 ++-
>>>  drivers/gpu/drm/i915/i915_gem_shrinker.c | 132
>>> +++
>>>  3 files changed, 142 insertions(+), 1 deletion(-)
>>>
snip
>>
>>> @@ -4185,6 +4189,8 @@ struct drm_i915_gem_object *
>>> goto fail;
>>>
>>> mask = GFP_HIGHUSER | __GFP_RECLAIMABLE;
>>> +   if (IS_ENABLED(MIGRATION))
>>> +   mask |= __GFP_MOVABLE;
>>
>>
>> I was going to suggest just make that unconditional,
>> mask = GFP_HIGHUSER_MOVABLE | __GFP_RECLAIMABLE;
>>
>> But then I wondered what that __GFP_RECLAIMABLE actually achieves?
>> These pages are already __GFP_RECLAIM (inside GFP_HIGHUSER) and on
>> the LRU.  It affects gfpflags_to_migratetype(), but I'm not familiar
>> with what that different migratetype will end up doing.
>>
>
> Will check for this.
>

The anti-fragmentation technique used by kernel is based on the idea
of grouping pages with identical mobility (UNMOVABLE, RECLAIMABLE,
MOVABLE) together.
__GFP_RECLAIMABLE, like  __GFP_MOVABLE, specifies the
mobility/migration type of the page and serves a different purpose
than __GFP_RECLAIM.

Also as per the below snippet from gfpflags_to_migratetype(), looks
like __GFP_MOVABLE &  __GFP_RECLAIMABLE can't be used together, which
makes sense.
/* Convert GFP flags to their corresponding migrate type */