| Message ID | 1464686746-5099-4-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 31, 2016 at 02:55:44PM +0530, Shashank Sharma wrote: > lspcon is a tricky device to detect. > When in LS mode: > It should be detected as a HDMI device, by reading EDID, on > I2C over Aux chanel > > When in PCON mode: > It should be detected as a DP device by reading DPCD over the > Aux channel. > > This patch accommodates these specific requirements of lspcon detection > by adding appropriate changes in I915 drivers HDMI/DP detection sequence. > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 14 ++++++++++++++ > drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_hdmi.c | 14 ++++++++++---- > drivers/gpu/drm/i915/intel_lspcon.c | 6 ++++++ > 5 files changed, 44 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index c454744..811c829 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2243,12 +2243,26 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder, > { > int type = encoder->type; > int port = intel_ddi_get_encoder_port(encoder); > + struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); > > WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n"); > > if (port == PORT_A) > pipe_config->cpu_transcoder = TRANSCODER_EDP; > > + /* > + * A digital port with active lspcon device, should be detected > + * as HDMI when in LS mode and as DP when in PCON mode. > + */ > + if (is_lspcon_active(dig_port)) { > + struct intel_lspcon *lspcon = &dig_port->lspcon; > + > + if (lspcon->mode_of_op == DRM_LSPCON_MODE_LS) > + type = INTEL_OUTPUT_HDMI; > + else > + type = INTEL_OUTPUT_DISPLAYPORT; > + } Argh. I really wanted to kill this dual role DDI encoder mess (see [1]) I realize that with LSPCON we probably don't want separate encoders for the two roles. Or maybe we do? At least we don't want two connectors, which probably means two encoders might become messy. In any case I don't think we want to be frobbing around with the encoder->type anymore. Maybe I need to rethink my approach to DDI encoders, and try to come up with something sane. For LSPCON at least we want to keep track of the current mode in pipe_config most likely. For LSPCON it's maybe a bit easier since the mode won't affect the DDC stuff and whanot, so from the probe side there is just one role except perhaps w.r.t. DPCD. For regular DDI stuff I'm still thinking two encoder might be the most sensible apporach since we have totally different paths for probe as well. Probably the biggest kink in all of this is hpd handling. What, if anything, should hpd_pulse do for LSPCON? And if something, should it only do it in PCON mode? How is HPD routed anyway with LSPCON? [1] https://patchwork.freedesktop.org/series/1596/ > + > if (type == INTEL_OUTPUT_HDMI) > return intel_hdmi_compute_config(encoder, pipe_config); > else > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index aa9c59e..39ce16e 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4276,6 +4276,18 @@ intel_dp_detect(struct drm_connector *connector, bool force) > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > connector->base.id, connector->name); > > + /* > + * LSPCON is a DP->HDMI converter which should be detected as > + * HDMI in LS mode, and DP in PCON mode. So if LSPCON is in LS > + * mode, do not try to read DPCD, but detect as HDMI. > + */ > + if (is_lspcon_active(intel_dig_port)) { > + struct intel_lspcon *lspcon = &intel_dig_port->lspcon; > + > + if (lspcon->mode_of_op == DRM_LSPCON_MODE_LS) > + return lspcon_ls_mode_detect(connector, force); > + } > + > if (intel_dp->is_mst) { > /* MST devices are disconnected from a monitor POV */ > intel_dp_unset_edid(intel_dp); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 205a463..fa77886 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1452,6 +1452,8 @@ struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder); > bool intel_hdmi_compute_config(struct intel_encoder *encoder, > struct intel_crtc_state *pipe_config); > void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable); > +enum drm_connector_status > +intel_hdmi_detect(struct drm_connector *connector, bool force); > > /* intel_lvds.c */ > void intel_lvds_init(struct drm_device *dev); > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index 6b52c6a..79184e2 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1444,15 +1444,21 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force) > { > struct drm_i915_private *dev_priv = to_i915(connector->dev); > struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); > + struct intel_digital_port *dig_port = hdmi_to_dig_port(intel_hdmi); > + struct i2c_adapter *adapter; > struct edid *edid = NULL; > bool connected = false; > > if (force) { > intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > > - edid = drm_get_edid(connector, > - intel_gmbus_get_adapter(dev_priv, > - intel_hdmi->ddc_bus)); > + if (is_lspcon_active(dig_port)) > + adapter = &dig_port->lspcon.aux->ddc; > + else > + adapter = intel_gmbus_get_adapter(dev_priv, > + intel_hdmi->ddc_bus); > + > + edid = drm_get_edid(connector, adapter); I'm not a fan of this. This is even taking the wrong power domain now, as does intel_hdmi_detect(). Probably LSPCON should just have a dedicated codepath for this stuff. If we ever have a board design with a DP++ port without a GMBUS connection, then we might have rethink things because then we'd have to use AUX also for HDMI DDC. But so far I'm not aware of such board designs existing, so might as well keep LSPCON separate. > > intel_hdmi_dp_dual_mode_detect(connector, edid != NULL); > > @@ -1479,7 +1485,7 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force) > return connected; > } > > -static enum drm_connector_status > +enum drm_connector_status > intel_hdmi_detect(struct drm_connector *connector, bool force) > { > enum drm_connector_status status; > diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c > index dd50491..75b5028 100644 > --- a/drivers/gpu/drm/i915/intel_lspcon.c > +++ b/drivers/gpu/drm/i915/intel_lspcon.c > @@ -37,6 +37,12 @@ bool is_lspcon_active(struct intel_digital_port *dig_port) > return dig_port->lspcon.active; > } > > +enum drm_connector_status > +lspcon_ls_mode_detect(struct drm_connector *connector, bool force) > +{ > + return intel_hdmi_detect(connector, force); > +} > + > enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon) > { > enum drm_lspcon_mode current_mode; > -- > 1.9.1
Regards Shashank On 5/31/2016 10:00 PM, Ville Syrjälä wrote: > On Tue, May 31, 2016 at 02:55:44PM +0530, Shashank Sharma wrote: >> lspcon is a tricky device to detect. >> When in LS mode: >> It should be detected as a HDMI device, by reading EDID, on >> I2C over Aux chanel >> >> When in PCON mode: >> It should be detected as a DP device by reading DPCD over the >> Aux channel. >> >> This patch accommodates these specific requirements of lspcon detection >> by adding appropriate changes in I915 drivers HDMI/DP detection sequence. >> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> drivers/gpu/drm/i915/intel_ddi.c | 14 ++++++++++++++ >> drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++++ >> drivers/gpu/drm/i915/intel_drv.h | 2 ++ >> drivers/gpu/drm/i915/intel_hdmi.c | 14 ++++++++++---- >> drivers/gpu/drm/i915/intel_lspcon.c | 6 ++++++ >> 5 files changed, 44 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> index c454744..811c829 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -2243,12 +2243,26 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder, >> { >> int type = encoder->type; >> int port = intel_ddi_get_encoder_port(encoder); >> + struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); >> >> WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n"); >> >> if (port == PORT_A) >> pipe_config->cpu_transcoder = TRANSCODER_EDP; >> >> + /* >> + * A digital port with active lspcon device, should be detected >> + * as HDMI when in LS mode and as DP when in PCON mode. >> + */ >> + if (is_lspcon_active(dig_port)) { >> + struct intel_lspcon *lspcon = &dig_port->lspcon; >> + >> + if (lspcon->mode_of_op == DRM_LSPCON_MODE_LS) >> + type = INTEL_OUTPUT_HDMI; >> + else >> + type = INTEL_OUTPUT_DISPLAYPORT; >> + } > > Argh. I really wanted to kill this dual role DDI encoder mess (see [1]) > > I realize that with LSPCON we probably don't want separate encoders for > the two roles. Or maybe we do? At least we don't want two connectors, > which probably means two encoders might become messy. > Yes, you are right. We neither need two encoders or two connectors. Actually if you closely observe, we have just attached once encoder, which is type DDI, and once connector, which is type DP. > In any case I don't think we want to be frobbing around with the > encoder->type anymore. Actually this check is just to cover any corner case / future case where a lspcon device detection path reaches this path, in LS mode. In LS mode of operation, there is no DPCD, and the aux channel read/write for DPCD register will not be valid, and we will not be able to detect the monitor. In this case, we should just get EDID over i2c_over_aux and detect. Will it make more sense, if I add a separate lspcon_detect function, and call it from HDMI/DP detect, and in that function take this call of reading EDID or DPCD based on lspcon_mode ? Please suggest. Maybe I need to rethink my approach to DDI > encoders, and try to come up with something sane. For LSPCON at least > we want to keep track of the current mode in pipe_config most likely. > For LSPCON it's maybe a bit easier since the mode won't affect the > DDC stuff and whanot, so from the probe side there is just one role > except perhaps w.r.t. DPCD. For regular DDI stuff I'm still thinking > two encoder might be the most sensible apporach since we have totally > different paths for probe as well. > > Probably the biggest kink in all of this is hpd handling. What, if > anything, should hpd_pulse do for LSPCON? And if something, should > it only do it in PCON mode? How is HPD routed anyway with LSPCON? > Right now, from this patch series onwards, I am keeping lspcon always in PCON mode, so it will always be acting as a DP device, so any long HPD sequence which is valid for DP, will be valid for lspcon. May be I should block short pulse handling if lspcon is active on the port ? > [1] https://patchwork.freedesktop.org/series/1596/ > >> + >> if (type == INTEL_OUTPUT_HDMI) >> return intel_hdmi_compute_config(encoder, pipe_config); >> else >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index aa9c59e..39ce16e 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -4276,6 +4276,18 @@ intel_dp_detect(struct drm_connector *connector, bool force) >> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", >> connector->base.id, connector->name); >> >> + /* >> + * LSPCON is a DP->HDMI converter which should be detected as >> + * HDMI in LS mode, and DP in PCON mode. So if LSPCON is in LS >> + * mode, do not try to read DPCD, but detect as HDMI. >> + */ >> + if (is_lspcon_active(intel_dig_port)) { >> + struct intel_lspcon *lspcon = &intel_dig_port->lspcon; >> + >> + if (lspcon->mode_of_op == DRM_LSPCON_MODE_LS) >> + return lspcon_ls_mode_detect(connector, force); >> + } >> + >> if (intel_dp->is_mst) { >> /* MST devices are disconnected from a monitor POV */ >> intel_dp_unset_edid(intel_dp); >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 205a463..fa77886 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -1452,6 +1452,8 @@ struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder); >> bool intel_hdmi_compute_config(struct intel_encoder *encoder, >> struct intel_crtc_state *pipe_config); >> void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable); >> +enum drm_connector_status >> +intel_hdmi_detect(struct drm_connector *connector, bool force); >> >> /* intel_lvds.c */ >> void intel_lvds_init(struct drm_device *dev); >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >> index 6b52c6a..79184e2 100644 >> --- a/drivers/gpu/drm/i915/intel_hdmi.c >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >> @@ -1444,15 +1444,21 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force) >> { >> struct drm_i915_private *dev_priv = to_i915(connector->dev); >> struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); >> + struct intel_digital_port *dig_port = hdmi_to_dig_port(intel_hdmi); >> + struct i2c_adapter *adapter; >> struct edid *edid = NULL; >> bool connected = false; >> >> if (force) { >> intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); >> >> - edid = drm_get_edid(connector, >> - intel_gmbus_get_adapter(dev_priv, >> - intel_hdmi->ddc_bus)); >> + if (is_lspcon_active(dig_port)) >> + adapter = &dig_port->lspcon.aux->ddc; >> + else >> + adapter = intel_gmbus_get_adapter(dev_priv, >> + intel_hdmi->ddc_bus); >> + >> + edid = drm_get_edid(connector, adapter); > > I'm not a fan of this. This is even taking the wrong power domain > now, as does intel_hdmi_detect(). Probably LSPCON should just have > a dedicated codepath for this stuff. > Got it, I will add-in a new function for lspcon's edid detection. And thanks for pointing out this power domain stuff. > If we ever have a board design with a DP++ port without a GMBUS > connection, then we might have rethink things because then we'd have to > use AUX also for HDMI DDC. But so far I'm not aware of such board > designs existing, so might as well keep LSPCON separate. > Yes, and also, lspcon will be valid only for gen9/9.5 devices, until we get HDMI 2.0 native port. So I think we can call it ok to go. >> >> intel_hdmi_dp_dual_mode_detect(connector, edid != NULL); >> >> @@ -1479,7 +1485,7 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force) >> return connected; >> } >> >> -static enum drm_connector_status >> +enum drm_connector_status >> intel_hdmi_detect(struct drm_connector *connector, bool force) >> { >> enum drm_connector_status status; >> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c >> index dd50491..75b5028 100644 >> --- a/drivers/gpu/drm/i915/intel_lspcon.c >> +++ b/drivers/gpu/drm/i915/intel_lspcon.c >> @@ -37,6 +37,12 @@ bool is_lspcon_active(struct intel_digital_port *dig_port) >> return dig_port->lspcon.active; >> } >> >> +enum drm_connector_status >> +lspcon_ls_mode_detect(struct drm_connector *connector, bool force) >> +{ >> + return intel_hdmi_detect(connector, force); >> +} >> + >> enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon) >> { >> enum drm_lspcon_mode current_mode; >> -- >> 1.9.1 >
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index c454744..811c829 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2243,12 +2243,26 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder, { int type = encoder->type; int port = intel_ddi_get_encoder_port(encoder); + struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n"); if (port == PORT_A) pipe_config->cpu_transcoder = TRANSCODER_EDP; + /* + * A digital port with active lspcon device, should be detected + * as HDMI when in LS mode and as DP when in PCON mode. + */ + if (is_lspcon_active(dig_port)) { + struct intel_lspcon *lspcon = &dig_port->lspcon; + + if (lspcon->mode_of_op == DRM_LSPCON_MODE_LS) + type = INTEL_OUTPUT_HDMI; + else + type = INTEL_OUTPUT_DISPLAYPORT; + } + if (type == INTEL_OUTPUT_HDMI) return intel_hdmi_compute_config(encoder, pipe_config); else diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index aa9c59e..39ce16e 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4276,6 +4276,18 @@ intel_dp_detect(struct drm_connector *connector, bool force) DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, connector->name); + /* + * LSPCON is a DP->HDMI converter which should be detected as + * HDMI in LS mode, and DP in PCON mode. So if LSPCON is in LS + * mode, do not try to read DPCD, but detect as HDMI. + */ + if (is_lspcon_active(intel_dig_port)) { + struct intel_lspcon *lspcon = &intel_dig_port->lspcon; + + if (lspcon->mode_of_op == DRM_LSPCON_MODE_LS) + return lspcon_ls_mode_detect(connector, force); + } + if (intel_dp->is_mst) { /* MST devices are disconnected from a monitor POV */ intel_dp_unset_edid(intel_dp); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 205a463..fa77886 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1452,6 +1452,8 @@ struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder); bool intel_hdmi_compute_config(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config); void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable); +enum drm_connector_status +intel_hdmi_detect(struct drm_connector *connector, bool force); /* intel_lvds.c */ void intel_lvds_init(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 6b52c6a..79184e2 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1444,15 +1444,21 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force) { struct drm_i915_private *dev_priv = to_i915(connector->dev); struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); + struct intel_digital_port *dig_port = hdmi_to_dig_port(intel_hdmi); + struct i2c_adapter *adapter; struct edid *edid = NULL; bool connected = false; if (force) { intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); - edid = drm_get_edid(connector, - intel_gmbus_get_adapter(dev_priv, - intel_hdmi->ddc_bus)); + if (is_lspcon_active(dig_port)) + adapter = &dig_port->lspcon.aux->ddc; + else + adapter = intel_gmbus_get_adapter(dev_priv, + intel_hdmi->ddc_bus); + + edid = drm_get_edid(connector, adapter); intel_hdmi_dp_dual_mode_detect(connector, edid != NULL); @@ -1479,7 +1485,7 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force) return connected; } -static enum drm_connector_status +enum drm_connector_status intel_hdmi_detect(struct drm_connector *connector, bool force) { enum drm_connector_status status; diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c index dd50491..75b5028 100644 --- a/drivers/gpu/drm/i915/intel_lspcon.c +++ b/drivers/gpu/drm/i915/intel_lspcon.c @@ -37,6 +37,12 @@ bool is_lspcon_active(struct intel_digital_port *dig_port) return dig_port->lspcon.active; } +enum drm_connector_status +lspcon_ls_mode_detect(struct drm_connector *connector, bool force) +{ + return intel_hdmi_detect(connector, force); +} + enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon) { enum drm_lspcon_mode current_mode;
lspcon is a tricky device to detect. When in LS mode: It should be detected as a HDMI device, by reading EDID, on I2C over Aux chanel When in PCON mode: It should be detected as a DP device by reading DPCD over the Aux channel. This patch accommodates these specific requirements of lspcon detection by adding appropriate changes in I915 drivers HDMI/DP detection sequence. Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 14 ++++++++++++++ drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 2 ++ drivers/gpu/drm/i915/intel_hdmi.c | 14 ++++++++++---- drivers/gpu/drm/i915/intel_lspcon.c | 6 ++++++ 5 files changed, 44 insertions(+), 4 deletions(-)