Re: [Intel-gfx] [PATCH 2/2] drm/doc: Update docs about device instance setup

[David Herrmann]

Hi

On Mon, Aug 10, 2015 at 4:30 PM, Daniel Vetter  wrote:
> On Mon, Aug 10, 2015 at 02:34:18PM +0200, Thierry Reding wrote:
>> On Mon, Aug 10, 2015 at 02:07:49PM +0200, Daniel Vetter wrote:
>> > On Mon, Aug 10, 2015 at 01:59:07PM +0200, Thierry Reding wrote:
>> > > On Mon, Aug 10, 2015 at 11:55:38AM +0200, Daniel Vetter wrote:
>> > > > ->load is depracated, bus functionst are deprecated and everyone
>> > > > should use drm_dev_alloc
>> > >
>> > > Why would you want to deprecated ->load()? Even if you use
>> > > drm_dev_alloc() and drm_dev_register(), there's still use for ->load()
>> > > because it gives you the subsystem-level initialization entry point.
>> >
>> > ->load is called after the drm /dev node is registered and hence you can't
>> > really do any driver setup in there without risking races. We paper over
>> > that using drm_global_mutex, but that doesn't work for any other
>> > driver/userspace interface like sysfs/debugfs because of deadlocks.
>> >
>> > And we can't just reorder ->load to happen before the /dev nodes are
>> > registered because a lot of drivers would fall over if we do that.
>> >
>> > This is typical midlayer fail where the core calls into the driver instead
>> > of the other way round.
>>
>> Okay, but then if we're going to deprecate ->load(), I think we should
>> also come up with an upgrade plan. As it is, this just says that users
>> shouldn't do ->load(), but it doesn't tell them what to do instead.
>>
>> Would the proper procedure be to fix drivers so that ->load() can be
>> called between drm_dev_alloc() and drm_dev_register()? I suppose we
>> could add some sort of (temporary) flag for drivers to signal this and
>> then have drm_dev_register() call ->load() at the right time. If drivers
>> don't support it, we can keep what we have.
>>
>> That, of course, doesn't get rid of the midlayer, so perhaps a better
>> way forward would be to tell driver writers that they should be doing
>> subsystem-level setup between drm_dev_alloc() and drm_dev_register().
>
> That's exactly what this patch tries to accomplish by updating the
> kerneldoc and docbook. New sequence should be
>
> device_probe_callback_or_whatever()
> {
> drm_dev_alloc();
>
> ... driver init code ...
>
> drm_dev_register();
>
> return 0;
> }

_Yes_!

Acked-by: David Herrmann 

Thanks
David
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/doc: Update docs about device instance setup

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Monday 10 August 2015 11:55:38 Daniel Vetter wrote:
> ->load is depracated, bus functionst are deprecated and everyone

s/depracated/deprecated/

s/functionst/functions/

> should use drm_dev_alloc
> 
> So update the .tmpl (and pull a bunch of the overview docs into the
> sourcecode to increase chances that it'll stay in sync in the future)
> and add notes to functions which are deprecated. I didn't bother to
> clean up and document the unload sequence similarly since that one is
> still a bit a mess: drm_dev_unregister does way too much,
> drm_unplug_dev does what _unregister should be doing but then has the
> complication of promising something it doesn't actually do (it doesn't
> unplug existing open fds for instance, only prevents new ones).
> 
> Motivated since I don't want to hunt every new driver for usage of
> drm_platform_init any more ;-)
> 
> Signed-off-by: Daniel Vetter 
> ---
>  Documentation/DocBook/drm.tmpl | 99 ---
>  drivers/gpu/drm/drm_drv.c  | 55 +--
>  drivers/gpu/drm/drm_pci.c  | 11 +
>  drivers/gpu/drm/drm_platform.c |  3 ++
>  4 files changed, 83 insertions(+), 85 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index ec34b9becebd..f1884038b90f 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -138,14 +138,10 @@
>  
>At the core of every DRM driver is a
> drm_driver structure. Drivers typically statically
> initialize a drm_driver structure, -  and then pass it to one of the
> drm_*_init() functions -  to register it with the
> DRM subsystem.
> -
> -
> -  Newer drivers that no longer require a
> drm_bus -  structure can alternatively use the
> low-level device initialization and -  registration functions such as
> drm_dev_alloc() and - 
> drm_dev_register() directly.
> +  and then pass it to drm_dev_alloc() to allocate
> a +  device instance. After the device instance is fully initialized it
> can be +  registered (which makes it accessible from userspace) using +
>  drm_dev_register().
>  
>  
>The drm_driver structure contains static
> @@ -296,83 +292,12 @@ char *date;
>
>  
>  
> -  Device Registration
> -  
> -A number of functions are provided to help with device
> registration. -The functions deal with PCI and platform devices,
> respectively. -  
> -!Edrivers/gpu/drm/drm_pci.c
> -!Edrivers/gpu/drm/drm_platform.c
> -  
> -New drivers that no longer rely on the services provided by the
> -drm_bus structure can call the low-level
> -device registration functions directly. The
> -drm_dev_alloc() function can be used to
> allocate -and initialize a new drm_device
> structure. -Drivers will typically want to perform some additional
> setup on this -structure, such as allocating driver-specific data
> and storing a -pointer to it in the DRM device's
> dev_private -field. Drivers should also
> set the device's unique name using the -   
> drm_dev_set_unique() function. After it has been -
>set up a device can be registered with the DRM subsystem by calling -   
> drm_dev_register(). This will cause the device to
> -be exposed to userspace and will call the driver's
> -.load() implementation. When a device is
> -removed, the DRM device can safely be unregistered and freed by
> calling -drm_dev_unregister() followed by a
> call to -drm_dev_unref().
> -  
> +  Device Instance and Driver Handling
> +!Pdrivers/gpu/drm/drm_drv.c driver instance overview
>  !Edrivers/gpu/drm/drm_drv.c
>  
>  
>Driver Load
> -  
> -The load method is the driver and device
> -initialization entry point. The method is responsible for
> allocating and -  initializing driver private data, performing resource
> allocation and -  mapping (e.g. acquiring
> -clocks, mapping registers or allocating command buffers),
> initializing -the memory manager ( linkend="drm-memory-management"/>), installing -the IRQ handler
> (), setting up -vertical
> blanking handling (), mode -  setting
> () and initial output
> - configuration ().
> -  
> -  
> -If compatibility is a concern (e.g. with drivers converted over
> from -User Mode Setting to Kernel Mode Setting), care must be taken
> to prevent -device initialization and control that is incompatible
> with currently -active userspace drivers. For instance, if user
> level mode setting -drivers are in use, it would be problematic to
> perform output discovery - configuration at load time.
> Likewise, if user-level drivers -unaware of memory management are
> in 

Re: [Intel-gfx] [PATCH 2/2] drm/doc: Update docs about device instance setup

[shuang . he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: 
shuang...@intel.com)
Task id: 7129
-Summary-
Platform  Delta  drm-intel-nightly  Series Applied
ILK -1  302/302  301/302
SNB  315/315  315/315
IVB  336/336  336/336
BYT  283/283  283/283
HSW  378/378  378/378
-Detailed-
Platform  Testdrm-intel-nightly  Series 
Applied
*ILK  igt@kms_flip@wf_vblank-vs-modeset-interruptible  PASS(1)  
DMESG_WARN(1)
Note: You need to pay more attention to line start with '*'
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/doc: Update docs about device instance setup

[Daniel Vetter]

On Mon, Aug 10, 2015 at 02:34:18PM +0200, Thierry Reding wrote:
 On Mon, Aug 10, 2015 at 02:07:49PM +0200, Daniel Vetter wrote:
  On Mon, Aug 10, 2015 at 01:59:07PM +0200, Thierry Reding wrote:
   On Mon, Aug 10, 2015 at 11:55:38AM +0200, Daniel Vetter wrote:
