[v3,5/7] drm/nouveau: utilize subconnector property for DP
diff mbox series

Message ID 20190826132216.2823-6-oleg.vasilev@intel.com
State New
Headers show
Series
  • [v3,1/7] drm: move DP_MAX_DOWNSTREAM_PORTS from i915 to drm core
Related show

Commit Message

Oleg Vasilev Aug. 26, 2019, 1:22 p.m. UTC
Since DP-specific information is stored in driver's structures, every
driver needs to implement subconnector property by itself.

Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: nouveau@lists.freedesktop.org
---
 drivers/gpu/drm/nouveau/nouveau_connector.c | 13 +++++++++++++
 drivers/gpu/drm/nouveau/nouveau_dp.c        |  9 +++++++++
 drivers/gpu/drm/nouveau/nouveau_encoder.h   |  1 +
 3 files changed, 23 insertions(+)

Comments

Ilia Mirkin Aug. 26, 2019, 1:36 p.m. UTC | #1
This should probably be fixed to account for the scenario where an
HDMI connector is plugged directly into the DP++ port. I don't think
the dp.subconnector property will be valid in that case.
(Unfortunately I don't remember how one detects that particular
situation.)

On Mon, Aug 26, 2019 at 9:22 AM Oleg Vasilev <oleg.vasilev@intel.com> wrote:
>
> Since DP-specific information is stored in driver's structures, every
> driver needs to implement subconnector property by itself.
>
> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
> Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: nouveau@lists.freedesktop.org
> ---
>  drivers/gpu/drm/nouveau/nouveau_connector.c | 13 +++++++++++++
>  drivers/gpu/drm/nouveau/nouveau_dp.c        |  9 +++++++++
>  drivers/gpu/drm/nouveau/nouveau_encoder.h   |  1 +
>  3 files changed, 23 insertions(+)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index 94dfa2e5a9ab..d9c116cc11b9 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> @@ -635,6 +635,17 @@ nouveau_connector_detect(struct drm_connector *connector, bool force)
>         pm_runtime_mark_last_busy(dev->dev);
>         pm_runtime_put_autosuspend(dev->dev);
>
> +       if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
> +           connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
> +               enum drm_mode_subconnector subconnector = DRM_MODE_SUBCONNECTOR_Unknown;
> +
> +               if (conn_status == connector_status_connected && nv_encoder)
> +                       subconnector = nv_encoder->dp.subconnector;
> +               drm_object_property_set_value(&connector->base,
> +                       connector->dev->mode_config.dp_subconnector_property,
> +                       subconnector);
> +       }
> +
>         return conn_status;
>  }
>
> @@ -1359,6 +1370,8 @@ nouveau_connector_create(struct drm_device *dev,
>                         kfree(nv_connector);
>                         return ERR_PTR(ret);
>                 }
> +
> +               drm_mode_add_dp_subconnector_property(connector);
>                 funcs = &nouveau_connector_funcs;
>                 break;
>         default:
> diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c b/drivers/gpu/drm/nouveau/nouveau_dp.c
> index 2674f1587457..85eac853e3f8 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_dp.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
> @@ -62,6 +62,7 @@ nouveau_dp_detect(struct nouveau_encoder *nv_encoder)
>         struct nouveau_drm *drm = nouveau_drm(dev);
>         struct nvkm_i2c_aux *aux;
>         u8 dpcd[8];
> +       u8 port_cap[DP_MAX_DOWNSTREAM_PORTS] = {};
>         int ret;
>
>         aux = nv_encoder->aux;
> @@ -72,6 +73,14 @@ nouveau_dp_detect(struct nouveau_encoder *nv_encoder)
>         if (ret)
>                 return ret;
>
> +       if (dpcd[DP_DPCD_REV] > 0x10) {
> +               ret = nvkm_rdaux(aux, DP_DOWNSTREAM_PORT_0,
> +                                port_cap, DP_MAX_DOWNSTREAM_PORTS);
> +               if (ret)
> +                       memset(port_cap, 0, DP_MAX_DOWNSTREAM_PORTS);
> +       }
> +       nv_encoder->dp.subconnector = drm_dp_subconnector_type(dpcd, port_cap);
> +
>         nv_encoder->dp.link_bw = 27000 * dpcd[1];
>         nv_encoder->dp.link_nr = dpcd[2] & DP_MAX_LANE_COUNT_MASK;
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_encoder.h b/drivers/gpu/drm/nouveau/nouveau_encoder.h
> index 3517f920bf89..e17971a30221 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_encoder.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_encoder.h
> @@ -63,6 +63,7 @@ struct nouveau_encoder {
>                         struct nv50_mstm *mstm;
>                         int link_nr;
>                         int link_bw;
> +                       enum drm_mode_subconnector subconnector;
>                 } dp;
>         };
>
> --
> 2.23.0
>
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
Ville Syrjälä Aug. 28, 2019, 2:38 p.m. UTC | #2
On Mon, Aug 26, 2019 at 09:36:50AM -0400, Ilia Mirkin wrote:
> This should probably be fixed to account for the scenario where an
> HDMI connector is plugged directly into the DP++ port. I don't think
> the dp.subconnector property will be valid in that case.
> (Unfortunately I don't remember how one detects that particular
> situation.)

One may or may not be able to detect it very well. I've seen dongles
that automagically change the DFP type from DP++ to DP/HDMI depending
on what's plugged in, and I've also seen dongles that leave the DFP
type to DP++.

I'm actually broiling a series of patches which try to improve how
i915 handles various DFP types, and for that I'm thinking of using
a combination of the DFP type and the EDID digital input type to
differentiate between the two cases like so:

static bool is_edid_digital_input_dp(const struct edid *edid)
{
	return edid && edid->revision >= 4 &&
		edid->input & DRM_EDID_INPUT_DIGITAL &&
		(edid->input & DRM_EDID_DIGITAL_TYPE_MASK) == DRM_EDID_DIGITAL_TYPE_DP;
}

{
	switch (port_cap[0] & DP_DS_PORT_TYPE_MASK) {
	case DP_DS_PORT_TYPE_DP:
		DP_STUFF;
	case DP_DS_PORT_TYPE_DP_DUALMODE:
		if (is_edid_digital_input_dp(edid))
			DP_STUFF;
		/* fall through */
	case DP_DS_PORT_TYPE_HDMI:
	case DP_DS_PORT_TYPE_DVI:
		TMDS_STUFF;
}

> 
> On Mon, Aug 26, 2019 at 9:22 AM Oleg Vasilev <oleg.vasilev@intel.com> wrote:
> >
> > Since DP-specific information is stored in driver's structures, every
> > driver needs to implement subconnector property by itself.
> >
> > Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
> > Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>
> > Cc: Ben Skeggs <bskeggs@redhat.com>
> > Cc: nouveau@lists.freedesktop.org
> > ---
> >  drivers/gpu/drm/nouveau/nouveau_connector.c | 13 +++++++++++++
> >  drivers/gpu/drm/nouveau/nouveau_dp.c        |  9 +++++++++
> >  drivers/gpu/drm/nouveau/nouveau_encoder.h   |  1 +
> >  3 files changed, 23 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
> > index 94dfa2e5a9ab..d9c116cc11b9 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> > @@ -635,6 +635,17 @@ nouveau_connector_detect(struct drm_connector *connector, bool force)
> >         pm_runtime_mark_last_busy(dev->dev);
> >         pm_runtime_put_autosuspend(dev->dev);
> >
> > +       if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
> > +           connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
> > +               enum drm_mode_subconnector subconnector = DRM_MODE_SUBCONNECTOR_Unknown;
> > +
> > +               if (conn_status == connector_status_connected && nv_encoder)
> > +                       subconnector = nv_encoder->dp.subconnector;
> > +               drm_object_property_set_value(&connector->base,
> > +                       connector->dev->mode_config.dp_subconnector_property,
> > +                       subconnector);
> > +       }
> > +
> >         return conn_status;
> >  }
> >
> > @@ -1359,6 +1370,8 @@ nouveau_connector_create(struct drm_device *dev,
> >                         kfree(nv_connector);
> >                         return ERR_PTR(ret);
> >                 }
> > +
> > +               drm_mode_add_dp_subconnector_property(connector);
> >                 funcs = &nouveau_connector_funcs;
> >                 break;
> >         default:
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c b/drivers/gpu/drm/nouveau/nouveau_dp.c
> > index 2674f1587457..85eac853e3f8 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_dp.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
> > @@ -62,6 +62,7 @@ nouveau_dp_detect(struct nouveau_encoder *nv_encoder)
> >         struct nouveau_drm *drm = nouveau_drm(dev);
> >         struct nvkm_i2c_aux *aux;
> >         u8 dpcd[8];
> > +       u8 port_cap[DP_MAX_DOWNSTREAM_PORTS] = {};
> >         int ret;
> >
> >         aux = nv_encoder->aux;
> > @@ -72,6 +73,14 @@ nouveau_dp_detect(struct nouveau_encoder *nv_encoder)
> >         if (ret)
> >                 return ret;
> >
> > +       if (dpcd[DP_DPCD_REV] > 0x10) {
> > +               ret = nvkm_rdaux(aux, DP_DOWNSTREAM_PORT_0,
> > +                                port_cap, DP_MAX_DOWNSTREAM_PORTS);
> > +               if (ret)
> > +                       memset(port_cap, 0, DP_MAX_DOWNSTREAM_PORTS);
> > +       }
> > +       nv_encoder->dp.subconnector = drm_dp_subconnector_type(dpcd, port_cap);
> > +
> >         nv_encoder->dp.link_bw = 27000 * dpcd[1];
> >         nv_encoder->dp.link_nr = dpcd[2] & DP_MAX_LANE_COUNT_MASK;
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_encoder.h b/drivers/gpu/drm/nouveau/nouveau_encoder.h
> > index 3517f920bf89..e17971a30221 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_encoder.h
> > +++ b/drivers/gpu/drm/nouveau/nouveau_encoder.h
> > @@ -63,6 +63,7 @@ struct nouveau_encoder {
> >                         struct nv50_mstm *mstm;
> >                         int link_nr;
> >                         int link_bw;
> > +                       enum drm_mode_subconnector subconnector;
> >                 } dp;
> >         };
> >
> > --
> > 2.23.0
> >
> > _______________________________________________
> > Nouveau mailing list
> > Nouveau@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/nouveau
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Ilia Mirkin Aug. 28, 2019, 2:47 p.m. UTC | #3
On Wed, Aug 28, 2019 at 10:38 AM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Mon, Aug 26, 2019 at 09:36:50AM -0400, Ilia Mirkin wrote:
> > This should probably be fixed to account for the scenario where an
> > HDMI connector is plugged directly into the DP++ port. I don't think
> > the dp.subconnector property will be valid in that case.
> > (Unfortunately I don't remember how one detects that particular
> > situation.)
>
> One may or may not be able to detect it very well. I've seen dongles
> that automagically change the DFP type from DP++ to DP/HDMI depending
> on what's plugged in, and I've also seen dongles that leave the DFP
> type to DP++.

