Re: [RFC PATCH v2 1/4] drm/i915: add i915_ved.c to setup bridge for VED

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Oct 22, 2014 at 07:09:11AM +0000, Cheng, Yao wrote:
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Tuesday, October 21, 2014 6:30 PM
> > To: Cheng, Yao
> > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Kelley,
> > Sean V; Vetter, Daniel; Abel, Michael J; Jiang, Fei; Rao, Ram R
> > Subject: Re: [Intel-gfx] [RFC PATCH v2 1/4] drm/i915: add i915_ved.c to setup
> > bridge for VED
> > 
> > On Tue, Oct 21, 2014 at 02:36:41PM +0800, Yao Cheng wrote:
<snip> 
> > 
> > >  		/* Call regardless, as some status bits might not be
> > >  		 * signalled in iir */
> > >  		valleyview_pipestat_irq_handler(dev, iir); diff --git
> > > a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 2ed02c3..95421ef 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -1284,6 +1284,11 @@ enum punit_power_well {  #define
> > > VLV_DISPLAY_BASE 0x180000  #define VLV_MIPI_BASE
> > VLV_DISPLAY_BASE
> > >
> > > +/* forwarding VED irq and sharing MMIO to the VED driver */
> > > +#define VLV_VED_BLOCK_INTERRUPT			(1 << 23)
> > 
> > This define should be alongside the other IxR bits.
> 
> Do you mean like this:
> Rename it to VLV_VED_IRQ and put alongside VLV_IIR?

No, I think the name is fine as is, but it should be where the other "old"
IxR bits are defined. So between I915_PM_INTERRUPT and I915_ISP_INTERRUPT
looks like right spot to me.

I'm not sure why those defines are where they are. We should probably
move the lot to sit next to the IxR register defines themselves, but
that's a separate issue.

<snip>
> > 
> > > +	if (!rsc) {
> > > +		DRM_ERROR("Failed to allocate resource for VED platform
> > device\n");
> > > +		ret = -ENOMEM;
> > > +		goto err;
> > > +	}
> > > +
> > > +	WARN_ON(dev_priv->ved_irq < 0);
> > > +	rsc[0].start    = rsc[0].end = dev_priv->ved_irq;
> > > +	rsc[0].flags    = IORESOURCE_IRQ;
> > > +	rsc[0].name     = "ipvr-ved-vlv-irq";
> > > +
> > > +	/* MMIO/REG for child's use */
> > > +	rsc[1].start    = pci_resource_start(dev->pdev, 0);
> > > +	rsc[1].end      = pci_resource_start(dev->pdev, 0) + 2*1024*1024; /*
> > gen7 */
> > > +	rsc[1].flags    = IORESOURCE_MEM;
> > > +	rsc[1].name     = "ipvr-ved-vlv-mmio";
> > > +
> > > +	rsc[2].start    = VLV_VED_BASE;
> > > +	rsc[2].end      = VLV_VED_BASE + VLV_VED_SIZE;
> > > +	rsc[2].flags    = IORESOURCE_REG;
> > > +	rsc[2].name     = "ipvr-ved-vlv-reg";
> > 
> > I don't see why you need both MEM and REG resources. Just the MEM itself
> > should suffice:
> > 
> > rsc[1].start    = pci_resource_start(dev->pdev, 0) + VLV_VED_BASE;
> > rsc[1].end      = pci_resource_start(dev->pdev, 0) + VLV_VED_BASE +
> > VLV_VED_SIZE;
> > rsc[1].flags    = IORESOURCE_MEM;
> > rsc[1].name     = "ipvr-ved-vlv-mmio";
> > 
> 
> When I write the original code on k3.10 I always got ioremap conflict by binding only one MMIO resource.
> I just tested this on k3.17 and no conflict. :) thanks for pointing out this and I will update the code.
> BTW, it's interesting, is there any update on the ioremap code from 3.10 to 3.17?

Not sure. But the UC vs. WC could be one explanation for the conflict.

> 
> > Also in the ved driver you're mapping it with ioremap_wc() which isn't
> > generally safe to do with registers. I'm not sure the kernel would even allow
> > it since i915 has already mapped it as UC.
> 
> Thanks, I'll change it to UC.

-- 
Ville Syrjälä
Intel OTC