dri-devel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH v2 1/3] drm: Add helper to compare edids.
  2019-06-28  8:24 ` [PATCH v2 1/3] drm: Add helper to compare edids Stanislav Lisovskiy
@ 2019-06-28  4:13   ` Ramalingam C
  2019-07-01 19:52   ` Lyude Paul
  1 sibling, 0 replies; 10+ messages in thread
From: Ramalingam C @ 2019-06-28  4:13 UTC (permalink / raw)
  To: Stanislav Lisovskiy
  Cc: simon.ser, daniel.vetter, intel-gfx, martin.peres, dri-devel,
	paul.kocialkowski, jani.saarinen

On 2019-06-28 at 11:24:52 +0300, Stanislav Lisovskiy wrote:
> Many drivers would benefit from using
> drm helper to compare edid, rather
> than bothering with own implementation.
> 
> v2: Added documentation for this function.
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 33 +++++++++++++++++++++++++++++++++
>  include/drm/drm_edid.h     |  9 +++++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 9d8f2b952004..eaad5155fbdd 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1361,6 +1361,39 @@ static bool drm_edid_is_zero(const u8 *in_edid, int length)
>  	return true;
>  }
>  
> +/**
> + * drm_edid_are_equal - compare two edid blobs.
> + * @edid1: pointer to first blob
> + * @edid2: pointer to second blob
extra line here is preferred.
> + * This helper can be used during probing to determine if
> + * edid had changed.
bool is implicit. if you want you can explain the return value.
> + */
> +bool drm_edid_are_equal(struct edid *edid1, struct edid *edid2)
> +{
> +	int edid1_len, edid2_len;
> +	bool edid1_present = edid1 != NULL;
> +	bool edid2_present = edid2 != NULL;
> +
> +	if (edid1_present != edid2_present)
> +		return false;
> +
> +	if (edid1) {
> +
> +		edid1_len = EDID_LENGTH * (1 + edid1->extensions);
> +		edid2_len = EDID_LENGTH * (1 + edid2->extensions);
> +
> +		if (edid1_len != edid2_len)
> +			return false;
> +
> +		if (memcmp(edid1, edid2, edid1_len))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +EXPORT_SYMBOL(drm_edid_are_equal);
> +
> +
>  /**
>   * drm_edid_block_valid - Sanity check the EDID block (base or extension)
>   * @raw_edid: pointer to raw EDID block
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index b9719418c3d2..716964f63312 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -354,6 +354,15 @@ drm_load_edid_firmware(struct drm_connector *connector)
>  }
>  #endif
>  
> +/**
> + * drm_edid_are_equal - compare two edid blobs.
> + * @edid1: pointer to first blob
> + * @edid2: pointer to second blob
> + * This helper can be used during probing to determine if
> + * edid had changed.
> + */
Do we need kdoc for function declaration too!? Should be sufficient for
definition alone.

-Ram
> +bool drm_edid_are_equal(struct edid *edid1, struct edid *edid2);
> +
>  int
>  drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
>  					 struct drm_connector *connector,
> -- 
> 2.17.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/3] drm: Introduce change counter to drm_connector
  2019-06-28  8:24 ` [PATCH v2 2/3] drm: Introduce change counter to drm_connector Stanislav Lisovskiy
