Re: [PATCH 0/5] drm/i915 Avoid long delays when reading EDID on eDP

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Nov 25, 2014 at 06:20:17PM +0100, Egbert Eich wrote:
> Daniel Vetter writes:
> 
>  > Imo this approach with overwrite all the entry points won't scale since
>  > besides i2c and dpcd there will be more sooner or later (oui, dp mst, some
>  > debugfs userspace dp aux tools, ...).
>  > 
>  > I think what we need is the same as in the i2c layer has with the
>  > xfer_pre/post functions. To make this as painless as possible we should
>  > probably refcount that in the dp helper, protected by aux->hw_mutex. That
>  > way normal dp reads could just do the xfer_pre/post around the call to
>  > aux->transfer while i2c could do it around the entire i2c transaction.
> 
> I agree with your idea of having xfer_pre/post functions. 
> I'm not sure though if I understand your idea about reference counters:
> are you trying to protect from xfer_pre/post being called at different
> levels here? 
> With my proposal entire transactions (I2C) are serialized thru the pps_mutex 
> lock. This is a side effect that's probably unwanted. Is this what you are
> trying to avoid by using ref counters?

Yeah I want to protect entire transactions at the highest levels with
xfer_pre/post. The problem is that things nest and we can't grab
sufficient locking to prevent that nesting. E.g. for i2c we should do the
dp_aux xfer_pre/post calls from i2c xfer_pre/post (so that the entire edid
read only has one pre/post, assuming no retries). But the i2c lock which
protects that doesn't protect against concurrent dp aux transactions from
someone else (e.g. dp mst is done without modeset locks, or if we ever get
a userspace interface for debuggin db issues). So direct dp aux again
needs to do xfer_pre/post. Same for dp mst, we might want to wrap an
entire dp mst transaction with this, but the low-level code might not now.

Hence why I think we shold have a refcount for pre/post plus small helpers
(atm just static, we can export once we need them) like this

drm_dp_aux_xfer_pre(dp_aux)
{
	mutex_lock(dp_aux->hw_lock)
	if (dp_aux->xfer_count++ == 0)
		dp_aux->xfer_pre();
	mutex_unlock(dp_aux->hw_lock)
}

Same for xfer_post. Then we could wire that up for i2c-over-dp and for the
low-level dp xfer functions and it should all Just Work. And if there's a
new perf critical case where we want to do this over an entire series of
transactinos we can just call the two xfer_pre/post wrappers and done.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx