Re: [PATCH 1/2] drm/i915: Optimize HDMI EDID reads

["Sharma, Shashank" <shashank.sharma@intel.com>]

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.

 > 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.

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

Regards
Shashank
On 8/14/2014 1:58 PM, Daniel Vetter wrote:
> On Thu, Aug 14, 2014 at 8:15 AM, Sharma, Shashank
> <shashank.sharma@intel.com> wrote:
>> Can you please point out what is it, that's unexpected to you ?
>> I think this is what we (you & Shobhit) agreed on:
>> 1. Timeout based EDID caching
>> 2. Use of cached EDID within caching duration
>> 3. Separate path for HDMI compliance, controllable in kernel, which doesn't
>> affect current code flow.
>
> - The timeout is 1 minute instead of 1s. That breaks interactions with
> the periodical probing we do when there's a storm.
> - There's a generation check in there. This is a generic problem,
> restricting platforms only means that fewer people will be able to
> test it and find issues with broken hdmi sinks. The problem here are
> _sink_ devices, not necessarily platforms. So testing for platforms is
> bogus.
> - Keying off the force parameter isn't actually precise enough.
> There's an encoder->hot_plug callback where you should invalidate the
> edid instead.
> - Adding the edid caching to the intel_hdmi struct is the wrong place,
> we already have an edid pointer in intel_connector, which is used for
> panels. Augmenting that to allow caching with time-based invalidation
> is the right solution instead of inventing a completely new wheel.
>
> Aside: You commit message is misleading since it's actually not
> required to do a full probe cycle for the get_connector ioctl. You can
> get at the current cached mode list without any probe calls at all.
> Please have a look at SNA for how to do that. And if you have
> userspace constantly probing sysfs files and other stuff instead of
> listening to uevents then you need to fix your userspace, not cache
> the edid in the kernel for a minute.
> -Daniel
>