[Intel-gfx] [PATCH v3 2/6] drm/i915/opregion: Abstract opregion function

Anshuman Gupta anshuman.gupta at intel.com
Fri Apr 1 13:26:50 UTC 2022


On 2022-03-16 at 11:25:33 +0200, Jani Nikula wrote:
> On Sun, 20 Feb 2022, Anshuman Gupta <anshuman.gupta at intel.com> wrote:
> > Abstract opregion operations like get opregion base, get rvda and
> > opregion cleanup in form of i915_opregion_ops.
> > This will be required to converge igfx and dgfx opregion.
> >
> > v2:
> > - Keep only function pointer abstraction stuff. [Jani]
> > - Add alloc_rvda error handling.
> >
> > v3:
> > - Added necessary credit to Manasi for static analysis fix around
> >   drm_WARN_ON(&i915->drm, !opregion->asls || !opregion->header)
> >
> > Cc: Jani Nikula <jani.nikula at intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Cc: Badal Nilawar <badal.nilawar at intel.com>
> > Cc: Uma Shankar <uma.shankar at intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_opregion.c | 179 +++++++++++++-----
> >  drivers/gpu/drm/i915/display/intel_opregion.h |   3 +
> >  2 files changed, 134 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
> > index 9b56064ddb5d..94eb7c23fcb4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_opregion.c
> > +++ b/drivers/gpu/drm/i915/display/intel_opregion.c
> > @@ -138,6 +138,13 @@ struct opregion_asle_ext {
> >  	u8 rsvd[764];
> >  } __packed;
> >  
> > +struct i915_opregion_func {
> > +	void *(*alloc_opregion)(struct drm_i915_private *i915);
> > +	void *(*alloc_rvda)(struct drm_i915_private *i915);
> > +	void (*free_rvda)(struct drm_i915_private *i915);
> > +	void (*free_opregion)(struct drm_i915_private *i915);
> > +};
> > +
> >  /* Driver readiness indicator */
> >  #define ASLE_ARDY_READY		(1 << 0)
> >  #define ASLE_ARDY_NOT_READY	(0 << 0)
> > @@ -876,10 +883,7 @@ static int intel_load_vbt_firmware(struct drm_i915_private *dev_priv)
> >  static int intel_opregion_setup(struct drm_i915_private *dev_priv)
> >  {
> >  	struct intel_opregion *opregion = &dev_priv->opregion;
> > -	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
> > -	u32 asls, mboxes;
> > -	char buf[sizeof(OPREGION_SIGNATURE)];
> > -	int err = 0;
> > +	u32 mboxes;
> >  	void *base;
> >  	const void *vbt;
> >  	u32 vbt_size;
> > @@ -890,27 +894,12 @@ static int intel_opregion_setup(struct drm_i915_private *dev_priv)
> >  	BUILD_BUG_ON(sizeof(struct opregion_asle) != 0x100);
> >  	BUILD_BUG_ON(sizeof(struct opregion_asle_ext) != 0x400);
> >  
> > -	pci_read_config_dword(pdev, ASLS, &asls);
> > -	drm_dbg(&dev_priv->drm, "graphic opregion physical addr: 0x%x\n",
> > -		asls);
> > -	if (asls == 0) {
> > -		drm_dbg(&dev_priv->drm, "ACPI OpRegion not supported!\n");
> > -		return -ENOTSUPP;
> > -	}
> > -
> >  	INIT_WORK(&opregion->asle_work, asle_work);
> >  
> > -	base = memremap(asls, OPREGION_SIZE, MEMREMAP_WB);
> > -	if (!base)
> > -		return -ENOMEM;
> > +	base = opregion->opregion_func->alloc_opregion(dev_priv);
> > +	if (IS_ERR(base))
> > +		return PTR_ERR(base);
> >  
> > -	memcpy(buf, base, sizeof(buf));
> > -
> > -	if (memcmp(buf, OPREGION_SIGNATURE, 16)) {
> > -		drm_dbg(&dev_priv->drm, "opregion signature mismatch\n");
> > -		err = -EINVAL;
> > -		goto err_out;
> > -	}
> >  	opregion->header = base;
> >  	opregion->lid_state = base + ACPI_CLID;
> >  
> > @@ -970,23 +959,10 @@ static int intel_opregion_setup(struct drm_i915_private *dev_priv)
> >  
> >  	if (opregion->header->over.major >= 2 && opregion->asle &&
> >  	    opregion->asle->rvda && opregion->asle->rvds) {
> > -		resource_size_t rvda = opregion->asle->rvda;
> > -
> > -		/*
> > -		 * opregion 2.0: rvda is the physical VBT address.
> > -		 *
> > -		 * opregion 2.1+: rvda is unsigned, relative offset from
> > -		 * opregion base, and should never point within opregion.
> > -		 */
> > -		if (opregion->header->over.major > 2 ||
> > -		    opregion->header->over.minor >= 1) {
> > -			drm_WARN_ON(&dev_priv->drm, rvda < OPREGION_SIZE);
> > -
> > -			rvda += asls;
> > -		}
> >  
> > -		opregion->rvda = memremap(rvda, opregion->asle->rvds,
> > -					  MEMREMAP_WB);
> > +		opregion->rvda = opregion->opregion_func->alloc_rvda(dev_priv);
> 
> It's a basic principle of interfaces to do corresponding things at the
> same abstraction level.
> 
> Now, you set opregion->rvda at the level that calls ->alloc_rvda,
> however ->free_rvda accesses opregion->rvda directly.
> 
> Similarly for ->alloc_opregion and ->free_opregion.
Thanks for review comment.
Could you please shed light to make same abstraction level.
	free_rvda(void *rvda)
	{
		IS_ERR(rvda)
			return;
		if (rvda) {
			 memunmap(opregion->rvda);
			 rvda = NULL;
		}	
	}
I could think above kind of approach to address abstraction level
Please guide me if that is corrects.
> 
> > +		if (IS_ERR(opregion->rvda))
> > +			goto mbox4_vbt;
> >  
> >  		vbt = opregion->rvda;
> >  		vbt_size = opregion->asle->rvds;
> > @@ -999,11 +975,12 @@ static int intel_opregion_setup(struct drm_i915_private *dev_priv)
> >  		} else {
> >  			drm_dbg_kms(&dev_priv->drm,
> >  				    "Invalid VBT in ACPI OpRegion (RVDA)\n");
> > -			memunmap(opregion->rvda);
> > -			opregion->rvda = NULL;
> > +			opregion->opregion_func->free_rvda(dev_priv);
> >  		}
> >  	}
> >  
> > +mbox4_vbt:
> > +
> >  	vbt = base + OPREGION_VBT_OFFSET;
> >  	/*
> >  	 * The VBT specification says that if the ASLE ext mailbox is not used
> > @@ -1028,9 +1005,6 @@ static int intel_opregion_setup(struct drm_i915_private *dev_priv)
> >  out:
> >  	return 0;
> >  
> > -err_out:
> > -	memunmap(base);
> > -	return err;
> >  }
> >  
> >  static int intel_use_opregion_panel_type_callback(const struct dmi_system_id *id)
> > @@ -1215,11 +1189,9 @@ void intel_opregion_unregister(struct drm_i915_private *i915)
> >  	}
> >  
> >  	/* just clear all opregion memory pointers now */
> > -	memunmap(opregion->header);
> > -	if (opregion->rvda) {
> > -		memunmap(opregion->rvda);
> > -		opregion->rvda = NULL;
> > -	}
> > +	opregion->opregion_func->free_rvda(i915);
> > +	opregion->opregion_func->free_opregion(i915);
> > +
> >  	if (opregion->vbt_firmware) {
> >  		kfree(opregion->vbt_firmware);
> >  		opregion->vbt_firmware = NULL;
> > @@ -1233,6 +1205,113 @@ void intel_opregion_unregister(struct drm_i915_private *i915)
> >  	opregion->lid_state = NULL;
> >  }
> >  
> > +static int
> > +intel_opregion_get_asls(struct drm_i915_private *i915)
> 
> You'd expect a function named "get asls" to return asls.
will fix this.
> 
> > +{
> > +	struct intel_opregion *opregion = &i915->opregion;
> > +	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> > +	u32 asls;
> > +
> > +	pci_read_config_dword(pdev, ASLS, &asls);
> > +	drm_dbg(&i915->drm, "graphic opregion physical addr: 0x%x\n",
> > +		asls);
> > +	if (asls == 0) {
> > +		drm_dbg(&i915->drm, "ACPI OpRegion not supported!\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	opregion->asls = asls;
> > +
> > +	return 0;
> > +}
> > +
> > +static void *intel_igfx_alloc_opregion(struct drm_i915_private *i915)
> > +{
> > +	struct intel_opregion *opregion = &i915->opregion;
> > +	char buf[sizeof(OPREGION_SIGNATURE)];
> > +	int err = 0;
> > +	void *base;
> > +
> > +	err = intel_opregion_get_asls(i915);
> > +	if (err)
> > +		return ERR_PTR(err);
> > +
> > +	base = memremap(opregion->asls, OPREGION_SIZE, MEMREMAP_WB);
> > +	if (!base)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	memcpy(buf, base, sizeof(buf));
> > +
> > +	if (memcmp(buf, OPREGION_SIGNATURE, 16)) {
> > +		drm_dbg(&i915->drm, "opregion signature mismatch\n");
> > +		err = -EINVAL;
> > +		goto err_out;
> > +	}
> > +
> > +	return base;
> > +
> > +err_out:
> > +	memunmap(base);
> > +
> > +	return ERR_PTR(err);
> > +}
> > +
> > +static void *intel_igfx_alloc_rvda(struct drm_i915_private *i915)
> > +{
> > +	struct intel_opregion *opregion = &i915->opregion;
> > +	resource_size_t rvda;
> > +	void *opreg_rvda;
> > +
> > +	if(drm_WARN_ON(&i915->drm, !opregion->asls || !opregion->header))
>           ^
> 
> Missing space.
Will fix this.
Thanks,
Anshuman.
> 
> > +		return ERR_PTR(-ENODEV);
> > +
> > +	rvda = opregion->asle->rvda;
> > +
> > +	/*
> > +	 * opregion 2.0: rvda is the physical VBT address.
> > +	 *
> > +	 * opregion 2.1+: rvda is unsigned, relative offset from
> > +	 * opregion base, and should never point within opregion.
> > +	 */
> > +	if (opregion->header->over.major > 2 ||
> > +	    opregion->header->over.minor >= 1) {
> > +		drm_WARN_ON(&i915->drm, rvda < OPREGION_SIZE);
> > +
> > +		rvda += opregion->asls;
> > +	}
> > +
> > +	opreg_rvda = memremap(rvda, opregion->asle->rvds, MEMREMAP_WB);
> > +	if (!opreg_rvda)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	return opreg_rvda;
> > +}
> > +
> > +static void intel_igfx_free_rvda(struct drm_i915_private *i915)
> > +{
> > +	struct intel_opregion *opregion = &i915->opregion;
> > +
> > +	if (opregion->rvda) {
> > +		memunmap(opregion->rvda);
> > +		opregion->rvda = NULL;
> > +	}
> > +}
> > +
> > +static void intel_igfx_free_opregion(struct drm_i915_private *i915)
> > +{
> > +	struct intel_opregion *opregion = &i915->opregion;
> > +
> > +	if (opregion->header)
> > +		memunmap(opregion->header);
> > +}
> > +
> > +static const struct i915_opregion_func igfx_opregion_func = {
> > +	.alloc_opregion = intel_igfx_alloc_opregion,
> > +	.alloc_rvda = intel_igfx_alloc_rvda,
> > +	.free_rvda = intel_igfx_free_rvda,
> > +	.free_opregion = intel_igfx_free_opregion,
> > +};
> > +
> >  /**
> >   * intel_opregion_init() - Init ACPI opregion.
> >   * @i915 i915 device priv data.
> > @@ -1240,5 +1319,9 @@ void intel_opregion_unregister(struct drm_i915_private *i915)
> >   */
> >  int intel_opregion_init(struct drm_i915_private *i915)
> >  {
> > +	struct intel_opregion *opregion = &i915->opregion;
> > +
> > +	opregion->opregion_func = &igfx_opregion_func;
> > +
> >  	return intel_opregion_setup(i915);
> >  }
> > diff --git a/drivers/gpu/drm/i915/display/intel_opregion.h b/drivers/gpu/drm/i915/display/intel_opregion.h
> > index 744d53c804e2..7500c396b74d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_opregion.h
> > +++ b/drivers/gpu/drm/i915/display/intel_opregion.h
> > @@ -37,6 +37,7 @@ struct opregion_acpi;
> >  struct opregion_swsci;
> >  struct opregion_asle;
> >  struct opregion_asle_ext;
> > +struct i915_opregion_func;
> >  
> >  struct intel_opregion {
> >  	struct opregion_header *header;
> > @@ -46,6 +47,8 @@ struct intel_opregion {
> >  	u32 swsci_sbcb_sub_functions;
> >  	struct opregion_asle *asle;
> >  	struct opregion_asle_ext *asle_ext;
> > +	const struct i915_opregion_func *opregion_func;
> > +	resource_size_t asls;
> >  	void *rvda;
> >  	void *vbt_firmware;
> >  	const void *vbt;
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center


More information about the Intel-gfx mailing list