| Message ID | 20190603214940.11996-1-matthew.s.atwood@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dp: Correctly advertise HBR3 for GEN11+ | expand |
On Mon, Jun 03, 2019 at 02:49:40PM -0700, matthew.s.atwood@intel.com wrote: > From: Matt Atwood <matthew.s.atwood@intel.com> > > intel_dp_set_source_rates() calls intel_dp_is_edp(), which is unsafe to > use before encoder_type is set. This caused GEN11+ to incorrectly strip > HBR3 from source rates. Move intel_dp_set_source_rates() to after > encoder_type is set. Add comment to intel_dp_is_edp() describing unsafe > usages. May be also add a WARN_ON inside intel_dp_is_edp() for encoder->type still set to INTEL_OUTPUT_DDI > > Fixes: b265a2a6255f5 ("drm/i915/icl: combo port vswing programming > changes per BSPEC") > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 24b56b2a76c8..a4490bcad684 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -141,6 +141,8 @@ static const u8 valid_dsc_slicecount[] = {1, 2, 4}; > * > * If a CPU or PCH DP output is attached to an eDP panel, this function > * will return true, and false otherwise. > + * > + * This function is not safe to use prior to encoder type being set. > */ > bool intel_dp_is_edp(struct intel_dp *intel_dp) IMHO, this comment and WARN_ON like I mentioned above should be a separate patch since its just a cleanup and no functional change. Manasi > { > @@ -7342,8 +7344,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > intel_dig_port->max_lanes, port_name(port))) > return false; > > - intel_dp_set_source_rates(intel_dp); > - > intel_dp->reset_link_params = true; > intel_dp->pps_pipe = INVALID_PIPE; > intel_dp->active_pipe = INVALID_PIPE; > @@ -7388,6 +7388,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > type == DRM_MODE_CONNECTOR_eDP ? "eDP" : "DP", > port_name(port)); > > + intel_dp_set_source_rates(intel_dp); > + > drm_connector_init(dev, connector, &intel_dp_connector_funcs, type); > drm_connector_helper_add(connector, &intel_dp_connector_helper_funcs); > > -- > 2.17.2 >
On Mon, Jun 03, 2019 at 02:49:40PM -0700, matthew.s.atwood@intel.com wrote: > From: Matt Atwood <matthew.s.atwood@intel.com> > > intel_dp_set_source_rates() calls intel_dp_is_edp(), which is unsafe to > use before encoder_type is set. This caused GEN11+ to incorrectly strip > HBR3 from source rates. Move intel_dp_set_source_rates() to after > encoder_type is set. Add comment to intel_dp_is_edp() describing unsafe > usages. > > Fixes: b265a2a6255f5 ("drm/i915/icl: combo port vswing programming > changes per BSPEC") > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 24b56b2a76c8..a4490bcad684 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -141,6 +141,8 @@ static const u8 valid_dsc_slicecount[] = {1, 2, 4}; > * > * If a CPU or PCH DP output is attached to an eDP panel, this function > * will return true, and false otherwise. > + * > + * This function is not safe to use prior to encoder type being set. > */ > bool intel_dp_is_edp(struct intel_dp *intel_dp) > { > @@ -7342,8 +7344,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > intel_dig_port->max_lanes, port_name(port))) > return false; > > - intel_dp_set_source_rates(intel_dp); > - > intel_dp->reset_link_params = true; > intel_dp->pps_pipe = INVALID_PIPE; > intel_dp->active_pipe = INVALID_PIPE; > @@ -7388,6 +7388,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > type == DRM_MODE_CONNECTOR_eDP ? "eDP" : "DP", > port_name(port)); > > + intel_dp_set_source_rates(intel_dp); > + I would suggest moving the encoder->type setup earlier so that we might avoid hitting this same problem in the future. > drm_connector_init(dev, connector, &intel_dp_connector_funcs, type); > drm_connector_helper_add(connector, &intel_dp_connector_helper_funcs); > > -- > 2.17.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Jun 04, 2019 at 03:51:58PM +0300, Ville Syrjälä wrote: > On Mon, Jun 03, 2019 at 02:49:40PM -0700, matthew.s.atwood@intel.com wrote: > > From: Matt Atwood <matthew.s.atwood@intel.com> > > > > intel_dp_set_source_rates() calls intel_dp_is_edp(), which is unsafe to > > use before encoder_type is set. This caused GEN11+ to incorrectly strip > > HBR3 from source rates. Move intel_dp_set_source_rates() to after > > encoder_type is set. Add comment to intel_dp_is_edp() describing unsafe > > usages. > > > > Fixes: b265a2a6255f5 ("drm/i915/icl: combo port vswing programming > > changes per BSPEC") > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 24b56b2a76c8..a4490bcad684 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -141,6 +141,8 @@ static const u8 valid_dsc_slicecount[] = {1, 2, 4}; > > * > > * If a CPU or PCH DP output is attached to an eDP panel, this function > > * will return true, and false otherwise. > > + * > > + * This function is not safe to use prior to encoder type being set. > > */ > > bool intel_dp_is_edp(struct intel_dp *intel_dp) > > { > > @@ -7342,8 +7344,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > > intel_dig_port->max_lanes, port_name(port))) > > return false; > > > > - intel_dp_set_source_rates(intel_dp); > > - > > intel_dp->reset_link_params = true; > > intel_dp->pps_pipe = INVALID_PIPE; > > intel_dp->active_pipe = INVALID_PIPE; > > @@ -7388,6 +7388,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > > type == DRM_MODE_CONNECTOR_eDP ? "eDP" : "DP", > > port_name(port)); > > > > + intel_dp_set_source_rates(intel_dp); > > + > > I would suggest moving the encoder->type setup earlier so that we might > avoid hitting this same problem in the future. Oh and while you're in there would be nice if you could fix up the comment that still refers to INTEL_OUTPUT_UNKNOWN. > > > drm_connector_init(dev, connector, &intel_dp_connector_funcs, type); > > drm_connector_helper_add(connector, &intel_dp_connector_helper_funcs); > > > > -- > > 2.17.2 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel
On Tue, Jun 04, 2019 at 03:51:58PM +0300, Ville Syrjälä wrote: > On Mon, Jun 03, 2019 at 02:49:40PM -0700, matthew.s.atwood@intel.com wrote: > > From: Matt Atwood <matthew.s.atwood@intel.com> > > > > intel_dp_set_source_rates() calls intel_dp_is_edp(), which is unsafe to > > use before encoder_type is set. This caused GEN11+ to incorrectly strip > > HBR3 from source rates. Move intel_dp_set_source_rates() to after > > encoder_type is set. Add comment to intel_dp_is_edp() describing unsafe > > usages. > > > > Fixes: b265a2a6255f5 ("drm/i915/icl: combo port vswing programming > > changes per BSPEC") > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 24b56b2a76c8..a4490bcad684 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -141,6 +141,8 @@ static const u8 valid_dsc_slicecount[] = {1, 2, 4}; > > * > > * If a CPU or PCH DP output is attached to an eDP panel, this function > > * will return true, and false otherwise. > > + * > > + * This function is not safe to use prior to encoder type being set. > > */ > > bool intel_dp_is_edp(struct intel_dp *intel_dp) > > { > > @@ -7342,8 +7344,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > > intel_dig_port->max_lanes, port_name(port))) > > return false; > > > > - intel_dp_set_source_rates(intel_dp); > > - > > intel_dp->reset_link_params = true; > > intel_dp->pps_pipe = INVALID_PIPE; > > intel_dp->active_pipe = INVALID_PIPE; > > @@ -7388,6 +7388,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > > type == DRM_MODE_CONNECTOR_eDP ? "eDP" : "DP", > > port_name(port)); > > > > + intel_dp_set_source_rates(intel_dp); > > + > > I would suggest moving the encoder->type setup earlier so that we might > avoid hitting this same problem in the future. IMHO, Ideally this encoder->type should be set in intel_ddi_init or intel_ddi_init_dp_connector() for us to never get into a situation where intel_dp_is_edp() is called before encoder->type is set. Is that what you are recommending? Manasi > > > drm_connector_init(dev, connector, &intel_dp_connector_funcs, type); > > drm_connector_helper_add(connector, &intel_dp_connector_helper_funcs); > > > > -- > > 2.17.2 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Jun 04, 2019 at 12:44:48PM -0700, Manasi Navare wrote: > On Tue, Jun 04, 2019 at 03:51:58PM +0300, Ville Syrjälä wrote: > > On Mon, Jun 03, 2019 at 02:49:40PM -0700, matthew.s.atwood@intel.com wrote: > > > From: Matt Atwood <matthew.s.atwood@intel.com> > > > > > > intel_dp_set_source_rates() calls intel_dp_is_edp(), which is unsafe to > > > use before encoder_type is set. This caused GEN11+ to incorrectly strip > > > HBR3 from source rates. Move intel_dp_set_source_rates() to after > > > encoder_type is set. Add comment to intel_dp_is_edp() describing unsafe > > > usages. > > > > > > Fixes: b265a2a6255f5 ("drm/i915/icl: combo port vswing programming > > > changes per BSPEC") > > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > index 24b56b2a76c8..a4490bcad684 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -141,6 +141,8 @@ static const u8 valid_dsc_slicecount[] = {1, 2, 4}; > > > * > > > * If a CPU or PCH DP output is attached to an eDP panel, this function > > > * will return true, and false otherwise. > > > + * > > > + * This function is not safe to use prior to encoder type being set. > > > */ > > > bool intel_dp_is_edp(struct intel_dp *intel_dp) > > > { > > > @@ -7342,8 +7344,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > > > intel_dig_port->max_lanes, port_name(port))) > > > return false; > > > > > > - intel_dp_set_source_rates(intel_dp); > > > - > > > intel_dp->reset_link_params = true; > > > intel_dp->pps_pipe = INVALID_PIPE; > > > intel_dp->active_pipe = INVALID_PIPE; > > > @@ -7388,6 +7388,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > > > type == DRM_MODE_CONNECTOR_eDP ? "eDP" : "DP", > > > port_name(port)); > > > > > > + intel_dp_set_source_rates(intel_dp); > > > + > > > > I would suggest moving the encoder->type setup earlier so that we might > > avoid hitting this same problem in the future. > > IMHO, Ideally this encoder->type should be set in intel_ddi_init or intel_ddi_init_dp_connector() > for us to never get into a situation where intel_dp_is_edp() is called before encoder->type is set. > > Is that what you are recommending? I was thinking just moving it to the start of intel_dp_init_connector(), but I suppose moving it even earlier would be possible since intel_ddi.c doesn't seem to do any of the eDP->HDMI fallback we needed to add for VLV/CHV. > > Manasi > > > > > > drm_connector_init(dev, connector, &intel_dp_connector_funcs, type); > > > drm_connector_helper_add(connector, &intel_dp_connector_helper_funcs); > > > > > > -- > > > 2.17.2 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Tested-by: Clint Taylor <Clinton.A.Taylor@intel.com> -Clint On 6/3/19 2:49 PM, matthew.s.atwood@intel.com wrote: > From: Matt Atwood <matthew.s.atwood@intel.com> > > intel_dp_set_source_rates() calls intel_dp_is_edp(), which is unsafe to > use before encoder_type is set. This caused GEN11+ to incorrectly strip > HBR3 from source rates. Move intel_dp_set_source_rates() to after > encoder_type is set. Add comment to intel_dp_is_edp() describing unsafe > usages. > > Fixes: b265a2a6255f5 ("drm/i915/icl: combo port vswing programming > changes per BSPEC") > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 24b56b2a76c8..a4490bcad684 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -141,6 +141,8 @@ static const u8 valid_dsc_slicecount[] = {1, 2, 4}; > * > * If a CPU or PCH DP output is attached to an eDP panel, this function > * will return true, and false otherwise. > + * > + * This function is not safe to use prior to encoder type being set. > */ > bool intel_dp_is_edp(struct intel_dp *intel_dp) > { > @@ -7342,8 +7344,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > intel_dig_port->max_lanes, port_name(port))) > return false; > > - intel_dp_set_source_rates(intel_dp); > - > intel_dp->reset_link_params = true; > intel_dp->pps_pipe = INVALID_PIPE; > intel_dp->active_pipe = INVALID_PIPE; > @@ -7388,6 +7388,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > type == DRM_MODE_CONNECTOR_eDP ? "eDP" : "DP", > port_name(port)); > > + intel_dp_set_source_rates(intel_dp); > + > drm_connector_init(dev, connector, &intel_dp_connector_funcs, type); > drm_connector_helper_add(connector, &intel_dp_connector_helper_funcs); >
On Tue, 2019-06-04 at 23:16 +0300, Ville Syrjälä wrote: > On Tue, Jun 04, 2019 at 12:44:48PM -0700, Manasi Navare wrote: > > On Tue, Jun 04, 2019 at 03:51:58PM +0300, Ville Syrjälä wrote: > > > On Mon, Jun 03, 2019 at 02:49:40PM -0700, > > > matthew.s.atwood@intel.com wrote: > > > > From: Matt Atwood <matthew.s.atwood@intel.com> > > > > > > > > intel_dp_set_source_rates() calls intel_dp_is_edp(), which is > > > > unsafe to > > > > use before encoder_type is set. This caused GEN11+ to > > > > incorrectly strip > > > > HBR3 from source rates. Move intel_dp_set_source_rates() to > > > > after > > > > encoder_type is set. Add comment to intel_dp_is_edp() > > > > describing unsafe > > > > usages. > > > > > > > > Fixes: b265a2a6255f5 ("drm/i915/icl: combo port vswing > > > > programming > > > > changes per BSPEC") > > > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++-- > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > index 24b56b2a76c8..a4490bcad684 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -141,6 +141,8 @@ static const u8 valid_dsc_slicecount[] = > > > > {1, 2, 4}; > > > > * > > > > * If a CPU or PCH DP output is attached to an eDP panel, this > > > > function > > > > * will return true, and false otherwise. > > > > + * > > > > + * This function is not safe to use prior to encoder type > > > > being set. > > > > */ > > > > bool intel_dp_is_edp(struct intel_dp *intel_dp) > > > > { > > > > @@ -7342,8 +7344,6 @@ intel_dp_init_connector(struct > > > > intel_digital_port *intel_dig_port, > > > > intel_dig_port->max_lanes, port_name(port))) > > > > return false; > > > > > > > > - intel_dp_set_source_rates(intel_dp); > > > > - > > > > intel_dp->reset_link_params = true; > > > > intel_dp->pps_pipe = INVALID_PIPE; > > > > intel_dp->active_pipe = INVALID_PIPE; > > > > @@ -7388,6 +7388,8 @@ intel_dp_init_connector(struct > > > > intel_digital_port *intel_dig_port, > > > > type == DRM_MODE_CONNECTOR_eDP ? "eDP" > > > > : "DP", > > > > port_name(port)); > > > > > > > > + intel_dp_set_source_rates(intel_dp); > > > > + > > > > > > I would suggest moving the encoder->type setup earlier so that we > > > might > > > avoid hitting this same problem in the future. > > > > IMHO, Ideally this encoder->type should be set in intel_ddi_init or > > intel_ddi_init_dp_connector() > > for us to never get into a situation where intel_dp_is_edp() is > > called before encoder->type is set. > > > > Is that what you are recommending? > > I was thinking just moving it to the start of > intel_dp_init_connector(), > but I suppose moving it even earlier would be possible since > intel_ddi.c > doesn't seem to do any of the eDP->HDMI fallback we needed to add for > VLV/CHV. ack > > > > > Manasi > > > > > > > > > drm_connector_init(dev, connector, > > > > &intel_dp_connector_funcs, type); > > > > drm_connector_helper_add(connector, > > > > &intel_dp_connector_helper_funcs); > > > > > > > > -- > > > > 2.17.2 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > -- > > > Ville Syrjälä > > > Intel > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 24b56b2a76c8..a4490bcad684 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -141,6 +141,8 @@ static const u8 valid_dsc_slicecount[] = {1, 2, 4}; * * If a CPU or PCH DP output is attached to an eDP panel, this function * will return true, and false otherwise. + * + * This function is not safe to use prior to encoder type being set. */ bool intel_dp_is_edp(struct intel_dp *intel_dp) { @@ -7342,8 +7344,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, intel_dig_port->max_lanes, port_name(port))) return false; - intel_dp_set_source_rates(intel_dp); - intel_dp->reset_link_params = true; intel_dp->pps_pipe = INVALID_PIPE; intel_dp->active_pipe = INVALID_PIPE; @@ -7388,6 +7388,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, type == DRM_MODE_CONNECTOR_eDP ? "eDP" : "DP", port_name(port)); + intel_dp_set_source_rates(intel_dp); + drm_connector_init(dev, connector, &intel_dp_connector_funcs, type); drm_connector_helper_add(connector, &intel_dp_connector_helper_funcs);