Re: fixing up DRM device model usage
On Fri, 2007-10-26 at 14:31 -0700, Jesse Barnes wrote: > On Friday, October 26, 2007 12:08 pm Kay Sievers wrote: > > The open coded: device_create_file(&dev->dev, &device_attrs[i]) > > should probably replaced by passing the array to the class, and the > > core will do that for you. > > You mean just set drm_class->dev_attrs = device_attrs? I didn't see in > the core device model code where that would create the files... Yeah, it is in device_add_attrs() in drivers/base/core.c. Kay - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: fixing up DRM device model usage
On Friday, October 26, 2007 12:08 pm Kay Sievers wrote:
> > How does this conversion look?
>
> Seems fine, at a first look. You moved the device structure into the
> object where it belongs, instead of allocating one, and saving the
> pointer. You should really considering changing the core to do the
> free()ing of your object with the embedded devices release function,
> that is called when the last reference to the object is gone, instead
> of "hoping the best". :) But if the drm core does that properly, it
> might work, sure.
Yeah, but that'll require other changes to the DRM. Maybe I can tackle
that as part of the refactoring I'm doing for some other work.
> The open coded: device_create_file(&dev->dev, &device_attrs[i])
> should probably replaced by passing the array to the class, and the
> core will do that for you.
You mean just set drm_class->dev_attrs = device_attrs? I didn't see in
the core device model code where that would create the files...
> Do you assign the dev_t: MKDEV(DRM_MAJOR, head->minor) somewhere? You
> need to put it in dev->devt, if you want a device node created by
> userspace.
Yeah, that's part of the drm_head structure. I'll go ahead and assign
it to dev->devt too.
> > It retains directory compatibility with
> > the old scheme, but I'm not sure about the dev->dev.parent value.
>
> You should use the same value as the old code:
> &(head->dev->pdev)->dev
> and assign it as the parent, seems right..
>
> > I'm using the PCI device corresponding to the DRM device as the
> > parent, but maybe I don't need one at all?
>
> Keep it, you want to express the relationship in sysfs, so that a
> "device" link is created, or that the device directory lives as a
> child below the parent device. Seems fine so far.
Ok, sounds good.
>
> > Dave, the drm_head stuff is a bit funky; it seems like a partially
> > implemented feature? I wonder if we should rip that out too, just
> > to keep things simple...
>
> Hehe, that's always a solution. :)
Yeah, removing code is nearly always a win. :)
Thanks,
Jesse
diff --git a/linux-core/drmP.h b/linux-core/drmP.h
index d0ab2c9..82a3a23 100644
--- a/linux-core/drmP.h
+++ b/linux-core/drmP.h
@@ -619,6 +619,8 @@ struct drm_driver {
void (*postclose) (struct drm_device *, struct drm_file *);
void (*lastclose) (struct drm_device *);
int (*unload) (struct drm_device *);
+ int (*suspend) (struct drm_device *);
+ int (*resume) (struct drm_device *);
int (*dma_ioctl) (struct drm_device *dev, void *data, struct drm_file
*file_priv);
void (*dma_ready) (struct drm_device *);
int (*dma_quiescent) (struct drm_device *);
@@ -697,6 +699,7 @@ struct drm_head {
* may contain multiple heads.
*/
struct drm_device {
+ struct device dev; /**< Linux device */
char *unique; /**< Unique identifier: e.g., busid */
int unique_len; /**< Length of unique field */
char *devname; /**< For /proc/interrupts */
@@ -1163,10 +1166,9 @@ extern void drm_pci_free(struct drm_device *dev,
drm_dma_handle_t *dmah);
/* sysfs support (drm_sysfs.c) */
struct drm_sysfs_class;
extern struct class *drm_sysfs_create(struct module *owner, char *name);
-extern void drm_sysfs_destroy(struct class *cs);
-extern struct class_device *drm_sysfs_device_add(struct class *cs,
-struct drm_head * head);
-extern void drm_sysfs_device_remove(struct class_device *class_dev);
+extern void drm_sysfs_destroy(void);
+extern int drm_sysfs_device_add(struct drm_device *dev, struct drm_head *
head);
+extern void drm_sysfs_device_remove(struct drm_device *dev);
/*
* Basic memory manager support (drm_mm.c)
diff --git a/linux-core/drm_drv.c b/linux-core/drm_drv.c
index fe2b120..47d1765 100644
--- a/linux-core/drm_drv.c
+++ b/linux-core/drm_drv.c
@@ -519,7 +519,7 @@ static int __init drm_core_init(void)
CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL, CORE_DATE);
return 0;
err_p3:
- drm_sysfs_destroy(drm_class);
+ drm_sysfs_destroy();
err_p2:
unregister_chrdev(DRM_MAJOR, "drm");
drm_free(drm_heads, sizeof(*drm_heads) * drm_cards_limit, DRM_MEM_STUB);
@@ -530,7 +530,7 @@ err_p1:
static void __exit drm_core_exit(void)
{
remove_proc_entry("dri", NULL);
- drm_sysfs_destroy(drm_class);
+ drm_sysfs_destroy();
unregister_chrdev(DRM_MAJOR, "drm");
diff --git a/linux-core/drm_stub.c b/linux-core/drm_stub.c
index 9e140ac..1d88d37 100644
--- a/linux-core/drm_stub.c
+++ b/linux-core/drm_stub.c
@@ -183,11 +183,10 @@ static int drm_get_head(struct drm_device * dev, struct
drm_head * head)
goto err_g1;
}
- head->dev_class = drm_sysfs_device_add(drm_class, head);
- if (IS_ERR(head->dev_class)) {
+
Re: fixing up DRM device model usage
On Fri, 2007-10-26 at 11:40 -0700, Jesse Barnes wrote: > On Friday, October 26, 2007 10:10 am Kay Sievers wrote: > > On 10/26/07, Jesse Barnes <[EMAIL PROTECTED]> wrote: > > > On Thursday, October 25, 2007 9:59 pm Greg KH wrote: > > > > On Thu, Oct 25, 2007 at 04:53:18PM -0700, Jesse Barnes wrote: > > > > > Ok, here's yet another version that uses the device model for > > > > > the suspend/resume, rather than pci hooks. > > > > > > > > > > Greg, DRM desperately needs review of its device model usage, > > > > > can you take a look at this patch and the current drm_sysfs.c > > > > > code? Right now, we're mixing class_devices and regular devices > > > > > (the latter seem to be required for suspend/resume to work > > > > > correctly), but this seems wrong. Any ideas? Should we just > > > > > rip out the class_device stuff and create full-on DRM device > > > > > nodes? > > > > > > > > The class_device stuff is already ripped out in the latest -mm > > > > trees and I will be forwarding that change on for 2.6.25 after > > > > 2.6.24 is out. So yes, it should be taken away :) > > > > > > > > But converting from class_device to struct device does not mean > > > > you use a "device node". But you could if you want to :) > > > > > > Yeah, bad choice of words. :) > > > > > > To retain compatibility, we need to have directories under the DRM > > > class dir (/sys/class/drm) for each card (e.g. card0) that contains > > > a file describing which graphics driver is bound to the device. > > > For class devices, we could just add an attributes structure to the > > > device. Can we do the same with regular, non-class devices? > > > > The conversion is already queued in Greg's tree, and in -mm: > > http://git.kernel.org/?p=linux/kernel/git/gregkh/patches.git;a=blob;f > >=driver/drm-convert-from-class_device-to-device-in-drivers-char-drm.pa > >tch;h=f993183d1cb017f981cc2232d17930af40459bd8;hb=HEAD > > > > /sys/class/drm will look the same as with the class_device's, only if > > !CONFIG_SYSFS_DEPRECATED, there will be symlinks instead of > > directories, otherwise the same pathes, like for all other > > (converted) classes too. > > How does this conversion look? Seems fine, at a first look. You moved the device structure into the object where it belongs, instead of allocating one, and saving the pointer. You should really considering changing the core to do the free()ing of your object with the embedded devices release function, that is called when the last reference to the object is gone, instead of "hoping the best". :) But if the drm core does that properly, it might work, sure. The open coded: device_create_file(&dev->dev, &device_attrs[i]) should probably replaced by passing the array to the class, and the core will do that for you. Do you assign the dev_t: MKDEV(DRM_MAJOR, head->minor) somewhere? You need to put it in dev->devt, if you want a device node created by userspace. > It retains directory compatibility with > the old scheme, but I'm not sure about the dev->dev.parent value. You should use the same value as the old code: &(head->dev->pdev)->dev and assign it as the parent, seems right.. > I'm using the PCI device corresponding to the DRM device as the parent, but > maybe I don't need one at all? Keep it, you want to express the relationship in sysfs, so that a "device" link is created, or that the device directory lives as a child below the parent device. Seems fine so far. > Dave, the drm_head stuff is a bit funky; it seems like a partially > implemented feature? I wonder if we should rip that out too, just to > keep things simple... Hehe, that's always a solution. :) Kay - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: fixing up DRM device model usage
On Friday, October 26, 2007 10:10 am Kay Sievers wrote:
> On 10/26/07, Jesse Barnes <[EMAIL PROTECTED]> wrote:
> > On Thursday, October 25, 2007 9:59 pm Greg KH wrote:
> > > On Thu, Oct 25, 2007 at 04:53:18PM -0700, Jesse Barnes wrote:
> > > > Ok, here's yet another version that uses the device model for
> > > > the suspend/resume, rather than pci hooks.
> > > >
> > > > Greg, DRM desperately needs review of its device model usage,
> > > > can you take a look at this patch and the current drm_sysfs.c
> > > > code? Right now, we're mixing class_devices and regular devices
> > > > (the latter seem to be required for suspend/resume to work
> > > > correctly), but this seems wrong. Any ideas? Should we just
> > > > rip out the class_device stuff and create full-on DRM device
> > > > nodes?
> > >
> > > The class_device stuff is already ripped out in the latest -mm
> > > trees and I will be forwarding that change on for 2.6.25 after
> > > 2.6.24 is out. So yes, it should be taken away :)
> > >
> > > But converting from class_device to struct device does not mean
> > > you use a "device node". But you could if you want to :)
> >
> > Yeah, bad choice of words. :)
> >
> > To retain compatibility, we need to have directories under the DRM
> > class dir (/sys/class/drm) for each card (e.g. card0) that contains
> > a file describing which graphics driver is bound to the device.
> > For class devices, we could just add an attributes structure to the
> > device. Can we do the same with regular, non-class devices?
>
> The conversion is already queued in Greg's tree, and in -mm:
> http://git.kernel.org/?p=linux/kernel/git/gregkh/patches.git;a=blob;f
>=driver/drm-convert-from-class_device-to-device-in-drivers-char-drm.pa
>tch;h=f993183d1cb017f981cc2232d17930af40459bd8;hb=HEAD
>
> /sys/class/drm will look the same as with the class_device's, only if
> !CONFIG_SYSFS_DEPRECATED, there will be symlinks instead of
> directories, otherwise the same pathes, like for all other
> (converted) classes too.
How does this conversion look? It retains directory compatibility with
the old scheme, but I'm not sure about the dev->dev.parent value. I'm
using the PCI device corresponding to the DRM device as the parent, but
maybe I don't need one at all?
Dave, the drm_head stuff is a bit funky; it seems like a partially
implemented feature? I wonder if we should rip that out too, just to
keep things simple...
Thanks,
Jesse
diff --git a/linux-core/Kconfig b/linux-core/Kconfig
diff --git a/linux-core/drmP.h b/linux-core/drmP.h
index d0ab2c9..82a3a23 100644
--- a/linux-core/drmP.h
+++ b/linux-core/drmP.h
@@ -619,6 +619,8 @@ struct drm_driver {
void (*postclose) (struct drm_device *, struct drm_file *);
void (*lastclose) (struct drm_device *);
int (*unload) (struct drm_device *);
+ int (*suspend) (struct drm_device *);
+ int (*resume) (struct drm_device *);
int (*dma_ioctl) (struct drm_device *dev, void *data, struct drm_file
*file_priv);
void (*dma_ready) (struct drm_device *);
int (*dma_quiescent) (struct drm_device *);
@@ -697,6 +699,7 @@ struct drm_head {
* may contain multiple heads.
*/
struct drm_device {
+ struct device dev; /**< Linux device */
char *unique; /**< Unique identifier: e.g., busid */
int unique_len; /**< Length of unique field */
char *devname; /**< For /proc/interrupts */
@@ -1163,10 +1166,9 @@ extern void drm_pci_free(struct drm_device *dev,
drm_dma_handle_t *dmah);
/* sysfs support (drm_sysfs.c) */
struct drm_sysfs_class;
extern struct class *drm_sysfs_create(struct module *owner, char *name);
-extern void drm_sysfs_destroy(struct class *cs);
-extern struct class_device *drm_sysfs_device_add(struct class *cs,
-struct drm_head * head);
-extern void drm_sysfs_device_remove(struct class_device *class_dev);
+extern void drm_sysfs_destroy(void);
+extern int drm_sysfs_device_add(struct drm_device *dev, struct drm_head *
head);
+extern void drm_sysfs_device_remove(struct drm_device *dev);
/*
* Basic memory manager support (drm_mm.c)
diff --git a/linux-core/drm_drv.c b/linux-core/drm_drv.c
index fe2b120..47d1765 100644
--- a/linux-core/drm_drv.c
+++ b/linux-core/drm_drv.c
@@ -519,7 +519,7 @@ static int __init drm_core_init(void)
CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL, CORE_DATE);
return 0;
err_p3:
- drm_sysfs_destroy(drm_class);
+ drm_sysfs_destroy();
err_p2:
unregister_chrdev(DRM_MAJOR, "drm");
drm_free(drm_heads, sizeof(*drm_heads) * drm_cards_limit, DRM_MEM_STUB);
@@ -530,7 +530,7 @@ err_p1:
static void __exit drm_core_exit(void)
{
remove_proc_entry("dri", NULL);
- drm_sysfs_destroy(drm_class);
+ drm_sysfs_destroy();
unregister_chrdev(DRM_MAJOR, "drm");
diff --git a/linux-core/drm_stub
