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

[Sharma, Shashank]

Thanks Daniel.
Let me incorporate these changes you suggested, and prepare a new patch set. 

Regards
Shashank 
-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
Sent: Thursday, August 14, 2014 5:27 PM
To: Sharma, Shashank
Cc: Daniel Vetter; intel-gfx; Vetter, Daniel; Kumar, Shobhit
Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915: Optimize HDMI EDID reads

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