From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 1/2] drm/i915: Optimize HDMI EDID reads Date: Thu, 14 Aug 2014 10:28:00 +0200 Message-ID: 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" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ig0-f182.google.com (mail-ig0-f182.google.com [209.85.213.182]) by gabe.freedesktop.org (Postfix) with ESMTP id 28F806E2E9 for ; Thu, 14 Aug 2014 01:28:01 -0700 (PDT) Received: by mail-ig0-f182.google.com with SMTP id c1so4278199igq.3 for ; Thu, 14 Aug 2014 01:28:00 -0700 (PDT) In-Reply-To: <53EC53E9.2030809@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "Sharma, Shashank" Cc: Daniel Vetter , intel-gfx , "Kumar, Shobhit" List-Id: intel-gfx@lists.freedesktop.org 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch