Re: [PATCH v2 17/17] drm/i915: Split out load time interface registration

[Imre Deak <imre.deak@intel.com>]

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@chris-wilson.co.uk>
> > Signed-off-by: Imre Deak <imre.deak@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?

--Imre

> Anyway the above should give us better coverage of these error paths
> and
> so hopefully prevent a panic-on-boot for some unsuspecting users.
> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx