Re: [PATCH 13/14] drm/i915: don't set up gem ring functions on gen5 for !kms

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Apr 12, 2012 at 06:05:14PM -0700, Ben Widawsky wrote:
> On Wed, 11 Apr 2012 22:12:58 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > We already disallow initialition of gem in this case in the
> > corresponding ioctl, so don't bother setting up the gem support ring
> > functions in the legacy dri render ring init.
> > 
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_ringbuffer.c |   24 ++++++++++--------------
> >  1 files changed, 10 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 111981a..108bfd6 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -1336,21 +1336,17 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
> >  	if (INTEL_INFO(dev)->gen >= 6) {
> >  		/* non-kms not supported on gen6+ */
> >  		return -ENODEV;
> > -	} else if (IS_GEN5(dev)) {
> > -		ring->add_request = pc_render_add_request;
> > -		ring->flush = render_ring_flush;
> > -		ring->get_seqno = pc_render_get_seqno;
> > -		ring->irq_get = gen5_ring_get_irq;
> > -		ring->irq_put = gen5_ring_put_irq;
> > -		ring->irq_enable_mask = GT_USER_INTERRUPT | GT_PIPE_NOTIFY;
> > -	} else {
> > -		ring->add_request = i9xx_add_request;
> > -		ring->flush = render_ring_flush;
> > -		ring->get_seqno = ring_get_seqno;
> > -		ring->irq_get = i9xx_ring_get_irq;
> > -		ring->irq_put = i9xx_ring_put_irq;
> > -		ring->irq_enable_mask = I915_USER_INTERRUPT;
> >  	}
> > +
> > +	/* Note: gem is not supported on gen5/ilk without kms (the corresponding
> > +	 * gem_init ioctl returns with -ENODEV). Hence we do not need to set up
> > +	 * the special gen5 functions. */
> Can we make the check >= 5 above? I think doing so would remove the need
> for this comment.

We can bail out for gen6+ because the driver will flat-out refuse to load,
so we can never actually hit this path. For gen5 we cannot refuse to load
and hence can't fail ring init (otoh it doesn't matter how broken the ring
is). Imo we have two options: Either try not to initialize any of the ring
stuff for gen5 and carefully auditing that this does not have any bad side
effects for vt-switching, suspend/resume where we might fall over a NULL
render ring. Or just initialize it half-assed and add this comment. I've
figured the latter is the less risky option and went with it.
-Daniel

> > +	ring->add_request = i9xx_add_request;
> > +	ring->flush = render_ring_flush;
> > +	ring->get_seqno = ring_get_seqno;
> > +	ring->irq_get = i9xx_ring_get_irq;
> > +	ring->irq_put = i9xx_ring_put_irq;
> > +	ring->irq_enable_mask = I915_USER_INTERRUPT;
> >  	ring->write_tail = ring_write_tail;
> >  	if (INTEL_INFO(dev)->gen >= 4)
> >  		ring->dispatch_execbuffer = i965_dispatch_execbuffer;
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48