Well, our internal logic certainly knows if it's driving DP or TMDS. I
just don't remember how it knows this.

  -ilia
Ville Syrjälä Aug. 28, 2019, 2:54 p.m. UTC | #4
On Wed, Aug 28, 2019 at 10:47:08AM -0400, Ilia Mirkin wrote:
> On Wed, Aug 28, 2019 at 10:38 AM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Mon, Aug 26, 2019 at 09:36:50AM -0400, Ilia Mirkin wrote:
> > > This should probably be fixed to account for the scenario where an
> > > HDMI connector is plugged directly into the DP++ port. I don't think
> > > the dp.subconnector property will be valid in that case.
> > > (Unfortunately I don't remember how one detects that particular
> > > situation.)
> >
> > One may or may not be able to detect it very well. I've seen dongles
> > that automagically change the DFP type from DP++ to DP/HDMI depending
> > on what's plugged in, and I've also seen dongles that leave the DFP
> > type to DP++.
> 
> Well, our internal logic certainly knows if it's driving DP or TMDS. I
> just don't remember how it knows this.

You'll be driving DP in this case. The DFP will be the one driving
DP or TMDS depending on what's plugged in.
Ilia Mirkin Aug. 28, 2019, 3:11 p.m. UTC | #5
On Wed, Aug 28, 2019 at 10:54 AM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Wed, Aug 28, 2019 at 10:47:08AM -0400, Ilia Mirkin wrote:
> > On Wed, Aug 28, 2019 at 10:38 AM Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > >
> > > On Mon, Aug 26, 2019 at 09:36:50AM -0400, Ilia Mirkin wrote:
> > > > This should probably be fixed to account for the scenario where an
> > > > HDMI connector is plugged directly into the DP++ port. I don't think
> > > > the dp.subconnector property will be valid in that case.
> > > > (Unfortunately I don't remember how one detects that particular
> > > > situation.)
> > >
> > > One may or may not be able to detect it very well. I've seen dongles
> > > that automagically change the DFP type from DP++ to DP/HDMI depending
> > > on what's plugged in, and I've also seen dongles that leave the DFP
> > > type to DP++.
> >
> > Well, our internal logic certainly knows if it's driving DP or TMDS. I
> > just don't remember how it knows this.
>
> You'll be driving DP in this case. The DFP will be the one driving
> DP or TMDS depending on what's plugged in.

OK. That's not the case I was worried about though :)

