[Intel-gfx] [PATCH v4 2/3] drm: Introduce change counter to drm_connector

[Lisovskiy, Stanislav]

On Thu, 2019-09-05 at 13:01 +0200, Maarten Lankhorst wrote:
> Op 05-09-2019 om 12:37 schreef Stanislav Lisovskiy:
> > 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 at 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 4c766624b20d..18b1ad2a4eee 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -246,6 +246,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 351cbc40f0f8..5131ae56e676 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -777,6 +777,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;
> > @@ -790,20 +791,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);
> 
> Pass a u32 *changed as argument to drm_helper_probe_detect? would
> require signature change,
> >  		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;
> > +		}
> 
> Could we bump the epoch counter for any event being sent out? Would
> make more sense.

I was thinking about this. That would be quite sane approach I think.
But then we might have to change also many other places where it checks
connection status only. I will take a look as it anyways looks better
than checking both epoch and connection status at the same time.

> >  	}
> >  	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 681cb590f952..a3cc7d0927d6 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -1288,6 +1288,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
> 
>