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

[Ramalingam C <ramalingam.c@intel.com>]

On 2019-06-28 at 17:06:09 +0530, Lisovskiy, Stanislav wrote:
> 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.
Meant the new line addition and line removal. just usual suggestions.
you can ignore if you prefer. apart from that changes looks good.

-Ram
> 
> > 
> > -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