I'm worried about the case where you plug a true HDMI thing into the
DP port, which will then be driven by HDMI and none of the dp.* things
will be set (I don't remember, but they might even be in a union). I
think Intel multiplies connectors for this, but nouveau/amd just have
a single connector that handles both.

  -ilia
Ville Syrjälä Aug. 28, 2019, 3:41 p.m. UTC | #6
On Wed, Aug 28, 2019 at 11:11:41AM -0400, Ilia Mirkin wrote:
> On Wed, Aug 28, 2019 at 10:54 AM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Wed, Aug 28, 2019 at 10:47:08AM -0400, Ilia Mirkin wrote:
> > > On Wed, Aug 28, 2019 at 10:38 AM Ville Syrjälä
> > > <ville.syrjala@linux.intel.com> wrote:
> > > >
> > > > On Mon, Aug 26, 2019 at 09:36:50AM -0400, Ilia Mirkin wrote:
> > > > > This should probably be fixed to account for the scenario where an
> > > > > HDMI connector is plugged directly into the DP++ port. I don't think
> > > > > the dp.subconnector property will be valid in that case.
> > > > > (Unfortunately I don't remember how one detects that particular
> > > > > situation.)
> > > >
> > > > One may or may not be able to detect it very well. I've seen dongles
> > > > that automagically change the DFP type from DP++ to DP/HDMI depending
> > > > on what's plugged in, and I've also seen dongles that leave the DFP
> > > > type to DP++.
> > >
> > > Well, our internal logic certainly knows if it's driving DP or TMDS. I
> > > just don't remember how it knows this.
> >
> > You'll be driving DP in this case. The DFP will be the one driving
> > DP or TMDS depending on what's plugged in.
> 
> OK. That's not the case I was worried about though :)
> 
> I'm worried about the case where you plug a true HDMI thing into the
> DP port, which will then be driven by HDMI and none of the dp.* things
> will be set (I don't remember, but they might even be in a union). I
> think Intel multiplies connectors for this, but nouveau/amd just have
> a single connector that handles both.

Ah. Yeah, that's a bit more confusing since you kinda now have two
levels of subconnectors. Not sure how to deal with that tbh.

Patch
diff mbox series

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 94dfa2e5a9ab..d9c116cc11b9 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -635,6 +635,17 @@  nouveau_connector_detect(struct drm_connector *connector, bool force)
 	pm_runtime_mark_last_busy(dev->dev);
 	pm_runtime_put_autosuspend(dev->dev);
 
+	if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
+	    connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
+		enum drm_mode_subconnector subconnector = DRM_MODE_SUBCONNECTOR_Unknown;
+
+		if (conn_status == connector_status_connected && nv_encoder)
+			subconnector = nv_encoder->dp.subconnector;
+		drm_object_property_set_value(&connector->base,
+			connector->dev->mode_config.dp_subconnector_property,
+			subconnector);
+	}
+
 	return conn_status;
 }
 
@@ -1359,6 +1370,8 @@  nouveau_connector_create(struct drm_device *dev,
 			kfree(nv_connector);
 			return ERR_PTR(ret);
 		}
+
+		drm_mode_add_dp_subconnector_property(connector);
 		funcs = &nouveau_connector_funcs;
 		break;
 	default:
diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c b/drivers/gpu/drm/nouveau/nouveau_dp.c
index 2674f1587457..85eac853e3f8 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dp.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
@@ -62,6 +62,7 @@  nouveau_dp_detect(struct nouveau_encoder *nv_encoder)
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_i2c_aux *aux;
 	u8 dpcd[8];
+	u8 port_cap[DP_MAX_DOWNSTREAM_PORTS] = {};
 	int ret;
 
 	aux = nv_encoder->aux;
@@ -72,6 +73,14 @@  nouveau_dp_detect(struct nouveau_encoder *nv_encoder)
 	if (ret)
 		return ret;
 
+	if (dpcd[DP_DPCD_REV] > 0x10) {
+		ret = nvkm_rdaux(aux, DP_DOWNSTREAM_PORT_0,
+				 port_cap, DP_MAX_DOWNSTREAM_PORTS);
+		if (ret)
+			memset(port_cap, 0, DP_MAX_DOWNSTREAM_PORTS);
+	}
+	nv_encoder->dp.subconnector = drm_dp_subconnector_type(dpcd, port_cap);
+
 	nv_encoder->dp.link_bw = 27000 * dpcd[1];
 	nv_encoder->dp.link_nr = dpcd[2] & DP_MAX_LANE_COUNT_MASK;
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_encoder.h b/drivers/gpu/drm/nouveau/nouveau_encoder.h
index 3517f920bf89..e17971a30221 100644
--- a/drivers/gpu/drm/nouveau/nouveau_encoder.h
+++ b/drivers/gpu/drm/nouveau/nouveau_encoder.h
@@ -63,6 +63,7 @@  struct nouveau_encoder {
 			struct nv50_mstm *mstm;
 			int link_nr;
 			int link_bw;
+			enum drm_mode_subconnector subconnector;
 		} dp;
 	};