-load is depracated, bus functionst are deprecated and everyone
should use drm_dev_allocregister.
   
   Why would you want to deprecated -load()? Even if you use
   drm_dev_alloc() and drm_dev_register(), there's still use for -load()
   because it gives you the subsystem-level initialization entry point.
  
  -load is called after the drm /dev node is registered and hence you can't
  really do any driver setup in there without risking races. We paper over
  that using drm_global_mutex, but that doesn't work for any other
  driver/userspace interface like sysfs/debugfs because of deadlocks.
  
  And we can't just reorder -load to happen before the /dev nodes are
  registered because a lot of drivers would fall over if we do that.
  
  This is typical midlayer fail where the core calls into the driver instead
  of the other way round.
 
 Okay, but then if we're going to deprecate -load(), I think we should
 also come up with an upgrade plan. As it is, this just says that users
 shouldn't do -load(), but it doesn't tell them what to do instead.
 
 Would the proper procedure be to fix drivers so that -load() can be
 called between drm_dev_alloc() and drm_dev_register()? I suppose we
 could add some sort of (temporary) flag for drivers to signal this and
 then have drm_dev_register() call -load() at the right time. If drivers
 don't support it, we can keep what we have.
 
 That, of course, doesn't get rid of the midlayer, so perhaps a better
 way forward would be to tell driver writers that they should be doing
 subsystem-level setup between drm_dev_alloc() and drm_dev_register().

That's exactly what this patch tries to accomplish by updating the
kerneldoc and docbook. New sequence should be

device_probe_callback_or_whatever()
{
drm_dev_alloc();

... driver init code ...

drm_dev_register();

return 0;
}

Unfortunately the kerneldoc markdown isn't merged yet, otherwise I'd have
added this code snippet to the docs too.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/doc: Update docs about device instance setup

[Thierry Reding]

On Mon, Aug 10, 2015 at 02:07:49PM +0200, Daniel Vetter wrote:
 On Mon, Aug 10, 2015 at 01:59:07PM +0200, Thierry Reding wrote:
  On Mon, Aug 10, 2015 at 11:55:38AM +0200, Daniel Vetter wrote:
   -load is depracated, bus functionst are deprecated and everyone
   should use drm_dev_allocregister.
  
  Why would you want to deprecated -load()? Even if you use
  drm_dev_alloc() and drm_dev_register(), there's still use for -load()
  because it gives you the subsystem-level initialization entry point.
 
 -load is called after the drm /dev node is registered and hence you can't
 really do any driver setup in there without risking races. We paper over
 that using drm_global_mutex, but that doesn't work for any other
 driver/userspace interface like sysfs/debugfs because of deadlocks.
 
 And we can't just reorder -load to happen before the /dev nodes are
 registered because a lot of drivers would fall over if we do that.
 
 This is typical midlayer fail where the core calls into the driver instead
 of the other way round.

Okay, but then if we're going to deprecate -load(), I think we should
also come up with an upgrade plan. As it is, this just says that users
shouldn't do -load(), but it doesn't tell them what to do instead.

Would the proper procedure be to fix drivers so that -load() can be
called between drm_dev_alloc() and drm_dev_register()? I suppose we
could add some sort of (temporary) flag for drivers to signal this and
then have drm_dev_register() call -load() at the right time. If drivers
don't support it, we can keep what we have.

That, of course, doesn't get rid of the midlayer, so perhaps a better
way forward would be to tell driver writers that they should be doing
subsystem-level setup between drm_dev_alloc() and drm_dev_register().

Thierry


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/doc: Update docs about device instance setup

[Thierry Reding]

On Mon, Aug 10, 2015 at 11:55:38AM +0200, Daniel Vetter wrote:
 -load is depracated, bus functionst are deprecated and everyone
 should use drm_dev_allocregister.

Why would you want to deprecated -load()? Even if you use
drm_dev_alloc() and drm_dev_register(), there's still use for -load()
because it gives you the subsystem-level initialization entry point.

Thierry


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/doc: Update docs about device instance setup

[Daniel Vetter]

On Mon, Aug 10, 2015 at 01:59:07PM +0200, Thierry Reding wrote:
 On Mon, Aug 10, 2015 at 11:55:38AM +0200, Daniel Vetter wrote:
  -load is depracated, bus functionst are deprecated and everyone
  should use drm_dev_allocregister.
 
 Why would you want to deprecated -load()? Even if you use
 drm_dev_alloc() and drm_dev_register(), there's still use for -load()
 because it gives you the subsystem-level initialization entry point.

-load is called after the drm /dev node is registered and hence you can't
really do any driver setup in there without risking races. We paper over
that using drm_global_mutex, but that doesn't work for any other
driver/userspace interface like sysfs/debugfs because of deadlocks.

And we can't just reorder -load to happen before the /dev nodes are
registered because a lot of drivers would fall over if we do that.

This is typical midlayer fail where the core calls into the driver instead
of the other way round.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx