[Intel-gfx] [PATCH 1/2] drm/i915: Optimize HDMI EDID reads

[Daniel Vetter]

On Thu, Aug 14, 2014 at 02:53:44PM +0530, Sharma, Shashank wrote:
> Hi Daniel,
> 
> I can do all the changes accordingly and upload a new patch.
> 
> This is what I am going to do:
> 1. Change the EDID caching time to a second from a minute (probably there
> was a miscommunication).
> 2. Remove the gen_check
> 3. Use the connector->edid pointer to cache EDID.
> 
> I have only few problems with these two suggestions:
> > Keying off the force parameter isn't actually precise enough.
> It is. All the calls to HDMI detect, which come as a result of user space
> interaction keep force = 1, whereas all the hot plug event callers keep it
> force = 0. Please have a look:
> 
> IOCTL / Sysfs calls, calling connector->funcs->detect()
> 1. drm_helper_probe_single_connector_modes_merge_bits => (force = 1)
> 2. status_show => (force = 1)
> 
> Hot plug handlers / hot plug poll
> 1. drm_helper_hpd_irq_event => (force = 0)
> 2. output_poll_execute => (force = 0)
> So this should work all right.

Hm indeed, I've mixed up things with the output poll worker. My concern is
mostly that "force" already has overloaded meaning. But I think if we
polish the code-flow a bit it should work out. Quick draft:

	/* at the top of the detect function before anything else */
	if (!force)
		intel_connector_invalidate_edid(conn);


Then we replace all current calls to drm_get_edid with a new
intel_connector_get_edid_cached, which first checks the conn->edid cache
and only if that's empty call drm_edid_cache.

btw for the edid cache itself I think Chris' idea to use errno pointers is
great, so the state machine would be:

connector->edid == NULL -> cache expired

IS_ERR(connector->edid) -> negative cache entry with errno value, e.g.
-ENXIO. this way we also speed up subsequent detect calls from userspace
when nothing is connected.

everything else -> cached edid

So in code this would look like

void intel_connector_invalidate_edid(conn)
{
	/* locking tbd */

	if (!conn->edid)
		return;

	if (!IS_ERR(conn->edid))
		kfree(conn->edid);

	conn->edid = NULL;
}

struct drm_edid *intel_connector_get_edid_cached(conn, ...)
{
	/* locking tbd */

	if (!conn->edid) {
		conn->edid = drm_get_edid(...);
		if (!conn->edid)
			conn->edid = ERR_PTR(-ENXIO);

		/* rearm invalidate timer */
	}

	return IS_ERR(conn->edid) ? NULL : conn->edid;
}

btw for locking I'd just pick a spinlock, then you can use a simple timer
to free the edid. Currently the drm modeset locking is a bit fuzzy around
connectors (due to the dp mst introduction), so separate locking would
simplify things.

> > There's an encoder->hot_plug callback where you should invalidate the
> > edid instead.
> In MCG branch, we are doing this in encoder->hot_plug only. But there the
> EDID stays, until there is one more next hotplug, and by that time, the
> detect code uses cached EDID only.
> As encoder->hot_plug function also gets called every time there is a
> hot_plug, from the hotplug_work_fn, I was afraid this might cause a race.
> Second, I still have to write a delayed_work wrapper, to call
> encoder->hot_plug from, after the timeout.

I've thought about the hotplug work function when we invalidate the edid,
and I think we can wait with that until there's demand. Currently if the
sink is horribly broken and does only respond after a while we will also
not detect it right away. And as long as we invalidate the edid soonish
(before the user could poke his system to figure out where the screen
went) we'll be good.

> If you feel that, its better to do it there, I can do changes accordingly.

Nah, I think if we wrap the caching/invalidation up into the above
suggested tiny helpers the code will be clear enough and your idea of
using "force" indeed works.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch