Re: [PATCH 7/8] drm/i915: Add support to parse DMI table and get platform memory info (v3)

[Matt Roper <matthew.d.roper@intel.com>]

On Tue, Mar 08, 2016 at 08:49:23PM +0200, Ville Syrjälä wrote:
> On Mon, Mar 07, 2016 at 05:05:45PM -0800, Matt Roper wrote:
> > From: Shobhit Kumar <shobhit.kumar@intel.com>
> > 
> > This is needed for WM computation workaround for arbitrated display
> > bandwidth.
> > 
> > v2: Address Matt's review comments
> >     - Be more paranoid while dmi decoding
> >     - Also add support for decoding speed from configured memory speed
> >       if availble in DMI memory entry
> > 
> > v3 (by Matt):
> >  - Use memdev_dmi_entry from dmi.h
> >  - Don't try to use/compare negative numbers in unsigned types
> > 
> > Cc: matthew.d.roper@intel.com
> > Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c | 34 ++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_drv.h |  7 +++++++
> >  2 files changed, 41 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 4aa3db6..4259afe 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -49,6 +49,7 @@
> >  #include <linux/pm.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/oom.h>
> > +#include <linux/dmi.h>
> >  
> >  
> >  static int i915_getparam(struct drm_device *dev, void *data,
> > @@ -973,6 +974,33 @@ static void i915_mmio_cleanup(struct drm_device *dev)
> >  	pci_iounmap(dev->pdev, dev_priv->regs);
> >  }
> >  
> > +static void dmi_decode_memory_info(const struct dmi_header *hdr, void *priv)
> > +{
> > +	struct drm_i915_private *dev_priv = (struct drm_i915_private *) priv;
> > +	const struct memdev_dmi_entry *memdev =
> > +		(const struct memdev_dmi_entry *)hdr;
> > +	uint16_t mem_speed = 0;
> > +
> > +	if (hdr->type != DMI_ENTRY_MEM_DEVICE)
> > +		return;
> > +
> > +	/* Get the speed */
> > +	if (hdr->length > offsetof(struct memdev_dmi_entry, conf_mem_clk_speed))
> > +		mem_speed = memdev->conf_mem_clk_speed;
> > +	else if (hdr->length > offsetof(struct memdev_dmi_entry, speed))
> > +		mem_speed = memdev->speed;
> > +	else
> > +		return;
> > +
> > +	dev_priv->dmi.mem_channel++;
> > +
> > +	/* All channels are expected to have same the speed */
> > +	if (dev_priv->dmi.mem_speed == 0)
> > +		dev_priv->dmi.mem_speed = mem_speed;
> > +	else if (mem_speed != dev_priv->dmi.mem_speed)
> > +		dev_priv->dmi.valid = false;
> > +}
> 
> Dunno if this was covered already, but trusting DMI for this feels
> rather fragile to me. Isn't there some way to get the relevant
> information from the hardware itself?

Not sure.  You're right that it's fragile though; I find that the DMI
never provides valid memdev information on the BXT board I'm working
with, so the workaround always winds up disabled for me.  Not sure if
Shobhit has any other ideas for how we could get valid memory
information.


Matt

> 
> > +
> >  /**
> >   * i915_driver_load - setup chip and create an initial config
> >   * @dev: DRM device
> > @@ -1000,6 +1028,12 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  	dev->dev_private = dev_priv;
> >  	dev_priv->dev = dev;
> >  
> > +	/* walk the dmi device table for getting platform memory information */
> > +	dev_priv->dmi.valid = true;
> > +	dmi_walk(dmi_decode_memory_info, dev_priv);
> > +	if (!dev_priv->dmi.mem_speed)
> > +		dev_priv->dmi.valid = false;
> > +
> >  	/* Setup the write-once "constant" device info */
> >  	device_info = (struct intel_device_info *)&dev_priv->info;
> >  	memcpy(device_info, info, sizeof(dev_priv->info));
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index f37ac12..a2a7d8d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2022,6 +2022,13 @@ struct drm_i915_private {
> >  	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
> >  	 * will be rejected. Instead look for a better place.
> >  	 */
> > +
> > +	/* DMI data for memory bandwidth calculation */
> > +	struct {
> > +		bool valid;
> > +		uint16_t mem_channel;
> > +		int16_t mem_speed;
> > +	} dmi;
> >  };
> >  
> >  static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx