From patchwork Mon May 28 13:42:49 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jani Nikula X-Patchwork-Id: 10432703 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 8A573603B5 for ; Mon, 28 May 2018 13:38:58 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 7B1F028ADE for ; Mon, 28 May 2018 13:38:58 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 6F7FA28AF1; Mon, 28 May 2018 13:38:58 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=2.0 tests=BAYES_00, MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id C6A7028AE6 for ; Mon, 28 May 2018 13:38:57 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 0550189F61; Mon, 28 May 2018 13:38:56 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTPS id C30A589F47 for ; Mon, 28 May 2018 13:38:54 +0000 (UTC) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 May 2018 06:38:54 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,452,1520924400"; d="scan'208";a="59194902" Received: from jnikula-mobl2.fi.intel.com (HELO localhost) ([10.237.72.62]) by fmsmga001.fm.intel.com with ESMTP; 28 May 2018 06:38:52 -0700 From: Jani Nikula To: colin.xu@intel.com, intel-gfx@lists.freedesktop.org In-Reply-To: <87r2lwhzzv.fsf@intel.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <20180529045038.31397-1-colin.xu@intel.com> <20180529045038.31397-2-colin.xu@intel.com> <87tvqsi0im.fsf@intel.com> <87r2lwhzzv.fsf@intel.com> Date: Mon, 28 May 2018 16:42:49 +0300 Message-ID: <87in77j3qe.fsf@intel.com> MIME-Version: 1.0 Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915: Update virtual PCH in single function X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Virus-Scanned: ClamAV using ClamSMTP On Mon, 28 May 2018, Jani Nikula wrote: > On Mon, 28 May 2018, Jani Nikula wrote: >> On Tue, 29 May 2018, colin.xu@intel.com wrote: >>> From: Colin Xu >>> >>> The existing way to update virtual PCH will return wrong PCH type >>> in case the host doesn't have PCH: >>> - intel_virt_detect_pch returns guessed PCH id 0 >>> - id 0 maps to PCH_NOP. >> should be PCH_NONE. >>> Since PCH_NONE and PCH_NOP are different types, mixing them up >>> will break vbt initialization logic. >>> >>> In addition, to add new none/nop PCH override for a specific >>> platform, branching need to be added to intel_virt_detect_pch(), >>> intel_pch_type() and the caller since none/nop PCH is not always >>> mapping to the same predefined PCH id. >>> >>> This patch merges the virtual PCH update/sanity check logic into >>> single function intel_virt_update_pch(), which still keeps using >>> existing intel_pch_type() to do the sanity check, while making it >>> clean to override virtual PCH id for a specific platform for future >>> platform enablement. >> >> Please keep the assignment out of intel_virt_{detect,update}_pch like. I >> think the patch here is unnecessarily complicated. > > To elaborate, intel_pch_type() should *always* be able to map pch id to > pch type. There should not be combinations that aren't covered by > that. If the sanity checks there need to accept Broxton as well, perhaps > pass a parameter to indicate virtualization, and accept certain pch ids > for Broxton as well. > > If you're faking a pch for Broxton, I don't think there's a case where > pch id should be 0 and pch type should be something else. Either both > are zero, or both are non-zero. -ENOCOFFEE in the morning. Is the fix you're looking for simply: --- BR, Jani. > > > BR, > Jani > > > > > >> >> BR, >> Jani. >> >> >>> >>> Signed-off-by: Colin Xu >>> --- >>> drivers/gpu/drm/i915/i915_drv.c | 56 ++++++++++++++++++--------------- >>> 1 file changed, 30 insertions(+), 26 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >>> index fb39e40c0847..637ba86104be 100644 >>> --- a/drivers/gpu/drm/i915/i915_drv.c >>> +++ b/drivers/gpu/drm/i915/i915_drv.c >>> @@ -209,10 +209,11 @@ static bool intel_is_virt_pch(unsigned short id, >>> sdevice == PCI_SUBDEVICE_ID_QEMU)); >>> } >>> >>> -static unsigned short >>> -intel_virt_detect_pch(const struct drm_i915_private *dev_priv) >>> +static void >>> +intel_virt_update_pch(struct drm_i915_private *dev_priv) >>> { >>> unsigned short id = 0; >>> + enum intel_pch pch_type = PCH_NONE; >>> >>> /* >>> * In a virtualized passthrough environment we can be in a >>> @@ -221,25 +222,37 @@ intel_virt_detect_pch(const struct drm_i915_private *dev_priv) >>> * make an educated guess as to which PCH is really there. >>> */ >>> >>> - if (IS_GEN5(dev_priv)) >>> + if (IS_GEN5(dev_priv)) { >>> id = INTEL_PCH_IBX_DEVICE_ID_TYPE; >>> - else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv)) >>> + pch_type = intel_pch_type(dev_priv, id); >>> + DRM_DEBUG_KMS("Assuming Ibex Peak PCH id %04x\n", id); >>> + } else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv)) { >>> id = INTEL_PCH_CPT_DEVICE_ID_TYPE; >>> - else if (IS_HSW_ULT(dev_priv) || IS_BDW_ULT(dev_priv)) >>> - id = INTEL_PCH_LPT_LP_DEVICE_ID_TYPE; >>> - else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) >>> - id = INTEL_PCH_LPT_DEVICE_ID_TYPE; >>> - else if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) >>> + pch_type = intel_pch_type(dev_priv, id); >>> + DRM_DEBUG_KMS("Assuming CougarPoint PCH id %04x\n", id); >>> + } else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { >>> + if (IS_HSW_ULT(dev_priv) || IS_BDW_ULT(dev_priv)) >>> + id = INTEL_PCH_LPT_LP_DEVICE_ID_TYPE; >>> + else >>> + id = INTEL_PCH_LPT_DEVICE_ID_TYPE; >>> + pch_type = intel_pch_type(dev_priv, id); >>> + DRM_DEBUG_KMS("Assuming LynxPoint PCH id %04x\n", id); >>> + } else if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) { >>> id = INTEL_PCH_SPT_DEVICE_ID_TYPE; >>> - else if (IS_COFFEELAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) >>> + pch_type = intel_pch_type(dev_priv, id); >>> + DRM_DEBUG_KMS("Assuming SunrisePoint PCH id %04x\n", id); >>> + } else if (IS_COFFEELAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) { >>> id = INTEL_PCH_CNP_DEVICE_ID_TYPE; >>> + pch_type = intel_pch_type(dev_priv, id); >>> + DRM_DEBUG_KMS("Assuming CannonPoint PCH id %04x\n", id); >>> + } else { >>> + id = 0; >>> + pch_type = PCH_NOP; >>> + DRM_DEBUG_KMS("Assuming NOP PCH\n"); >>> + } >>> >>> - if (id) >>> - DRM_DEBUG_KMS("Assuming PCH ID %04x\n", id); >>> - else >>> - DRM_DEBUG_KMS("Assuming no PCH\n"); >>> - >>> - return id; >>> + dev_priv->pch_type = pch_type; >>> + dev_priv->pch_id = id; >>> } >>> >>> static void intel_detect_pch(struct drm_i915_private *dev_priv) >>> @@ -281,16 +294,7 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv) >>> break; >>> } else if (intel_is_virt_pch(id, pch->subsystem_vendor, >>> pch->subsystem_device)) { >>> - id = intel_virt_detect_pch(dev_priv); >>> - if (id) { >>> - pch_type = intel_pch_type(dev_priv, id); >>> - if (WARN_ON(pch_type == PCH_NONE)) >>> - pch_type = PCH_NOP; >>> - } else { >>> - pch_type = PCH_NOP; >>> - } >>> - dev_priv->pch_type = pch_type; >>> - dev_priv->pch_id = id; >>> + intel_virt_update_pch(dev_priv); >>> break; >>> } >>> } diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9c449b8d8eab..ae07e36e364c 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -287,7 +287,7 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv) if (WARN_ON(pch_type == PCH_NONE)) pch_type = PCH_NOP; } else { - pch_type = PCH_NOP; + pch_type = PCH_NONE; } dev_priv->pch_type = pch_type; dev_priv->pch_id = id;