| Message ID | 20190826132216.2823-6-oleg.vasilev@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/7] drm: move DP_MAX_DOWNSTREAM_PORTS from i915 to drm core | expand |
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
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
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
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.
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
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.
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; };