@ 2019-06-28  4:16   ` Ramalingam C
  0 siblings, 0 replies; 10+ messages in thread
From: Ramalingam C @ 2019-06-28  4:16 UTC (permalink / raw)
  To: Stanislav Lisovskiy
  Cc: simon.ser, daniel.vetter, intel-gfx, martin.peres, dri-devel,
	paul.kocialkowski, ppaalanen

On 2019-06-28 at 11:24:53 +0300, Stanislav Lisovskiy wrote:
> This counter will be used by drm_helper_probe_detect caller to determine
> if something else had changed except connection status,
> like for example edid. Hardware specific drivers are responsible
> for updating this counter when some change is detected to notify
> the drm part, which can trigger for example hotplug event.
> 
> Currently there is no way to propagate that to a calling layer,
> as we send only connection_status update, however as we see with
> edid the changes can be broader.
> 
> v2: Added documentation for the new counter. Rename change_counter to
>     epoch_counter.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105540
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/drm_connector.c    |  1 +
>  drivers/gpu/drm/drm_probe_helper.c | 29 +++++++++++++++++++++++++++--
>  include/drm/drm_connector.h        |  3 +++
>  3 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 3ccdcf3dfcde..065eee61859e 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -245,6 +245,7 @@ int drm_connector_init(struct drm_device *dev,
>  	INIT_LIST_HEAD(&connector->modes);
>  	mutex_init(&connector->mutex);
>  	connector->edid_blob_ptr = NULL;
> +	connector->epoch_counter = 0;
>  	connector->tile_blob_ptr = NULL;
>  	connector->status = connector_status_unknown;
>  	connector->display_info.panel_orientation =
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index ef2c468205a2..5857053174da 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -776,6 +776,7 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
>  	struct drm_connector_list_iter conn_iter;
>  	enum drm_connector_status old_status;
>  	bool changed = false;
> +	uint64_t old_epoch_counter;
>  
>  	if (!dev->mode_config.poll_enabled)
>  		return false;
> @@ -789,20 +790,44 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
>  
>  		old_status = connector->status;
>  
> +		old_epoch_counter = connector->epoch_counter;
> +
> +		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Old change counter %llu\n", connector->base.id,
> +			      connector->name,
> +			      old_epoch_counter);
> +
>  		connector->status = drm_helper_probe_detect(connector, NULL, false);
>  		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n",
>  			      connector->base.id,
>  			      connector->name,
>  			      drm_get_connector_status_name(old_status),
>  			      drm_get_connector_status_name(connector->status));
> -		if (old_status != connector->status)
> +
> +		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] New change counter %llu\n",
> +			      connector->base.id,
> +			      connector->name,
> +			      connector->epoch_counter);
> +
> +		if (old_status != connector->status) {
{} is not required here.
>  			changed = true;
> +		}
> +
> +		/* Check changing of edid when a connector status still remains
> +		 * as "connector_status_connected".
> +		 */
> +		if (connector->status == connector_status_connected &&
> +		    old_status == connector_status_connected &&
> +		    old_epoch_counter != connector->epoch_counter) {
> +			changed = true;
> +		}
>  	}
>  	drm_connector_list_iter_end(&conn_iter);
>  	mutex_unlock(&dev->mode_config.mutex);
>  
> -	if (changed)
> +	if (changed) {
>  		drm_kms_helper_hotplug_event(dev);
> +		DRM_DEBUG_KMS("Sent hotplug event\n");
> +	}
>  
>  	return changed;
>  }
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index c6f8486d8b8f..a296bdac085f 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1155,6 +1155,9 @@ struct drm_connector {
>  	/** @override_edid: has the EDID been overwritten through debugfs for testing? */
>  	bool override_edid;
>  
> +	/** @epoch_counter: used to detect any other changes in connector, besides status */
Might want to wrap at 80char.

-Ram
> +	uint64_t epoch_counter;
> +
>  #define DRM_CONNECTOR_MAX_ENCODER 3
>  	/**
>  	 * @encoder_ids: Valid encoders for this connector. Please only use
> -- 
> 2.17.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] drm/i915: Send hotplug event if edid had changed.
  2019-06-28  8:24 ` [PATCH v2 3/3] drm/i915: Send hotplug event if edid had changed Stanislav Lisovskiy
@ 2019-06-28  4:24   ` Ramalingam C
  2019-06-28 11:36     ` Lisovskiy, Stanislav
  0 siblings, 1 reply; 10+ messages in thread
