Re: [PATCH v2 3/3] drm/i915: Send hotplug event if edid had changed.

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Fri, 2019-06-28 at 09:54 +0530, Ramalingam C wrote:
> On 2019-06-28 at 11:24:54 +0300, Stanislav Lisovskiy wrote:
> > Added edid checking to dp and hdmi edid setting functions, which
> > are called from detect hooks. The result currently is propagated
> > to calling layer using drm_connector->change_counter(proposed by
> > Daniel Vetter).
> > drm_helper_hpd_irq_event and intel_encoder_hotplug are currently
> > both
> > responsible for checking if this counter or connection status is
> > changed.
> > 
> > There are conflicting parts in drm and i915 which attempt
> > to do the same job - drm_helper_hpd_irq_event attempts to
> > check connector status changes and then call hotplug,
> > just as i915_hotplug_work_func, which calls encoder->hotplug
> > hook which in turn calls generic intel_encoder_hotplug function
> > which attempts to probe if output has changed.
> > Looks like both needs to be changed, so added edid checking
> > also to intel_encoder_hotplug function which is called both
> > for hdmi and dp.
> > 
> > v2: Renamed change counter to epoch counter. Fixed type name.
> > 
> > 

> > @@ -2524,11 +2524,22 @@ intel_hdmi_set_edid(struct drm_connector
> > *connector)
> >  
> >  	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS, wakeref);
> >  
> > +	old_edid = to_intel_connector(connector)->detect_edid;
> > +
> > +	if (!drm_edid_are_equal(edid, old_edid)) {
> > +		intel_connector_update_modes(connector, edid);
> > +		DRM_DEBUG_KMS("Updating change counter to %llu\n",
> > connector->epoch_counter);
> > +		connector->epoch_counter += 1;
> > +
> > +		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Edid was changed!
> > Updating blob property.\n",
> > +		    connector->base.id, connector->name);
> > +	}
> > +	intel_hdmi_unset_edid(connector);
> >  	to_intel_connector(connector)->detect_edid = edid;
> > +

> 
> This and next changes are unrelated with this commit. Might want to
> keep
> it for separate patch.

What do you mean by unrelated? I thought dependent changes should go in
one patch series and here we are taking into use epoch_counter for
i915, which was introduced in previous 2 separate drm patches from that
series.

The drm changes should obviously always go first here, otherwise this
patch will fail - I wouldn't even be able to send it for testing if
that would be in another series.

> 
> -Ram
> >  	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
> >  		intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
> >  		intel_hdmi->has_hdmi_sink =
> > drm_detect_hdmi_monitor(edid);
> > -
> >  		connected = true;
> >  	}
> >  
> > @@ -2555,7 +2566,6 @@ intel_hdmi_detect(struct drm_connector
> > *connector, bool force)
> >  	    !intel_digital_port_connected(encoder))
> >  		goto out;
> >  
> > -	intel_hdmi_unset_edid(connector);
> >  
> >  	if (intel_hdmi_set_edid(connector))
> >  		status = connector_status_connected;
> > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c
> > b/drivers/gpu/drm/i915/display/intel_hotplug.c
> > index ea3de4acc850..f9d9e963196a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hotplug.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
> > @@ -271,23 +271,33 @@ bool intel_encoder_hotplug(struct
> > intel_encoder *encoder,
> >  {
> >  	struct drm_device *dev = connector->base.dev;
> >  	enum drm_connector_status old_status;
> > +	u64 old_epoch_counter;
> > +	bool ret = false;
> >  
> >  	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> >  	old_status = connector->base.status;
> >  
> > +	old_epoch_counter = connector->base.epoch_counter;
> > +
> >  	connector->base.status =
> >  		drm_helper_probe_detect(&connector->base, NULL, false);
> >  
> > -	if (old_status == connector->base.status)
> > -		return false;
> > +	if (old_epoch_counter != connector->base.epoch_counter)
> > +		ret = true;
> >  
> > -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to
> > %s\n",
> > +	if (old_status != connector->base.status)
> > +		ret = true;
> > +
> > +	if (ret) {
> > +		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s
> > to %s(change counter %llu)\n",
> >  		      connector->base.base.id,
> >  		      connector->base.name,
> >  		      drm_get_connector_status_name(old_status),
> > -		      drm_get_connector_status_name(connector-
> > >base.status));
> > +		      drm_get_connector_status_name(connector-
> > >base.status),
> > +		      connector->base.epoch_counter);
> > +	}
> >  
> > -	return true;
> > +	return ret;
> >  }
> >  
> >  static bool intel_encoder_has_hpd_pulse(struct intel_encoder
> > *encoder)
> > -- 
> > 2.17.1
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx