Message-ID: <1500967544.1277.1.camel@linux.intel.com>
Subject: Re: [PATCH 1/2] drm/edid: Add helper to detect whether EDID changed
From: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        intel-gfx@lists.freedesktop.org,
        Daniel Vetter <daniel.vetter@intel.com>
Date: Tue, 25 Jul 2017 10:25:44 +0300
In-Reply-To: <20170725065328.fhe75lpunqn7vcqv@phenom.ffwll.local>
References: <20170724145447.23449-1-paul.kocialkowski@linux.intel.com>
         <20170725065328.fhe75lpunqn7vcqv@phenom.ffwll.local>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160
 Espoo, Finland
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4160
Lines: 132

On Tue, 2017-07-25 at 08:53 +0200, Daniel Vetter wrote:
> On Mon, Jul 24, 2017 at 05:54:46PM +0300, Paul Kocialkowski wrote:
> > This adds a common drm helper to detect whether the EDID changed
> > from
> > the last known cached one. This is useful help detect that a monitor
> > was
> > changed during a suspend/resume cycle.
> > 
> > When that happens (a monitor is replaced by another one during
> > suspend),
> > no hotplug event will be triggered so the change will not be caught
> > at
> > resume time. Detecting that the EDID changed allows detecting it.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> 
> I can't find the older mails I've typed about this, but the plan we've
> discussed a while back was:
> - Add a generational counter to each connector, maybe even expose it
> to
>   userspace.
> 
> - Increment that counter every time something changed, e.g.
>   connector->status in the propbe helpers, or when attaching a new
> edid
>   with the set_edid helper.
> 
> - Tada, no changes needed to drivers, and easily extensible to other
>   things than edid!

I don't see how it solves the problem here though. After a
suspend/resume cycle, there is simply no indication that anything has
changed when a monitor was replaced by another one, so I don't see how
adding a counter in the mix would help.

Could you provide more details about the reasoning? I feel like I'm
missing something here.

Cheers,

Paul

> > ---
> >  drivers/gpu/drm/drm_edid.c | 45
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_edid.h     |  3 +++
> >  2 files changed, 48 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 6bb6337be920..f6ce8bc2907a 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -5036,3 +5036,48 @@ static void drm_get_displayid(struct
> > drm_connector *connector,
> >  	}
> >  	return;
> >  }
> > +
> > +/**
> > + * drm_check_edid_changed - Check whether the EDID changed since
> > the last update
> > + * @connector: connector we're probing
> > + * @adapter: I2C adapter to use for DDC
> > + *
> > + * Check whether the EDID changed since the last time it was
> > updated in the
> > + * drm blob cache.
> > + *
> > + * Return: A boolean indicating whether a change happened or not.
> > + */
> > +bool drm_check_edid_changed(struct drm_connector *connector,
> > +			    struct i2c_adapter *adapter)
> > +{
> > +	struct drm_property_blob *edid_blob;
> > +	struct edid *edid_stored;
> > +	struct edid *edid_read;
> > +	int ret = 0;
> > +
> > +	if (!connector->edid_blob_ptr)
> > +		return false;
> > +
> > +	edid_blob = drm_property_blob_get(connector-
> > >edid_blob_ptr);
> > +	if (!edid_blob)
> > +		return false;
> > +
> > +	if (!edid_blob->data || edid_blob->length != sizeof(struct
> > edid))
> > +		goto out;
> > +
> > +	edid_stored = (struct edid *) edid_blob->data;
> > +
> > +	edid_read = drm_get_edid(connector, adapter);
> > +	if (!edid_read)
> > +		goto out;
> > +
> > +	ret = memcmp(edid_stored, edid_read, sizeof(struct edid));
> > +
> > +	kfree(edid_read);
> > +
> > +out:
> > +	drm_property_blob_put(edid_blob);
> > +
> > +	return ret != 0;
> > +}
> > +EXPORT_SYMBOL_GPL(drm_check_edid_changed);
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index 1e1908a6b1d6..593a97b269c3 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -485,4 +485,7 @@ void drm_edid_get_monitor_name(struct edid
> > *edid, char *name,
> >  struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
> >  					   int hsize, int vsize,
> > int fresh,
> >  					   bool rb);
> > +bool drm_check_edid_changed(struct drm_connector *connector,
> > +			    struct i2c_adapter *adapter);
> > +
> >  #endif /* __DRM_EDID_H__ */
> > -- 
> > 2.13.2
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
-- 
Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland