Re: [PATCH 1/2] drm/i915: Update virtual PCH in single function

[Colin Xu <Colin.Xu@xxxxxxxxx>]

On 05/28/2018 09:42 PM, Jani Nikula wrote:
> On Mon, 28 May 2018, Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
> > On Mon, 28 May 2018, Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
> > > On Tue, 29 May 2018, colin.xu@xxxxxxxxx wrote:
> > > > From: Colin Xu <colin.xu@xxxxxxxxx>
> > > >
> > > > 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:

Yes this is the most simply way.
The reason I didn't craft the patch like this in the beginning is that
I'm not sure after your refactoring patch, if the case exists that pch
id 0 maps to type either nop or none.
As you said there is no such case, the simply change should work well.
Will you made the change sometime or I need update my patch set?
--
Best Regards,
Colin Xu

> 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;
>
> ---
>
> BR,
> Jani.
>
>
> > BR,
> > Jani
> >
> >
> >
> >
> >
> > > BR,
> > > Jani.
> > >
> > >
> > > > Signed-off-by: Colin Xu <colin.xu@xxxxxxxxx>
> > > > ---
> > > >   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;
> > > >   		}
> > > >   	}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx