Re: [Dri-devel] Re: DRM and pci_driver conversion

Wouldn't it be better to add ROM enable/disable functions to the PCI driver
than to scatter it out into every driver? All of the framebuffer and DRM
drivers need to do this. I also seem to remember that there are more steps
needed if this is going to work on an ARM chip.

--- Linus Torvalds <torvalds@osdl.org> wrote:

[ Jeff: is that PCI ROM enable _really_ that complicated? Ouch. Or is
  there some helper function I missed? ]

On Thu, 23 Oct 2003, Jon Smirl wrote:

quoted

I don't think DRM drivers are doing things correctly yet. DRM is missing

the

quoted

code for marking PCI resources as being in use while DRM is using them.

This

quoted

could lead to problems with hotplug.  XFree is also mapping PCI ROMs in

without

quoted

informing the kernel and that can definitely cause problems.

Absolutely. Changing PCI configurations without telling the kernel _will_ 
cause problems. Especially for hotplug systems, but it's also very iffy to 
do if the card is behind a PCI bridge, since you have to take bridge 
resources into account (and know which bridges are transparent, which are 
not, etc etc). 

The kernel spends a lot of effort on getting this right, and even so it 
fails every once in a while (devices that use IO or memory regions without 
announcing them in the standard BAR's are quite common, and the kernel has 
to have special quirk entries for things like that).

Few enough drivers actually want to enable the roms, but the code should 
look something like

	/* Assign space for ROM resource if not already assigned. Ugly. */
	if (!pci_resource_start(dev, PCI_ROM_RESOURCE))
		if (pci_assign_resource(dev, PCI_ROM_RESOURCE) < 0)
			return -ENOMEM;

	/* Enable it. This is too ugly */
	if (!(pci_resource_flags(dev, PCI_ROM_RESOURCE) & PCI_ROM_ADDRESS_ENABLE)) {
		u32 val;
		pci_read_config_dword(dev, PCI_ROM_ADDRESS, &val);
		val |= PCI_ROM_ADDRESS_ENABLE;
		pci_write_config_dword(dev, PCI_ROM_ADDRESS, val);
		pci_resource_flags(dev, PCI_ROM_RESOURCE) |= PCI_ROM_ADDRESS_ENABLE;
	}


	/* Enable the device, and regular resources */
	if (pci_enable_device(dev))
		return -ENODEV;

	pci_set_master(dev);	/* If we want to */
	pci_set_mwi(dev);	/* If we want to */

(Yeah, that is more complex than it really should need to be. That's just 
a sign of exactly how few device drivers tend to want to do this: the 
usual helper stuff is all geared for the normal case).

quoted

new style probe
if (new probe has device)
   mark resources in use

You shouldn't need to mark the resources in use. Just registering the 
driver should do everything for you, including making sure that no other 
driver will register that device.

Of course, if you are worried about mixing with drivers that use the old
"find device and just use it", then yes, you'll need to mark the resources 
in use. That can be as trivial as just doing a

	if (pci_request_regions(dev, "drivername") < 0)
		return -EAIIEEEE!;

in the probe function (but then you need to remember to release them in 
the drop function).

			Linus

The framebuffer drivers are doing it like this. Should they be replaced with
pci_request_regions?

	/* request the mem regions */
	if (!request_mem_region (rinfo->fb_base_phys,
				 pci_resource_len(pdev, 0), "radeonfb")) {
		printk (KERN_ERR "radeonfb: cannot reserve FB region\n");
		goto free_rinfo;
	}

	if (!request_mem_region (rinfo->mmio_base_phys,
				 pci_resource_len(pdev, 2), "radeonfb")) {
		printk (KERN_ERR "radeonfb: cannot reserve MMIO region\n");
		goto release_fb;
	}



=====
Jon Smirl
jonsmirl@yahoo.com

__________________________________
Do you Yahoo!?
The New Yahoo! Shopping - with improved product search
http://shopping.yahoo.com


-------------------------------------------------------
This SF.net email is sponsored by: The SF.net Donation Program.
Do you like what SourceForge.net is doing for the Open
Source Community?  Make a contribution, and help us add new
features and functionality. Click here: http://sourceforge.net/donate/