| Message ID | 20220912111814.17466-14-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Some house cleaning | expand |
On Mon, 12 Sep 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > In the unlikely case of not finding a fixed mode don't register > the eDP connector. I think there are some places where we'd oops > if we didn't have a fixed mode for eDP so presumable this doesn't > typically happen. But better safe than sorry. I think this is fine as the first step. ISTR there are provisions in the DP spec for adding some default mode if all else fails, maybe we should look into adding something like that? Guaranteeing we always have a fixed mode for eDP opens up possibilities for some further cleanup if we want. We have some "is edp and fixed mode" style conditions. Up to and including this patch in the series, Reviewed-by: Jani Nikula <jani.nikula@intel.com> > > Also pimp the debugs with the encoder id+name. I think dumping > the encoder rather than the connector provides more information > here (eg. to match again the port information in the VBT). > > We can also drop the extra check from intel_edp_add_properties(). > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 7b4ffb74c94c..8fe48634eb9d 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -5197,9 +5197,6 @@ intel_edp_add_properties(struct intel_dp *intel_dp) > > intel_attach_scaling_mode_property(&connector->base); > > - if (!fixed_mode) > - return; > - > drm_connector_set_panel_orientation_with_quirk(&connector->base, > i915->display.vbt.orientation, > fixed_mode->hdisplay, > @@ -5272,7 +5269,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > if (!has_dpcd) { > /* if this fails, presume the device is a ghost */ > drm_info(&dev_priv->drm, > - "failed to retrieve link info, disabling eDP\n"); > + "[ENCODER:%d:%s] failed to retrieve link info, disabling eDP\n", > + encoder->base.base.id, encoder->base.name); > goto out_vdd_off; > } > > @@ -5318,6 +5316,13 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > > mutex_unlock(&dev->mode_config.mutex); > > + if (!intel_panel_preferred_fixed_mode(intel_connector)) { > + drm_info(&dev_priv->drm, > + "[ENCODER:%d:%s] failed to find fixed mode for the panel, disabling eDP\n", > + encoder->base.base.id, encoder->base.name); > + goto out_vdd_off; > + } > + > intel_panel_init(intel_connector); > > intel_edp_backlight_setup(intel_dp, intel_connector);
On Mon, Sep 12, 2022 at 03:02:36PM +0300, Jani Nikula wrote: > On Mon, 12 Sep 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > In the unlikely case of not finding a fixed mode don't register > > the eDP connector. I think there are some places where we'd oops > > if we didn't have a fixed mode for eDP so presumable this doesn't > > typically happen. But better safe than sorry. > > I think this is fine as the first step. ISTR there are provisions in the > DP spec for adding some default mode if all else fails, maybe we should > look into adding something like that? I have no idea if eDP panels would support even the 640x480 fallback mode. My hunch is no, but might be interesting to test if someone is willing to risk their eDP panel... I have a feeling we should also just remove the current VBT fixed mode fallback. I think the Windows driver has no such thing, though should probably double check that. > > Guaranteeing we always have a fixed mode for eDP opens up possibilities > for some further cleanup if we want. We have some "is edp and fixed > mode" style conditions. Yeah. Though I'm not sure if we'd want to go with if (edp) or if (fixed_mode) in those cases. I have occasionally pondered about exposing a user configurable fixed_mode for external displays as well. Would let users run their external display with a specific mode and use the pfit to scale things instead of relying on the display having a decent scaler. > > Up to and including this patch in the series, I think I'll skip it for now. > > Reviewed-by: Jani Nikula <jani.nikula@intel.com> Thanks. > > > > > > Also pimp the debugs with the encoder id+name. I think dumping > > the encoder rather than the connector provides more information > > here (eg. to match again the port information in the VBT). > > > > We can also drop the extra check from intel_edp_add_properties(). > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp.c | 13 +++++++++---- > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > index 7b4ffb74c94c..8fe48634eb9d 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -5197,9 +5197,6 @@ intel_edp_add_properties(struct intel_dp *intel_dp) > > > > intel_attach_scaling_mode_property(&connector->base); > > > > - if (!fixed_mode) > > - return; > > - > > drm_connector_set_panel_orientation_with_quirk(&connector->base, > > i915->display.vbt.orientation, > > fixed_mode->hdisplay, > > @@ -5272,7 +5269,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > > if (!has_dpcd) { > > /* if this fails, presume the device is a ghost */ > > drm_info(&dev_priv->drm, > > - "failed to retrieve link info, disabling eDP\n"); > > + "[ENCODER:%d:%s] failed to retrieve link info, disabling eDP\n", > > + encoder->base.base.id, encoder->base.name); > > goto out_vdd_off; > > } > > > > @@ -5318,6 +5316,13 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > > > > mutex_unlock(&dev->mode_config.mutex); > > > > + if (!intel_panel_preferred_fixed_mode(intel_connector)) { > > + drm_info(&dev_priv->drm, > > + "[ENCODER:%d:%s] failed to find fixed mode for the panel, disabling eDP\n", > > + encoder->base.base.id, encoder->base.name); > > + goto out_vdd_off; > > + } > > + > > intel_panel_init(intel_connector); > > > > intel_edp_backlight_setup(intel_dp, intel_connector); > > -- > Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 7b4ffb74c94c..8fe48634eb9d 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -5197,9 +5197,6 @@ intel_edp_add_properties(struct intel_dp *intel_dp) intel_attach_scaling_mode_property(&connector->base); - if (!fixed_mode) - return; - drm_connector_set_panel_orientation_with_quirk(&connector->base, i915->display.vbt.orientation, fixed_mode->hdisplay, @@ -5272,7 +5269,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, if (!has_dpcd) { /* if this fails, presume the device is a ghost */ drm_info(&dev_priv->drm, - "failed to retrieve link info, disabling eDP\n"); + "[ENCODER:%d:%s] failed to retrieve link info, disabling eDP\n", + encoder->base.base.id, encoder->base.name); goto out_vdd_off; } @@ -5318,6 +5316,13 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, mutex_unlock(&dev->mode_config.mutex); + if (!intel_panel_preferred_fixed_mode(intel_connector)) { + drm_info(&dev_priv->drm, + "[ENCODER:%d:%s] failed to find fixed mode for the panel, disabling eDP\n", + encoder->base.base.id, encoder->base.name); + goto out_vdd_off; + } + intel_panel_init(intel_connector); intel_edp_backlight_setup(intel_dp, intel_connector);