From: Ramalingam C @ 2019-06-28  4:24 UTC (permalink / raw)
  To: Stanislav Lisovskiy
  Cc: simon.ser, daniel.vetter, intel-gfx, martin.peres, dri-devel,
	paul.kocialkowski, ppaalanen

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.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105540
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c      | 16 +++++++++++++++-
>  drivers/gpu/drm/i915/display/intel_hdmi.c    | 16 +++++++++++++---
>  drivers/gpu/drm/i915/display/intel_hotplug.c | 20 +++++++++++++++-----
>  3 files changed, 43 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 4336df46fe78..c2ed02478cf9 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5510,10 +5510,24 @@ static void
>  intel_dp_set_edid(struct intel_dp *intel_dp)
>  {
>  	struct intel_connector *intel_connector = intel_dp->attached_connector;
> +	struct drm_connector *connector = &intel_connector->base;
>  	struct edid *edid;
> +	struct edid *old_edid;
>  
> -	intel_dp_unset_edid(intel_dp);
>  	edid = intel_dp_get_edid(intel_dp);
> +	old_edid = intel_connector->detect_edid;
> +
> +	if (!drm_edid_are_equal(edid, old_edid)) {
> +		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Edid was changed! Updating blob property.\n",
> +		    connector->base.id, connector->name);
> +
> +		connector->epoch_counter += 1;
> +		DRM_DEBUG_KMS("Updating change counter to %llu\n", connector->epoch_counter);
> +
> +		intel_connector_update_modes(&intel_connector->base, edid);
> +	}
> +
> +	intel_dp_unset_edid(intel_dp);
>  	intel_connector->detect_edid = edid;
>  
>  	intel_dp->has_audio = drm_detect_monitor_audio(edid);
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 0ebec69bbbfc..f892c7b795ce 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2503,7 +2503,7 @@ intel_hdmi_set_edid(struct drm_connector *connector)
>  	struct drm_i915_private *dev_priv = to_i915(connector->dev);
>  	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
>  	intel_wakeref_t wakeref;
> -	struct edid *edid;
> +	struct edid *edid, *old_edid;
>  	bool connected = false;
>  	struct i2c_adapter *i2c;
>  
> @@ -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.

-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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] drm/i915: Send hotplug event if edid had changed.
  2019-06-28 11:36     ` Lisovskiy, Stanislav
@ 2019-06-28  4:50       ` Ramalingam C
  0 siblings, 0 replies; 10+ messages in thread
From: Ramalingam C @ 2019-06-28  4:50 UTC (permalink / raw)
  To: Lisovskiy, Stanislav
  Cc: Ser, Simon, daniel.vetter@ffwll.ch,
	intel-gfx@lists.freedesktop.org, Peres, Martin,
	dri-devel@lists.freedesktop.org, paul.kocialkowski@bootlin.com,
	ppaalanen@gmail.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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 0/3] Send a hotplug when edid changes
@ 2019-06-28  8:24 Stanislav Lisovskiy
  2019-06-28  8:24 ` [PATCH v2 1/3] drm: Add helper to compare edids Stanislav Lisovskiy
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Stanislav Lisovskiy @ 2019-06-28  8:24 UTC (permalink / raw)
  To: dri-devel
  Cc: paul.kocialkowski, daniel.vetter, intel-gfx, martin.peres,
	ppaalanen, simon.ser

This series introduce to drm a way to determine if something else
except connection_status had changed during probing, which
can be used by other drivers as well. Another i915 specific part
uses this approach to determine if edid had changed without
changing the connection status and send a hotplug event.

Stanislav Lisovskiy (3):
  drm: Add helper to compare edids.
  drm: Introduce change counter to drm_connector
  drm/i915: Send hotplug event if edid had changed.

 drivers/gpu/drm/drm_connector.c              |  1 +
 drivers/gpu/drm/drm_edid.c                   | 33 ++++++++++++++++++++
 drivers/gpu/drm/drm_probe_helper.c           | 29 +++++++++++++++--
 drivers/gpu/drm/i915/display/intel_dp.c      | 16 +++++++++-
 drivers/gpu/drm/i915/display/intel_hdmi.c    | 16 ++++++++--
 drivers/gpu/drm/i915/display/intel_hotplug.c | 20 +++++++++---
 include/drm/drm_connector.h                  |  3 ++
 include/drm/drm_edid.h                       |  9 ++++++
 8 files changed, 116 insertions(+), 11 deletions(-)

-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/3] drm: Add helper to compare edids.
  2019-06-28  8:24 [PATCH v2 0/3] Send a hotplug when edid changes Stanislav Lisovskiy
@ 2019-06-28  8:24 ` Stanislav Lisovskiy
  2019-06-28  4:13   ` Ramalingam C
  2019-07-01 19:52   ` Lyude Paul
  2019-06-28  8:24 ` [PATCH v2 2/3] drm: Introduce change counter to drm_connector Stanislav Lisovskiy
  2019-06-28  8:24 ` [PATCH v2 3/3] drm/i915: Send hotplug event if edid had changed Stanislav Lisovskiy
  2 siblings, 2 replies; 10+ messages in thread
