From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Sharma, Shashank" Subject: Re: [PATCH 1/2] drm/i915: Optimize HDMI EDID reads Date: Thu, 14 Aug 2014 14:53:44 +0530 Message-ID: <53EC8020.1070707@intel.com> References: <1407847101-21654-1-git-send-email-shashank.sharma@intel.com> <1407847101-21654-2-git-send-email-shashank.sharma@intel.com> <20140813121455.GX10500@phenom.ffwll.local> <53EC53E9.2030809@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id 7EF086E645 for ; Thu, 14 Aug 2014 02:23:51 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: Daniel Vetter , intel-gfx , "Kumar, Shobhit" List-Id: intel-gfx@lists.freedesktop.org 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 > 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 >