[Intel-gfx] [PATCH v2 17/17] drm/i915: Split out load time interface registration

[Imre Deak]

On Fri, 2016-03-11 at 20:38 +0000, Chris Wilson wrote:
> On Fri, Mar 11, 2016 at 10:11:20PM +0200, Imre Deak wrote:
> > On Fri, 2016-03-11 at 19:55 +0000, Chris Wilson wrote:
> > > On Fri, Mar 11, 2016 at 06:31:42PM +0200, Imre Deak wrote:
> > > > According to the new init phases scheme we should register the
> > > > device
> > > > making it available via some kernel internal or user space
> > > > interface as
> > > > the last step in the init sequence, so move the corresponding
> > > > code
> > > > to a
> > > > separate function.
> > > > 
> > > > Also add a TODO comment about code that still needs to be moved
> > > > around
> > > > to one of the init phases functions depending on what the role
> > > > and
> > > > effect
> > > > of that code is.
> > > > 
> > > > No functional change, except for the reordering of the unload
> > > > time
> > > > unregistration steps of sysfs wrt. acpi and opregion.
> > > > 
> > > > Suggested by Chris.
> > > > 
> > > > CC: Chris Wilson <chris at chris-wilson.co.uk>
> > > > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_dma.c | 83
> > > > +++++++++++++++++++++++++++
> > > > --------------
> > > >  1 file changed, 54 insertions(+), 29 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c
> > > > b/drivers/gpu/drm/i915/i915_dma.c
> > > > index aaf1b17..43dcb5a 100644
> > > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > > @@ -1209,6 +1209,53 @@ static void
> > > > i915_driver_cleanup_hw(struct
> > > > drm_i915_private *dev_priv)
> > > >  }
> > > >  
> > > >  /**
> > > > + * i915_driver_init_register - register the driver with the
> > > > rest
> > > > of the system
> > > > + * @dev_priv: device private
> > > > + *
> > > > + * Perform any steps necessary to make the driver available
> > > > via
> > > > kernel
> > > > + * internal or userspace interfaces.
> > > > + */
> > > > +static void i915_driver_init_register(struct drm_i915_private
> > > > *dev_priv)
> > > 
> > > This is the only one I didn't like. The problem with _register is
> > > that
> > > it makes me think of mmio register (yes, I know that's too
> > > driver-centric and the use of register_callback_interface is
> > > widespread.) A compromise would be
> > > 
> > > i915_driver_init_frameworks()
> > 
> > Ok, can rename it.
> > 
> > > Other than I got to here without spotting anything obnoxious or
> > > troublematic wrt to mmio/gem. The only thing we lack is fault
> > > injection
> > > into igt/drv_module_reload, maybe we can do
> > > 	i915.inject_fault = LOAD_MMIO | LOAD_FRAMEWORK etc
> > > The other tricky part is deciding on what the failure should be.
> > > For
> > > critical faults we just expect the module to fail to load, but
> > > for
> > > aspects like GEM, we just want to the GPU to be disabled but
> > > modesetting
> > > still work.
> > 
> > Yep sounds useful, but can we do it as a follow-up?
> 
> I'd rather not. The question is how much of this churn is covered by
> igt? I think the answer is scarily low, since half of this is error
> path
> during init. Adding a module paramter and then checking bits in each
> of
> the new phase functions is going to be a relatively simple job and
> lets
> us have a little more confidence that the changes + fixes are solid.

Yes not much coverage, but that means that probably there are already
existing issues. I specifically avoided touching any of the problematic
parts (in the modesetting init function) and I don't see anything in
this patchset that would make things worse (as you also pointed out).
Realistically, fixing up all the error path handling in the modesetting
path would be quite a bit of work. Otoh, this patchset is a dependency
on other PM related stuff that is quite late already, so I'd like to
progress with that. That's why I suggested to fix up the other parts
later.

--Imre

> The tricker part would be adding the loop over insmod i915.ko into
> drv_module_reload_basic, but well worth the bugs it is likely to
> uncover.
> -Chris
>