From: Stanislav Lisovskiy @ 2019-06-28  8:24 UTC (permalink / raw)
  To: dri-devel
  Cc: paul.kocialkowski, daniel.vetter, intel-gfx, martin.peres,
	ppaalanen, simon.ser

Many drivers would benefit from using
drm helper to compare edid, rather
than bothering with own implementation.

v2: Added documentation for this function.

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 33 +++++++++++++++++++++++++++++++++
 include/drm/drm_edid.h     |  9 +++++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 9d8f2b952004..eaad5155fbdd 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1361,6 +1361,39 @@ static bool drm_edid_is_zero(const u8 *in_edid, int length)
 	return true;
 }
 
+/**
+ * drm_edid_are_equal - compare two edid blobs.
+ * @edid1: pointer to first blob
+ * @edid2: pointer to second blob
+ * This helper can be used during probing to determine if
+ * edid had changed.
+ */
+bool drm_edid_are_equal(struct edid *edid1, struct edid *edid2)
+{
+	int edid1_len, edid2_len;
+	bool edid1_present = edid1 != NULL;
+	bool edid2_present = edid2 != NULL;
+
+	if (edid1_present != edid2_present)
+		return false;
+
+	if (edid1) {
+
+		edid1_len = EDID_LENGTH * (1 + edid1->extensions);
+		edid2_len = EDID_LENGTH * (1 + edid2->extensions);
+
+		if (edid1_len != edid2_len)
+			return false;
+
+		if (memcmp(edid1, edid2, edid1_len))
+			return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL(drm_edid_are_equal);
+
+
 /**
  * drm_edid_block_valid - Sanity check the EDID block (base or extension)
  * @raw_edid: pointer to raw EDID block
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index b9719418c3d2..716964f63312 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -354,6 +354,15 @@ drm_load_edid_firmware(struct drm_connector *connector)
 }
 #endif
 
+/**
+ * drm_edid_are_equal - compare two edid blobs.
+ * @edid1: pointer to first blob
+ * @edid2: pointer to second blob
+ * This helper can be used during probing to determine if
+ * edid had changed.
+ */
+bool drm_edid_are_equal(struct edid *edid1, struct edid *edid2);
+
 int
 drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
 					 struct drm_connector *connector,
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/3] drm: Introduce change counter to drm_connector
  2019-06-28  8:24 [PATCH v2 0/3] Send a hotplug when edid changes Stanislav Lisovskiy
  2019-06-28  8:24 ` [PATCH v2 1/3] drm: Add helper to compare edids Stanislav Lisovskiy
@ 2019-06-28  8:24 ` Stanislav Lisovskiy
  2019-06-28  4:16   ` Ramalingam C
  2019-06-28  8:24 ` [PATCH v2 3/3] drm/i915: Send hotplug event if edid had changed Stanislav Lisovskiy
  2 siblings, 1 reply; 10+ messages in thread
From: Stanislav Lisovskiy @ 2019-06-28  8:24 UTC (permalink / raw)
  To: dri-devel
  Cc: paul.kocialkowski, daniel.vetter, intel-gfx, martin.peres,
	Stanislav.Lisovskiy, simon.ser, jani.saarinen

This counter will be used by drm_helper_probe_detect caller to determine
if something else had changed except connection status,
like for example edid. Hardware specific drivers are responsible
for updating this counter when some change is detected to notify
the drm part, which can trigger for example hotplug event.

Currently there is no way to propagate that to a calling layer,
as we send only connection_status update, however as we see with
edid the changes can be broader.

v2: Added documentation for the new counter. Rename change_counter to
    epoch_counter.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105540
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/drm_connector.c    |  1 +
 drivers/gpu/drm/drm_probe_helper.c | 29 +++++++++++++++++++++++++++--
 include/drm/drm_connector.h        |  3 +++
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 3ccdcf3dfcde..065eee61859e 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -245,6 +245,7 @@ int drm_connector_init(struct drm_device *dev,
 	INIT_LIST_HEAD(&connector->modes);
 	mutex_init(&connector->mutex);
 	connector->edid_blob_ptr = NULL;
+	connector->epoch_counter = 0;
 	connector->tile_blob_ptr = NULL;
 	connector->status = connector_status_unknown;
 	connector->display_info.panel_orientation =
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index ef2c468205a2..5857053174da 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -776,6 +776,7 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
 	struct drm_connector_list_iter conn_iter;
 	enum drm_connector_status old_status;
 	bool changed = false;
+	uint64_t old_epoch_counter;
 
 	if (!dev->mode_config.poll_enabled)
 		return false;
@@ -789,20 +790,44 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
 
 		old_status = connector->status;
 
+		old_epoch_counter = connector->epoch_counter;
+
+		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Old change counter %llu\n", connector->base.id,
+			      connector->name,
+			      old_epoch_counter);
+
 		connector->status = drm_helper_probe_detect(connector, NULL, false);
 		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n",
 			      connector->base.id,
 			      connector->name,
 			      drm_get_connector_status_name(old_status),
 			      drm_get_connector_status_name(connector->status));
