From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: Allow user modes to exceed DVI 165MHz limit Date: Thu, 27 Mar 2014 13:03:31 +0200 Message-ID: <20140327110331.GQ21652@intel.com> References: <1395911325-4981-1-git-send-email-ville.syrjala@linux.intel.com> <20140327093736.GA30820@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 260B06E4DF for ; Thu, 27 Mar 2014 04:03:41 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140327093736.GA30820@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson , intel-gfx@lists.freedesktop.org, stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Mar 27, 2014 at 09:37:36AM +0000, Chris Wilson wrote: > On Thu, Mar 27, 2014 at 11:08:45AM +0200, ville.syrjala@linux.intel.com w= rote: > > So relax the checks a bit, and apply the single-link DVI dotclock limit > > only when filtering the mode list, and ignore the limit when setting > > a user specified mode. > = > Mind enlightening me as to how this actually works? I thought all > display modes were validated before we used them, so how come this > sneaks through? > = > So it goes like this: > = > userspace calls GETCONNECTOR > kernel: fill_modes -> drm_helper_probe_single_connector_modes -> mode_val= id? > = > but > = > userspace calls SETCRTC with a random mode > kernel: applies random mode without validation > = > Seriously we don't do any checking that the mode given to SETCRTC is > applicable and not in any way harmful before setting registers? Pretty much. Calling our user mode validation even "minimal" is a bit of a stretch. And the checks we have in the .mode_valid() hooks are lacking as well. As far as this particular patch goes, we do check the port clock in the hdmi compute_config hook, so I had to relax it a bit to ignore the single link DVI limit there, and instead just check it against the hardware specific limit from BSpec. I think that's a reasonable thing to do. If we wanted to protect the user (or the equipment in case of CRTs) more, we should probably check things against the EDID min/max scan rates and max clock. -- = Ville Syrj=E4l=E4 Intel OTC