Message ID | 1334158428-23735-1-git-send-email-s.hauer@pengutronix.de |
---|---|
State | New |
Headers | show |
> +static int sdrm_suspend(struct drm_device *drm, pm_message_t state) > +{ > + /* TODO */ > + > + return 0; > +} > + > +static int sdrm_resume(struct drm_device *drm) > +{ > + /* TODO */ > + > + return 0; > +} These probably need to call into the sdrm device specific handling. > +static int sdrm_get_irq(struct drm_device *dev) > +{ > + /* > + * Return an arbitrary number to make the core happy. > + * We can't return anything meaningful here since drm > + * devices in general have multiple irqs > + */ > + return 1234; > +} If there isn't a meaningful IRQ then surely 0 should be returned. Actually I'd suggest returning sdrm->irq or similar, because some simple DRM type use cases will have a single IRQ (notably 2 on older PC hardware) > + * sdrm_device_get - find or allocate sdrm device with unique name > + * > + * This function returns the sdrm device with the unique name 'name' > + * If this already exists, return it, otherwise allocate a new > + * object. This naming is a bit confusing because the kernel mid layers etc tend to use _get and _put for ref counting not lookup ? > + /* > + * enable drm irq mode. > + * - with irq_enabled = 1, we can use the vblank feature. > + * > + * P.S. note that we wouldn't use drm irq handler but > + * just spsdrmific driver own one instead bsdrmause > + * drm framework supports only one irq handler and > + * drivers can well take care of their interrupts > + */ > + drm->irq_enabled = 1; We've got a couple of assumptions here I think I'd question for generality 1. That its a platform device 2. That it can't use the standard IRQ helpers in some cases. Probably it should take a struct device and a struct of the bits you'd fish out from platform or pci or other device type. And yes probably there would be a platform_ version that wraps it. > +static int sdrm_fb_dirty(struct drm_framebuffer *fb, > + struct drm_file *file_priv, unsigned flags, > + unsigned color, struct drm_clip_rect *clips, > + unsigned num_clips) > +{ > + /* TODO */ > + > + return 0; > +} Probably a helper method. > +static struct fb_ops sdrm_fb_ops = { > + .owner = THIS_MODULE, > + .fb_fillrect = cfb_fillrect, > + .fb_copyarea = cfb_copyarea, > + .fb_imageblit = cfb_imageblit, > + .fb_check_var = drm_fb_helper_check_var, > + .fb_set_par = drm_fb_helper_set_par, > + .fb_blank = drm_fb_helper_blank, > + .fb_pan_display = drm_fb_helper_pan_display, > + .fb_setcmap = drm_fb_helper_setcmap, > +}; If you re assuming any kind of gtt then you should probably allow for gtt based scrolling eventually, but thats an optimisation. > +int sdrm_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf) > +{ > + struct drm_gem_object *obj = vma->vm_private_data; > + struct sdrm_gem_obj *sdrm_gem_obj = to_sdrm_gem_obj(obj); > + struct drm_device *dev = obj->dev; > + unsigned long pfn; > + pgoff_t page_offset; > + int ret; For dumb hardware take a look how gma500 and some other bits do this - you can premap the entire buffer when you take the first fault, which for a dumb fb is a good bet. Looking at it from the point of view of x86 legacy devices then the things I see are - Device is quite possibly PCI (but may be platform eg vesa) - Memory will probably be allocated in the PCI space - Mappings are probably write combining but not on all hardware There's probably a case for pinning/unpinning scanout buffers according to whether they are used. On some hardware the io mapping needed is a precious resource. Also for stuff with a fixed fb space it means you can combine it with invalidating the mmap mappings of an object and copying objects in/out of the frame buffer to provide the expected interfaces to allocate/release framebuffers. Alan
On Wed, Apr 11, 2012 at 09:22:47PM +0100, Alan Cox wrote: > > +static int sdrm_suspend(struct drm_device *drm, pm_message_t state) > > +{ > > + /* TODO */ > > + > > + return 0; > > +} > > + > > +static int sdrm_resume(struct drm_device *drm) > > +{ > > + /* TODO */ > > + > > + return 0; > > +} > > These probably need to call into the sdrm device specific handling. > > > > +static int sdrm_get_irq(struct drm_device *dev) > > +{ > > + /* > > + * Return an arbitrary number to make the core happy. > > + * We can't return anything meaningful here since drm > > + * devices in general have multiple irqs > > + */ > > + return 1234; > > +} > > If there isn't a meaningful IRQ then surely 0 should be returned. > Actually I'd suggest returning sdrm->irq or similar, because some simple > DRM type use cases will have a single IRQ (notably 2 on older PC hardware) Hm, At the moment I can't even trigger this function to be called. I can simply return 0 here. Returning a real irq does not sound sane since I want the interrupt handled internally. Noone else has any business using it. > > > + * sdrm_device_get - find or allocate sdrm device with unique name > > + * > > + * This function returns the sdrm device with the unique name 'name' > > + * If this already exists, return it, otherwise allocate a new > > + * object. > > This naming is a bit confusing because the kernel mid layers etc tend to > use _get and _put for ref counting not lookup ? Ok, lookup sounds better. Will rename it. > > > > + /* > > + * enable drm irq mode. > > + * - with irq_enabled = 1, we can use the vblank feature. > > + * > > + * P.S. note that we wouldn't use drm irq handler but > > + * just spsdrmific driver own one instead bsdrmause > > + * drm framework supports only one irq handler and > > + * drivers can well take care of their interrupts > > + */ > > + drm->irq_enabled = 1; > > We've got a couple of assumptions here I think I'd question for generality > > 1. That its a platform device > 2. That it can't use the standard IRQ helpers in some cases. > > Probably it should take a struct device and a struct of the bits you'd > fish out from platform or pci or other device type. And yes probably > there would be a platform_ version that wraps it. I had a look and it turned out that I don't need anything specific to a platform_device, so I can simply pass in a regular struct device here. Having a platform_device here seems to be a leftover from earlier versions in which I used the drm_platform stubs. > > > > +static int sdrm_fb_dirty(struct drm_framebuffer *fb, > > + struct drm_file *file_priv, unsigned flags, > > + unsigned color, struct drm_clip_rect *clips, > > + unsigned num_clips) > > +{ > > + /* TODO */ > > + > > + return 0; > > +} > > Probably a helper method. Yes. > > > +static struct fb_ops sdrm_fb_ops = { > > + .owner = THIS_MODULE, > > + .fb_fillrect = cfb_fillrect, > > + .fb_copyarea = cfb_copyarea, > > + .fb_imageblit = cfb_imageblit, > > + .fb_check_var = drm_fb_helper_check_var, > > + .fb_set_par = drm_fb_helper_set_par, > > + .fb_blank = drm_fb_helper_blank, > > + .fb_pan_display = drm_fb_helper_pan_display, > > + .fb_setcmap = drm_fb_helper_setcmap, > > +}; > > If you re assuming any kind of gtt then you should probably allow for gtt > based scrolling eventually, but thats an optimisation. I'll keep that for later. > > > > +int sdrm_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf) > > +{ > > + struct drm_gem_object *obj = vma->vm_private_data; > > + struct sdrm_gem_obj *sdrm_gem_obj = to_sdrm_gem_obj(obj); > > + struct drm_device *dev = obj->dev; > > + unsigned long pfn; > > + pgoff_t page_offset; > > + int ret; > > For dumb hardware take a look how gma500 and some other bits do this - > you can premap the entire buffer when you take the first fault, which for > a dumb fb is a good bet. > > > > Looking at it from the point of view of x86 legacy devices then the > things I see are > > - Device is quite possibly PCI (but may be platform eg vesa) > - Memory will probably be allocated in the PCI space > - Mappings are probably write combining but not on all hardware > > There's probably a case for pinning/unpinning scanout buffers according > to whether they are used. On some hardware the io mapping needed is a > precious resource. Also for stuff with a fixed fb space it means you can > combine it with invalidating the mmap mappings of an object and copying > objects in/out of the frame buffer to provide the expected interfaces to > allocate/release framebuffers. I'll have a look. Unfortunately my knowledge of these things is quite limited. I am hoping a bit for Thierry here since he has a iommu on Tegra and maybe this helps making the GEM support more generic. Sascha
On Wed, Apr 11, 2012 at 4:33 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > This patch adds support for creating simple drm devices. The > basic idea of this patch is that drm drivers using the sdrm layer > no longer have to implement a full drm device but instead only > register crtcs, encoders and connectors with the sdrm layer. The > sdrm layer then registers a drm device with the drm core and > takes care of the drm device. I'm sorry to say I totally hate this on every level. I think I said to you before that midlayers are not the answer, and this is a hella big midlayer. I understand the SoC architecture but I can't think this is the way forward. The problems I see from a highlevel: This layer is highly restrictive on its users, I get the feeling once you get to implement 2-3 complete drivers or try and implement a driver say on x86 that should work in a similar fashion you are going to realise you made things overly generic and the driver can't change it. Then the drivers will start to workaround the midlayer and we'll end up worse off. I don't really want to pick on specifics but the taking over of the fb ops is on example, I think this should work as a set of helpers that might work in place of the current set of helpers. The current helpers are very directed towards desktop x86 experience, so a new set of these might be better. I get the feeling the drm can just be a virtual platform device of some sort, then it reads the device tree and binds all the information on what crtc/encoders are available, Also the mode group stuff isn't legacy, the render nodes stuff posted is what is intended to use it for, again it may not be useful on ARM, but on desktop it has a very useful use case. I'm sorry to not provide the answer I would fine acceptable, maybe if I had a week of time to write something I could figure it out, maybe someone else can give advice on how this sort of thing might look, Linearo/ARM guys can some of you guys look at this? Dave.
* Dave Airlie wrote: > I get the feeling the drm can just be a virtual platform device of > some sort, then it reads the device tree and binds all the information > on what crtc/encoders are available, That's pretty much what I've come up with in the second round of Tegra DRM patches. Basically display controllers and outputs (RGB, HDMI, TVO, DSI) get separate drivers and register themselves with the DRM driver which then looks at the device tree to see which display controllers to register as CRTCs and parses a list of connector nodes to create encoder/connector pairs that define the physical connectors and their corresponding outputs. I did take a brief look at the SDRM patches as well and they didn't quite seem to fit what was needed for Tegra. But if time allows I'll take a closer look. Thierry
[Added some embedded graphics maintainers to Cc who might be interested in this] On Fri, Apr 20, 2012 at 11:02:02AM +0100, Dave Airlie wrote: > On Wed, Apr 11, 2012 at 4:33 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > > This patch adds support for creating simple drm devices. The > > basic idea of this patch is that drm drivers using the sdrm layer > > no longer have to implement a full drm device but instead only > > register crtcs, encoders and connectors with the sdrm layer. The > > sdrm layer then registers a drm device with the drm core and > > takes care of the drm device. > > I'm sorry to say I totally hate this on every level. I think I said to > you before that midlayers are not the answer, and this is a hella big > midlayer. > > I understand the SoC architecture but I can't think this is the way forward. > > The problems I see from a highlevel: > > This layer is highly restrictive on its users, I get the feeling once > you get to implement 2-3 complete drivers or try and implement a > driver say on x86 that should work in a similar fashion you are going > to realise you made things overly generic and the driver can't change > it. That's maybe where our philosophies clash. I'd say that drivers should just have enough freedom to get their job done whereas you want to give drivers the freedom to do anything they want. I come from an area where we have dozens of drivers which mostly are quite similar, in the ARM world we currently try to get the duplication out of the drivers because otherwise we can't handle the sheer amount of devices we have. > Then the drivers will start to workaround the midlayer and we'll > end up worse off. I don't really want to pick on specifics but the > taking over of the fb ops is on example, The layer can be extended when we need it, no need to work around it. I took over the fb ops because currently I don't need to adjust them. I know that some devices have accelerated blit operations and this means that we may add a way to overwrite the ops later. Same with the ioctls. I didn't provide a way to add device specific ioctls now, partly because currently I don't need them, partly to get this series out. > > I think this should work as a set of helpers that might work in place > of the current set of helpers. The current helpers are very directed > towards desktop x86 experience, so a new set of these might be better. Hm, this means duplicating the helpers. The KMS support is to a great extend defined by the helpers. Duplicating them would mean more code fragmentation and different behaviour from a users point of view. I'd rather not go this way. > > I get the feeling the drm can just be a virtual platform device of > some sort, then it reads the device tree and binds all the information > on what crtc/encoders are available, We can do that. Currently I use a string matching mechanism to tie together the different pieces of a device, but sooner or later it will be devicetree anyway, so no problem to convert this sooner instead of later. The only problem I see with this is that for example X86 will not have devicetree, so I see a value in not binding whatever we come up with too tight to devicetree. > > Also the mode group stuff isn't legacy, the render nodes stuff posted > is what is intended to use it for, again it may not be useful on ARM, > but on desktop it has a very useful use case. I didn't remove them because they are not useful, but because currently I couldn't add an encoder/connector to a active drm device (see how the exynos driver currently works around this, I doubt it will work this way once the legacy_mode_group handling is actually used). The fact that this is currently unused motivated me to remove it. That said, I can live without it, no problem. > > I'm sorry to not provide the answer I would fine acceptable, maybe if > I had a week of time to write something I could figure it out, maybe > someone else can give advice on how this sort of thing might look, > Linearo/ARM guys can some of you guys look at this? Take these patches as my try to show that something has to change to make drm stuff more widely usable on embedded devices. As said, there are dozens of different devices out there, many of them are dumb framebuffer devices like the PXA and i.MX driver I posted, others are more advanced like the exynos, tegra and the newer i.MX devices. Some of the bigger players can effort to write (and maintain?) a 300KB driver, others can not. Those who cannot currently stay in drivers/video, but this has limitations when it comes to overlay, hot pluggable monitors, multiple crtcs, etc. I'd like to find a solution for this which makes us all happy. In the end reducing the amount of code duplication also helps you as a maintainer. (BTW each driver in drm has this layer somewhere in it. If I had hidden it in imx specific functions I probably wouldn't have raised any questions, but I don't want to go that way) Sascha
On Fri, Apr 20, 2012 at 02:38:43PM +0200, Thierry Reding wrote: > * Dave Airlie wrote: > > I get the feeling the drm can just be a virtual platform device of > > some sort, then it reads the device tree and binds all the information > > on what crtc/encoders are available, > > That's pretty much what I've come up with in the second round of Tegra DRM > patches. Basically display controllers and outputs (RGB, HDMI, TVO, DSI) get > separate drivers and register themselves with the DRM driver which then looks > at the device tree to see which display controllers to register as CRTCs and > parses a list of connector nodes to create encoder/connector pairs that > define the physical connectors and their corresponding outputs. > > I did take a brief look at the SDRM patches as well and they didn't quite > seem to fit what was needed for Tegra. But if time allows I'll take a closer > look. Can you elaborate which parts don't fit? I am very interested to improve this situation, and I think having code to share will be a benefit for us all. I know that the patches I wrote are no one-solution-fits-for-all yet, they are mainly something to show that drm drivers do not have to be huge complex drivers. Sascha
On Fri, Apr 20, 2012 at 03:10:05PM +0200, Sascha Hauer wrote: > (BTW each driver in drm has this layer somewhere in it. If I had hidden > it in imx specific functions I probably wouldn't have raised any > questions, but I don't want to go that way) That's _exactly_ what you should be doing. And once you have more than one driver that works in a similar way, you can extract the common code as helper functions to make life easier. Like Rob&Alan did with a few gem helpers they needed in omapdrm/gma500. For your case it sounds like a new set of modeset helper functions tailored for the embedded use-case would be good (as Dave Airlie suggested). Adding yet another middle-layer (like sdrm is) that forces drivers to go through it usually ends up in tears. And drm core unfortunately still has too much middle-layer heritage: For an awful lot of setup and teardown issues it's the core of the problme - because drivers can't control when drm sets up and tears down certain things, it's done at the wrong time (for certain drivers at least). Same problem when the abstraction doesn't quite fit. Helper functions leave the driver in full control of what's going on, and working around hw-specific madness with ease. https://lwn.net/Articles/336262/ is the canonical reference for why a lot of kernel people are allergic to anything that looks like a middle-layer. Yours, Daniel
On Fri, Apr 20, 2012 at 02:38:43PM +0200, Thierry Reding wrote: > * Dave Airlie wrote: > > I get the feeling the drm can just be a virtual platform device of > > some sort, then it reads the device tree and binds all the information > > on what crtc/encoders are available, > That's pretty much what I've come up with in the second round of Tegra DRM > patches. Basically display controllers and outputs (RGB, HDMI, TVO, DSI) get > separate drivers and register themselves with the DRM driver which then looks > at the device tree to see which display controllers to register as CRTCs and > parses a list of connector nodes to create encoder/connector pairs that > define the physical connectors and their corresponding outputs. > I did take a brief look at the SDRM patches as well and they didn't quite > seem to fit what was needed for Tegra. But if time allows I'll take a closer > look. This sounds an awful lot like how ASoC hangs together...
* Mark Brown wrote: > On Fri, Apr 20, 2012 at 02:38:43PM +0200, Thierry Reding wrote: > > * Dave Airlie wrote: > > > I get the feeling the drm can just be a virtual platform device of > > > some sort, then it reads the device tree and binds all the information > > > on what crtc/encoders are available, > > > That's pretty much what I've come up with in the second round of Tegra DRM > > patches. Basically display controllers and outputs (RGB, HDMI, TVO, DSI) get > > separate drivers and register themselves with the DRM driver which then looks > > at the device tree to see which display controllers to register as CRTCs and > > parses a list of connector nodes to create encoder/connector pairs that > > define the physical connectors and their corresponding outputs. > > > I did take a brief look at the SDRM patches as well and they didn't quite > > seem to fit what was needed for Tegra. But if time allows I'll take a closer > > look. > > This sounds an awful lot like how ASoC hangs together... What in particular sounds awful? Thierry
On Fri, Apr 20, 2012 at 04:49:43PM +0200, Thierry Reding wrote: > * Mark Brown wrote: > > This sounds an awful lot like how ASoC hangs together... > What in particular sounds awful? Nothing - "an awful" is an English idiom for "very".
* Mark Brown wrote: > On Fri, Apr 20, 2012 at 04:49:43PM +0200, Thierry Reding wrote: > > * Mark Brown wrote: > > > > This sounds an awful lot like how ASoC hangs together... > > > What in particular sounds awful? > > Nothing - "an awful" is an English idiom for "very". I know =) But it has a somewhat negative connotation, from which I deduced that you somehow thought it wasn't a good solution. Thierry
On Fri, Apr 20, 2012 at 03:25:58PM +0100, Mark Brown wrote: > On Fri, Apr 20, 2012 at 02:38:43PM +0200, Thierry Reding wrote: > > * Dave Airlie wrote: > > > I get the feeling the drm can just be a virtual platform device of > > > some sort, then it reads the device tree and binds all the information > > > on what crtc/encoders are available, > > > That's pretty much what I've come up with in the second round of Tegra DRM > > patches. Basically display controllers and outputs (RGB, HDMI, TVO, DSI) get > > separate drivers and register themselves with the DRM driver which then looks > > at the device tree to see which display controllers to register as CRTCs and > > parses a list of connector nodes to create encoder/connector pairs that > > define the physical connectors and their corresponding outputs. > > > I did take a brief look at the SDRM patches as well and they didn't quite > > seem to fit what was needed for Tegra. But if time allows I'll take a closer > > look. > > This sounds an awful lot like how ASoC hangs together... Very much, yes. In ASoC and DRM we both have several physical devices spread around the SoC which form a logical device. I assume that before ASoC existed also everyone had a single PCI device which could be used to collect the information together. Sascha
On Fri, Apr 20, 2012 at 05:15:18PM +0200, Sascha Hauer wrote: > On Fri, Apr 20, 2012 at 03:25:58PM +0100, Mark Brown wrote: > > This sounds an awful lot like how ASoC hangs together... > Very much, yes. In ASoC and DRM we both have several physical devices spread > around the SoC which form a logical device. I assume that before ASoC existed > also everyone had a single PCI device which could be used to collect the > information together. Yeah, it's a similar issue - on PC hardware we tend to have a single integrated device which does everything (at least from the point of view of the outside world, physically things may sometimes be split).
On Fri, Apr 20, 2012 at 03:33:14PM +0200, Daniel Vetter wrote: > On Fri, Apr 20, 2012 at 03:10:05PM +0200, Sascha Hauer wrote: > > (BTW each driver in drm has this layer somewhere in it. If I had hidden > > it in imx specific functions I probably wouldn't have raised any > > questions, but I don't want to go that way) > > That's _exactly_ what you should be doing. And once you have more than one > driver that works in a similar way, you can extract the common code as > helper functions to make life easier. I already have three drivers working in a similar way from which I posted two. I could easily throw in some more into the pot. Also the code is based on the exynos driver, so I think it's suitable for this aswell. > > For your case it sounds like a new set of modeset helper functions > tailored for the embedded use-case would be good (as Dave Airlie > suggested). One of my problems is that currently drm is based on the assumption that there is a single device which offers all needed resources and information to form a drm device. On embedded systems this is simply not the case, we have our resources all around the SoC. I have physical devices which are crtcs, encoders or connectors, but drm does not provide a way to glue them together. You can find this aswell in the exynos driver if you grep for exynos_drm_subdrv_register. If you follow this function you'll see that a good bunch of the driver actually handles the management of these subdevices. Do you have a suggestion solving the involved code duplication with helper functions? Also sooner or later it will happen that the same hdmi controller is used on two otherwise different SoCs. Currently the driver can't be shared between SoCs because each hdmi driver implements exynos or nouveau specific callbacks. I guess the answer is to put the common hdmi driver code into helper functions and to implement a middle layer in each drm driver wishing to use it. > Adding yet another middle-layer (like sdrm is) that forces > drivers to go through it usually ends up in tears. And drm core > unfortunately still has too much middle-layer heritage: For an awful lot > of setup and teardown issues it's the core of the problme - because > drivers can't control when drm sets up and tears down certain things, it's > done at the wrong time (for certain drivers at least). Same problem when > the abstraction doesn't quite fit. > > Helper functions leave the driver in full control of what's going on, and > working around hw-specific madness with ease. > > https://lwn.net/Articles/336262/ is the canonical reference for why a lot > of kernel people are allergic to anything that looks like a middle-layer. I have read the article when it was featured on LWN. While I agree to several things I have my problems with it. Take for example the MMC core. A MMC driver mainly has to implement two callbacks, .request and .set_ios. Noone has ever asked to get direct access from the driver to the underlying block device and eventually pass control to MMC helper functions. This makes the MMC core a middle layer sitting between the blockdevice and the driver. With drm instead it's normal that ioctls fall straight through to the driver. This leads to such funny things that the kernel itself cannot control the device to bring a console on the screen without dedicated help from the driver. Sascha