Re: [PATCH v2 07/21] drm/i915/dp: Recheck link state after modeset

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, May 23, 2024 at 04:23:44PM +0300, Ville Syrjälä wrote:
> On Wed, May 22, 2024 at 04:38:54PM +0300, Imre Deak wrote:
> > On Mon, May 20, 2024 at 09:58:05PM +0300, Imre Deak wrote:
> > > [...]
> > > +static void intel_ddi_link_check_work_fn(struct work_struct *work)
> > > +{
> > > +	struct intel_digital_port *dig_port =
> > > +		container_of(work, typeof(*dig_port), check_link_work.work);
> > > +	struct intel_encoder *encoder = &dig_port->base;
> > > +	struct drm_modeset_acquire_ctx ctx;
> > > +	int ret;
> > > +
> > > +	intel_modeset_lock_ctx_retry(&ctx, NULL, 0, ret)
> > > +		if (dig_port->dp.attached_connector)
> > > +			ret = intel_dp_retrain_link(encoder, &ctx);
> > 
> > ret should not be let here uninited making the loop spin forever. This
> > can't actually happen since the work won't get schedule w/o
> > dp.attached_connector, but the above check should be also just an assert
> > at this point (at least until handling HDMI is also added).
> 
> Hmm. Why are we actually initializing ret=-EDEADLK in
> _intel_modeset_lock_begin()?

answer: so that _drm_modeset_lock_loop() actually works

> 
> > 
> > > +}
> > > +
> > > +void intel_ddi_init_link_check_work(struct intel_digital_port *dig_port)
> > > +{
> > > +	INIT_DELAYED_WORK(&dig_port->check_link_work, intel_ddi_link_check_work_fn);
> > > +}
> > > +
> > > +void intel_ddi_flush_link_check_work(struct intel_digital_port *dig_port)
> > > +{
> > > +	cancel_delayed_work_sync(&dig_port->check_link_work);
> > > +}
> > > +
> > > +void intel_ddi_queue_link_check(struct intel_digital_port *dig_port, int delay_ms)
> > > +{
> > > +	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > +
> > > +	mod_delayed_work(i915->unordered_wq,
> > > +			 &dig_port->check_link_work, msecs_to_jiffies(delay_ms));
> > > +}
> > > +
> > >  static int intel_hdmi_reset_link(struct intel_encoder *encoder,
> > >  				 struct drm_modeset_acquire_ctx *ctx)
> > >  {
> > > @@ -4911,6 +4943,8 @@ void intel_ddi_init(struct drm_i915_private *dev_priv,
> > >  
> > >  	dig_port->aux_ch = AUX_CH_NONE;
> > >  
> > > +	intel_ddi_init_link_check_work(dig_port);
> > > +
> > >  	encoder = &dig_port->base;
> > >  	encoder->devdata = devdata;
> > >  
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel