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.sha...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