-		if (old_status != connector->status)
+
+		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] New change counter %llu\n",
+			      connector->base.id,
+			      connector->name,
+			      connector->epoch_counter);
+
+		if (old_status != connector->status) {
 			changed = true;
+		}
+
+		/* Check changing of edid when a connector status still remains
+		 * as "connector_status_connected".
+		 */
+		if (connector->status == connector_status_connected &&
+		    old_status == connector_status_connected &&
+		    old_epoch_counter != connector->epoch_counter) {
+			changed = true;
+		}
 	}
 	drm_connector_list_iter_end(&conn_iter);
 	mutex_unlock(&dev->mode_config.mutex);
 
-	if (changed)
+	if (changed) {
 		drm_kms_helper_hotplug_event(dev);
+		DRM_DEBUG_KMS("Sent hotplug event\n");
+	}
 
 	return changed;
 }
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index c6f8486d8b8f..a296bdac085f 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1155,6 +1155,9 @@ struct drm_connector {
 	/** @override_edid: has the EDID been overwritten through debugfs for testing? */
 	bool override_edid;
 
+	/** @epoch_counter: used to detect any other changes in connector, besides status */
+	uint64_t epoch_counter;
+
 #define DRM_CONNECTOR_MAX_ENCODER 3
 	/**
 	 * @encoder_ids: Valid encoders for this connector. Please only use
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 3/3] drm/i915: Send hotplug event if edid had changed.
  2019-06-28  8:24 [PATCH v2 0/3] Send a hotplug when edid changes Stanislav Lisovskiy
  2019-06-28  8:24 ` [PATCH v2 1/3] drm: Add helper to compare edids Stanislav Lisovskiy
  2019-06-28  8:24 ` [PATCH v2 2/3] drm: Introduce change counter to drm_connector Stanislav Lisovskiy
@ 2019-06-28  8:24 ` Stanislav Lisovskiy
  2019-06-28  4:24   ` Ramalingam C
  2 siblings, 1 reply; 10+ messages in thread
From: Stanislav Lisovskiy @ 2019-06-28  8:24 UTC (permalink / raw)
  To: dri-devel
  Cc: paul.kocialkowski, daniel.vetter, intel-gfx, martin.peres,
	ppaalanen, simon.ser

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.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105540
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c      | 16 +++++++++++++++-
 drivers/gpu/drm/i915/display/intel_hdmi.c    | 16 +++++++++++++---
 drivers/gpu/drm/i915/display/intel_hotplug.c | 20 +++++++++++++++-----
 3 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 4336df46fe78..c2ed02478cf9 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5510,10 +5510,24 @@ static void
 intel_dp_set_edid(struct intel_dp *intel_dp)
 {
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
+	struct drm_connector *connector = &intel_connector->base;
 	struct edid *edid;
+	struct edid *old_edid;
 
-	intel_dp_unset_edid(intel_dp);
 	edid = intel_dp_get_edid(intel_dp);
+	old_edid = intel_connector->detect_edid;
+
+	if (!drm_edid_are_equal(edid, old_edid)) {
+		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Edid was changed! Updating blob property.\n",
+		    connector->base.id, connector->name);
+
+		connector->epoch_counter += 1;
+		DRM_DEBUG_KMS("Updating change counter to %llu\n", connector->epoch_counter);
+
+		intel_connector_update_modes(&intel_connector->base, edid);
+	}
+
+	intel_dp_unset_edid(intel_dp);
 	intel_connector->detect_edid = edid;
 
 	intel_dp->has_audio = drm_detect_monitor_audio(edid);
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 0ebec69bbbfc..f892c7b795ce 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2503,7 +2503,7 @@ intel_hdmi_set_edid(struct drm_connector *connector)
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
 	intel_wakeref_t wakeref;
-	struct edid *edid;
+	struct edid *edid, *old_edid;
 	bool connected = false;
 	struct i2c_adapter *i2c;
 
@@ -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;
+
 	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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] drm/i915: Send hotplug event if edid had changed.
  2019-06-28  4:24   ` Ramalingam C
@ 2019-06-28 11:36     ` Lisovskiy, Stanislav
  2019-06-28  4:50       ` Ramalingam C
  0 siblings, 1 reply; 10+ messages in thread
From: Lisovskiy, Stanislav @ 2019-06-28 11:36 UTC (permalink / raw)
  To: C, Ramalingam
  Cc: Ser, Simon, daniel.vetter@ffwll.ch,
	intel-gfx@lists.freedesktop.org, Peres, Martin,
	dri-devel@lists.freedesktop.org, paul.kocialkowski@bootlin.com,
	ppaalanen@gmail.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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/3] drm: Add helper to compare edids.
  2019-06-28  8:24 ` [PATCH v2 1/3] drm: Add helper to compare edids Stanislav Lisovskiy
  2019-06-28  4:13   ` Ramalingam C
@ 2019-07-01 19:52   ` Lyude Paul
  1 sibling, 0 replies; 10+ messages in thread
From: Lyude Paul @ 2019-07-01 19:52 UTC (permalink / raw)
  To: Stanislav Lisovskiy, dri-devel
  Cc: simon.ser, daniel.vetter, intel-gfx, martin.peres,
	paul.kocialkowski, ppaalanen

Sorry for the late response! I like the idea here and I've brought up edid
comparison a couple times. Hopefully this isn't overkill, but I had a little
more in mind then just a helper like this (and I've had this on my mind for a
while!

When it comes to suspend/resume reprobing, I think there's more work then just
comparing edids that are redundant. I think most drivers have connectors that
fall into one of the following classes:

 * Always "in-sync" with events, e.g. event handling does not stop just
   because the device is suspended. This is actually true in some cases for
   nouveau and amdgpu, where both drivers can rely on ACPI firmware to send
   events while the device is suspended.
 * Only in-sync with events while the device is awake. Of course, that's the
   whole reason for these patches!

From what I understand based on previous discussions with some other intel
engineers back when they were trying to make the i915 suspend/resume process
faster, hotplug probing can be a pretty significant timesink in some cases.
Additionally, it's not always nessecarily everywhere if some connectors are
able to stay in sync, and that might be a benefit.

Additionally, I think there's a number of other parts of the process that I
would imagine every driver would end up needing to implement. A couple rather
simple examples: skipping edid comparisons for disconnected or newly-connected
connectors, assuming that failure to read an EDID on a connected connector
that could have it's EDID read before suspend means we have to consider said
connector to be changed, etc. So why not add helpers to handle all of this
boilerplate as well?

An idea I had at one point would be to add the ability to mark when a driver
believes a DRM connector has gone "out of sync" like I mentioned above. A
simple example: on laptops I've observed with nouveau that supported ACPI
hotplug events, ACPI hotplug events only ever seemed to come if a connector
was plugged in - not if a connector was removed. If we use this logic during
the runtime resume, we could ascertain that unless an ACPI hotplug event was
received before we resumed that we can actually skip reprobing any connectors
that were disconnected at runtime suspend and only probe the ones that were
connected.

So, maybe we could have helpers like this:

/* Inform DRM that we've disabled our primary means of receiving HPD
 * events.
 */
drm_connector_suspend_hpd()

/* A helper that goes through and performs basic connector reprobing and
 * EDID comparisons on connectors marked with
 * drm_connector_reprobe_on_hpd_resume(). Fires off an HPD event if any
 * connector changes are found/returns an int/etc. etc. whatever.
 *
 * To be called by the driver *after* it has re-enabled HPD detection
 * for all of it's connectors.
 */
drm_connector_resume_hpd()

/* Indicate to DRM that the given connector no longer has HPD, and will need
 * to be reprobed on resume
 */
drm_connector_reprobe_on_hpd_resume()

/* Convienence function to indicate to DRM that all connectors have lost
 * their primary means to receive HPD events, and will need to be
 * reprobed on resume. Useful for scenarios like S3 suspend.
 */
drm_connector_reprobe_all_on_hpd_resume()

I think this would also fit in nicely with the new hotplug uevent ideas
that have been floating around recently, since we could then use these
helpers to compress all of the connector changes that happened over a
s/r cycle into a single event (or, no event!)

On Fri, 2019-06-28 at 11:24 +0300, Stanislav Lisovskiy wrote:
> Many drivers would benefit from using
> drm helper to compare edid, rather
> than bothering with own implementation.
> 
> v2: Added documentation for this function.
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 33 +++++++++++++++++++++++++++++++++
>  include/drm/drm_edid.h     |  9 +++++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 9d8f2b952004..eaad5155fbdd 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1361,6 +1361,39 @@ static bool drm_edid_is_zero(const u8 *in_edid, int
> length)
>  	return true;
>  }
>  
> +/**
> + * drm_edid_are_equal - compare two edid blobs.
> + * @edid1: pointer to first blob
> + * @edid2: pointer to second blob
> + * This helper can be used during probing to determine if
> + * edid had changed.
> + */
> +bool drm_edid_are_equal(struct edid *edid1, struct edid *edid2)
> +{
> +	int edid1_len, edid2_len;
> +	bool edid1_present = edid1 != NULL;
> +	bool edid2_present = edid2 != NULL;
> +
> +	if (edid1_present != edid2_present)
> +		return false;
> +
> +	if (edid1) {
> +
> +		edid1_len = EDID_LENGTH * (1 + edid1->extensions);
> +		edid2_len = EDID_LENGTH * (1 + edid2->extensions);
> +
> +		if (edid1_len != edid2_len)
> +			return false;
> +
> +		if (memcmp(edid1, edid2, edid1_len))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +EXPORT_SYMBOL(drm_edid_are_equal);
> +
> +
>  /**
>   * drm_edid_block_valid - Sanity check the EDID block (base or extension)
>   * @raw_edid: pointer to raw EDID block
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index b9719418c3d2..716964f63312 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -354,6 +354,15 @@ drm_load_edid_firmware(struct drm_connector *connector)
>  }
>  #endif
>  
> +/**
> + * drm_edid_are_equal - compare two edid blobs.
> + * @edid1: pointer to first blob
> + * @edid2: pointer to second blob
> + * This helper can be used during probing to determine if
> + * edid had changed.
> + */
> +bool drm_edid_are_equal(struct edid *edid1, struct edid *edid2);
> +
>  int
>  drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
>  					 struct drm_connector *connector,
-- 
Cheers,
	Lyude Paul

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-07-01 19:52 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-06-28  8:24 [PATCH v2 0/3] Send a hotplug when edid changes Stanislav Lisovskiy
2019-06-28  8:24 ` [PATCH v2 1/3] drm: Add helper to compare edids Stanislav Lisovskiy
2019-06-28  4:13   ` Ramalingam C
2019-07-01 19:52   ` Lyude Paul
2019-06-28  8:24 ` [PATCH v2 2/3] drm: Introduce change counter to drm_connector Stanislav Lisovskiy
2019-06-28  4:16   ` Ramalingam C
2019-06-28  8:24 ` [PATCH v2 3/3] drm/i915: Send hotplug event if edid had changed Stanislav Lisovskiy
2019-06-28  4:24   ` Ramalingam C
2019-06-28 11:36     ` Lisovskiy, Stanislav
2019-06-28  4:50       ` Ramalingam C

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox