* Review request to DRM Driver for Samsung SoC Exynos4210. @ 2011-08-29 10:31 Inki Dae 2011-08-29 14:26 ` Dave Airlie 0 siblings, 1 reply; 8+ messages in thread From: Inki Dae @ 2011-08-29 10:31 UTC (permalink / raw) To: airlied; +Cc: kyungmin.park, dri-devel [-- Attachment #1.1: Type: text/plain, Size: 854 bytes --] Hello, David Airlie I had sent DRM Driver for Samsung SoC Exynos4210 over the three times. But I didn't received any comments from you. You can refer to these patches I sent from links below. v1: < https://lwn.net/Articles/454380/ > v2: < http://www.spinics.net/lists/kernel/msg1224275.html > v2: < http://www.spinics.net/lists/dri-devel/msg13755.html > and also you can refer to our working repository below. < http://git.infradead.org/users/kmpark/linux-2.6-samsung > Branch name : samsung-drm I would be pleased you to give me your any comments or advices if these have some problems. otherwise apply this driver to your git repository please. We are welcome to any comments, advices or pointing out and also ready to do mainline work aggressively. :) Thank you for your interesting in advice. Best Regards Inki Dae. [-- Attachment #1.2: Type: text/html, Size: 3804 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Review request to DRM Driver for Samsung SoC Exynos4210. 2011-08-29 10:31 Review request to DRM Driver for Samsung SoC Exynos4210 Inki Dae @ 2011-08-29 14:26 ` Dave Airlie 2011-08-30 0:01 ` Kyungmin Park 2011-08-30 5:27 ` Inki Dae 0 siblings, 2 replies; 8+ messages in thread From: Dave Airlie @ 2011-08-29 14:26 UTC (permalink / raw) To: Inki Dae; +Cc: kyungmin.park, dri-devel > I had sent DRM Driver for Samsung SoC Exynos4210 over the three times. > > But I didn’t received any comments from you. I was sort of hoping someone else would take a look at these ARM drivers before me, it might be worth getting some inter-company review between the vendors submitting drm components as I'm guessing its going to be a lot of the same thing. But I've done a quick review and some things that stick out. 1. include/drm/samsung_drm.h should only contain public information for use on the userspace ioctl interface, it shouldn't contain any kernel private data structures, they should be in drivers/gpu/drm/samsung/samsung_drv.h or something very similiar. 2. I'm not sure why samsung_drm_fn_encoder exists, it looks like from the crtc mode set functions you call the encoder mode set functions, is it not possible to use the helper drm_crtc_helper_set_mode so that the crtc mode set is called then the encoder ones without having the explicit calls? If not please either document it or point at the explaination I missed. 3. Not terribly urgent but is samsung the best name for this? what happens if you bring out another chip, I also think there is a lot of samsung_drm_ in function names that seems not that useful. dropping _drm_ might help. 4 ioctls: drm_samsung_gem_map_off needs explicit padding before the 64-bit, drm_samsung_gem_mmap needs explicit padding before the 64-bit,, though I'm not sure you need these ioctls all now that the dumb interface is upstream, what is usr_addr in the gem create ioctl for? this seems wrong, it also looks too short for 64-bit addresses, but passing in userspace addr to kernel is generally not a great plan. 5. you probably don't need master_create/set ops. Dave. ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: Review request to DRM Driver for Samsung SoC Exynos4210. 2011-08-29 14:26 ` Dave Airlie @ 2011-08-30 0:01 ` Kyungmin Park 2011-08-30 5:27 ` Inki Dae 1 sibling, 0 replies; 8+ messages in thread From: Kyungmin Park @ 2011-08-30 0:01 UTC (permalink / raw) To: 'Dave Airlie', 'Inki Dae' Cc: rob, eric.y.miao, dri-devel, 'Kyungmin Park' CC Eric (freescale), Rob (TI) who working the DRM with other ARM SoCs. As Dave mentioned, Please review Samsung DRM codes. Thank you, Kyungmin Park -----Original Message----- From: Dave Airlie [mailto:airlied@gmail.com] Sent: Monday, August 29, 2011 11:27 PM To: Inki Dae Cc: airlied@linux.ie; kyungmin.park@samsung.com; dri-devel@lists.freedesktop.org Subject: Re: Review request to DRM Driver for Samsung SoC Exynos4210. > I had sent DRM Driver for Samsung SoC Exynos4210 over the three times. > > But I didn't received any comments from you. I was sort of hoping someone else would take a look at these ARM drivers before me, it might be worth getting some inter-company review between the vendors submitting drm components as I'm guessing its going to be a lot of the same thing. But I've done a quick review and some things that stick out. 1. include/drm/samsung_drm.h should only contain public information for use on the userspace ioctl interface, it shouldn't contain any kernel private data structures, they should be in drivers/gpu/drm/samsung/samsung_drv.h or something very similiar. 2. I'm not sure why samsung_drm_fn_encoder exists, it looks like from the crtc mode set functions you call the encoder mode set functions, is it not possible to use the helper drm_crtc_helper_set_mode so that the crtc mode set is called then the encoder ones without having the explicit calls? If not please either document it or point at the explaination I missed. 3. Not terribly urgent but is samsung the best name for this? what happens if you bring out another chip, I also think there is a lot of samsung_drm_ in function names that seems not that useful. dropping _drm_ might help. 4 ioctls: drm_samsung_gem_map_off needs explicit padding before the 64-bit, drm_samsung_gem_mmap needs explicit padding before the 64-bit,, though I'm not sure you need these ioctls all now that the dumb interface is upstream, what is usr_addr in the gem create ioctl for? this seems wrong, it also looks too short for 64-bit addresses, but passing in userspace addr to kernel is generally not a great plan. 5. you probably don't need master_create/set ops. Dave. ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: Review request to DRM Driver for Samsung SoC Exynos4210. 2011-08-29 14:26 ` Dave Airlie 2011-08-30 0:01 ` Kyungmin Park @ 2011-08-30 5:27 ` Inki Dae 2011-08-30 9:46 ` Dave Airlie 1 sibling, 1 reply; 8+ messages in thread From: Inki Dae @ 2011-08-30 5:27 UTC (permalink / raw) To: 'Dave Airlie' Cc: eric.y.miao, sw0312.kim, dri-devel, kyungmin.park, rob Hello Dave. Thank you for your comments. Below is my answers and questions. > -----Original Message----- > From: Dave Airlie [mailto:airlied@gmail.com] > Sent: Monday, August 29, 2011 11:27 PM > To: Inki Dae > Cc: airlied@linux.ie; kyungmin.park@samsung.com; dri- > devel@lists.freedesktop.org > Subject: Re: Review request to DRM Driver for Samsung SoC Exynos4210. > > > I had sent DRM Driver for Samsung SoC Exynos4210 over the three times. > > > > But I didn't received any comments from you. > > I was sort of hoping someone else would take a look at these ARM > drivers before me, it might be worth getting some inter-company review > between the vendors submitting drm components as I'm guessing its > going to be a lot of the same thing. > > But I've done a quick review and some things that stick out. > > 1. include/drm/samsung_drm.h should only contain public information > for use on the userspace ioctl interface, it shouldn't contain any > kernel private data structures, they should be in > drivers/gpu/drm/samsung/samsung_drv.h or something very similiar. > Ok, I will move kernel private data structures of include/drm/samsung_drm.h to drivers/gpu/drm/samsung/any header file. > 2. I'm not sure why samsung_drm_fn_encoder exists, it looks like from > the crtc mode set functions you call the encoder mode set functions, > is it not possible to use the helper drm_crtc_helper_set_mode so that > the crtc mode set is called then the encoder ones without having the > explicit calls? If not please either document it or point at the > explaination I missed. > For generic usage of crtc, we got only encoder has display controller(or H/Ws) specific codes. as you pointed out, I will check how we use drm_crtc_helper_set_mode instead of samsung_drm_fb_encoder because drm_crtc_helper_set_mode calls not only crtc but also encoder's callbacks and in addition, we faced with vblank issue. drm framework has only one irq handler but in embedded system at least Samsung SoC, display controller and hdmi have independent their own irq. so I got crtc has common vblank operations such as enable_vblank and disable_vblank and each encoder's function to be called in accordance with activated crtc. if you think this way is wrong or not good way then do you have any good ideas to resolve this issue without Samsung_drm_fn_encoder, the ideas that crtc could access to encoder.? > 3. Not terribly urgent but is samsung the best name for this? what > happens if you bring out another chip, I also think there is a lot of > samsung_drm_ in function names that seems not that useful. dropping > _drm_ might help. > As you know, Samsung SoCs have a variety of prefixes such as s3c24xx, s3c64xx, s3c, s5p, s5pv210(=s5pc110) and exynos4210(=s5pc210, =s5pv310). except old SoCs, our drm driver would support only exynos4210 and later. so we used samsung_ as prefix to represent them. but if you think this prefix is not proper name then we would try to consider other names. And _drm_ would help to debug. for instance, we could know who is current function's owner. > 4 ioctls: drm_samsung_gem_map_off needs explicit padding before the > 64-bit, drm_samsung_gem_mmap needs explicit padding before the > 64-bit,, though I'm not sure you need these ioctls all now that the > dumb interface is upstream, > I am afraid I don't understand what you mean fully. Could you please give me more details about "drm_samsung_gem_map_off needs explicit padding before the 64-bit, drm_samsung_gem_mmap needs explicit padding before the 64-bit,,"? as DRM, I am a novice. drm_samsung_gem_map_off is used to get offset that actually this is hash key that this key is used to get a gem object corresponding to it at drm_gem_mmap(). and the gem object is transferred to page fault handler(samsung_drm_gem_fault) with vma->vm_private_data containing it. At this time, page fault handler gets a gem object from vma object and then maps user space to physical memory. call patch to drm_gem_mmap is as the following : mmap system call -> samsung_drm_gem_mmap -> drm_gem_mmap if the comments above are wrong way then feel free to give me your advices and I will be pleased. Basically gem_dumb_* and gem_* are same operation. The difference between them is that gem_dumb_ needs framebuffer information such width, height and bpp to allocate buffer and gem_ needs only size. in case of gem_dumb_, size would be calculated at kernel side(at samsung_drm_gem_dumb_create). do you think it's better using only gem_dumb_? if so, I will remove gem_create_ioctl, gem_map_offset_ioctl and gem_mmap functions. > what is usr_addr in the gem create ioctl for? this seems wrong, it > also looks too short for 64-bit addresses, but passing in userspace > addr to kernel is generally not a great plan. > This is my mistake. I will remove usr_addr from drm_samsung_ge_create structure. this variable isn't used anywhere. > 5. you probably don't need master_create/set ops. > For h/w lock support, I think these operations should be used. If unnecessary, I will remove these operations. and could you please explain the purpose of drm_master?. I didn't understand the master fully. I'd try to find how we could use the master feature after understanding. > Dave. Thank yo for your comments again. it's been very helpful Best Regards Inki Dae. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Review request to DRM Driver for Samsung SoC Exynos4210. 2011-08-30 5:27 ` Inki Dae @ 2011-08-30 9:46 ` Dave Airlie 2011-08-30 12:38 ` Inki Dae 0 siblings, 1 reply; 8+ messages in thread From: Dave Airlie @ 2011-08-30 9:46 UTC (permalink / raw) To: Inki Dae; +Cc: eric.y.miao, sw0312.kim, dri-devel, kyungmin.park, rob >> 2. I'm not sure why samsung_drm_fn_encoder exists, it looks like from >> the crtc mode set functions you call the encoder mode set functions, >> is it not possible to use the helper drm_crtc_helper_set_mode so that >> the crtc mode set is called then the encoder ones without having the >> explicit calls? If not please either document it or point at the >> explaination I missed. >> > For generic usage of crtc, we got only encoder has display controller(or > H/Ws) specific codes. > as you pointed out, I will check how we use drm_crtc_helper_set_mode instead > of samsung_drm_fb_encoder because drm_crtc_helper_set_mode calls not only > crtc but also encoder's callbacks and in addition, we faced with vblank > issue. drm framework has only one irq handler but in embedded system at > least Samsung SoC, display controller and hdmi have independent their own > irq. so I got crtc has common vblank operations such as enable_vblank and > disable_vblank and each encoder's function to be called in accordance with > activated crtc. if you think this way is wrong or not good way then do you > have any good ideas to resolve this issue without Samsung_drm_fn_encoder, > the ideas that crtc could access to encoder.? Ah yes I see the issue now, that is probably a good reason, we should think of some way to improve the vblank code to better allow this, though that can probably wait until the driver is in mainline. > >> 3. Not terribly urgent but is samsung the best name for this? what >> happens if you bring out another chip, I also think there is a lot of >> samsung_drm_ in function names that seems not that useful. dropping >> _drm_ might help. >> > As you know, Samsung SoCs have a variety of prefixes such as s3c24xx, > s3c64xx, s3c, s5p, s5pv210(=s5pc110) and exynos4210(=s5pc210, =s5pv310). > except old SoCs, our drm driver would support only exynos4210 and later. so > we used samsung_ as prefix to represent them. but if you think this prefix > is not proper name then we would try to consider other names. And _drm_ > would help to debug. for instance, we could know who is current function's > owner. Generally, I'd prefer the name to be closer to the chip than the manufacturer, so exynos4210 or something like that, I just worry calling it samsung will lead to a later samsung2. So far the driver hasn't much specific code in it, but that may change. > >> 4 ioctls: drm_samsung_gem_map_off needs explicit padding before the >> 64-bit, drm_samsung_gem_mmap needs explicit padding before the >> 64-bit,, though I'm not sure you need these ioctls all now that the >> dumb interface is upstream, >> > I am afraid I don't understand what you mean fully. Could you please give > me more details about "drm_samsung_gem_map_off needs explicit padding before > the 64-bit, drm_samsung_gem_mmap needs explicit padding before the > 64-bit,,"? as DRM, I am a novice. drm_samsung_gem_map_off is used to get > offset that actually this is hash key that this key is used to get a gem > object corresponding to it at drm_gem_mmap(). and the gem object is > transferred to page fault handler(samsung_drm_gem_fault) with > vma->vm_private_data containing it. At this time, page fault handler gets a > gem object from vma object and then maps user space to physical memory. > call patch to drm_gem_mmap is as the following : mmap system call -> > samsung_drm_gem_mmap -> drm_gem_mmap > if the comments above are wrong way then feel free to give me your advices > and I will be pleased. If you have an 32-bit followed by a 64-bit, on different platforms that will end up aligned differently, generally we just try to be explicit and add another 32-bit pad after the first 32-bit. It might not matter on ARM (I'm not sure) it also might not matter until you move from 32-bit ARM to 64-bit ARM, but its best to deal with these early. > Basically gem_dumb_* and gem_* are same operation. The difference between > them is that gem_dumb_ needs framebuffer information such width, height and > bpp to allocate buffer and gem_ needs only size. in case of gem_dumb_, size > would be calculated at kernel side(at samsung_drm_gem_dumb_create). do you > think it's better using only gem_dumb_? if so, I will remove > gem_create_ioctl, gem_map_offset_ioctl and gem_mmap functions. I think using the dumb ioctls initially is a good plan, esp as you have no tiling or acceleration support, the idea behind the dumb ioctls is to give splash screens and maybe write a generic X.org driver in the future that can just do sw rendering. Like at some point I forsee you needing driver specific ioctls for alloc/mapping, I'd just rather wait until you have some userspace available to use them that we can validate them with. >> what is usr_addr in the gem create ioctl for? this seems wrong, it >> also looks too short for 64-bit addresses, but passing in userspace >> addr to kernel is generally not a great plan. >> > This is my mistake. I will remove usr_addr from drm_samsung_ge_create > structure. this variable isn't used anywhere. Thanks. > >> 5. you probably don't need master_create/set ops. >> > For h/w lock support, I think these operations should be used. If > unnecessary, I will remove these operations. and could you please explain > the purpose of drm_master?. I didn't understand the master fully. I'd try to > find how we could use the master feature after understanding. The drm lock is only for old pre-KMS DRI1 drivers, no modern driver should be using it or initialising it. drm master is the process that controls authorisation for direct rendering clients on the DRM device. So if you have an X server + 3D app, the X server is a master, if you switch users to another X server, that becomes the master. The driver only really needs this stuff if handles direct rendering and has command submission ioctls. Dave. > > >> Dave. > > Thank yo for your comments again. it's been very helpful > > Best Regards > Inki Dae. > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: Review request to DRM Driver for Samsung SoC Exynos4210. 2011-08-30 9:46 ` Dave Airlie @ 2011-08-30 12:38 ` Inki Dae 2011-08-30 22:16 ` Rob Clark 0 siblings, 1 reply; 8+ messages in thread From: Inki Dae @ 2011-08-30 12:38 UTC (permalink / raw) To: 'Dave Airlie' Cc: eric.y.miao, sw0312.kim, dri-devel, kyungmin.park, rob Hello Dave. > -----Original Message----- > From: Dave Airlie [mailto:airlied@gmail.com] > Sent: Tuesday, August 30, 2011 6:46 PM > To: Inki Dae > Cc: airlied@linux.ie; kyungmin.park@samsung.com; dri- > devel@lists.freedesktop.org; jy0922.shim@samsung.com; > sw0312.kim@samsung.com; eric.y.miao@gmail.com; rob@ti.com > Subject: Re: Review request to DRM Driver for Samsung SoC Exynos4210. > > >> 2. I'm not sure why samsung_drm_fn_encoder exists, it looks like from > >> the crtc mode set functions you call the encoder mode set functions, > >> is it not possible to use the helper drm_crtc_helper_set_mode so that > >> the crtc mode set is called then the encoder ones without having the > >> explicit calls? If not please either document it or point at the > >> explaination I missed. > >> > > For generic usage of crtc, we got only encoder has display controller(or > > H/Ws) specific codes. > > as you pointed out, I will check how we use drm_crtc_helper_set_mode > instead > > of samsung_drm_fb_encoder because drm_crtc_helper_set_mode calls not > only > > crtc but also encoder's callbacks and in addition, we faced with vblank > > issue. drm framework has only one irq handler but in embedded system at > > least Samsung SoC, display controller and hdmi have independent their > own > > irq. so I got crtc has common vblank operations such as enable_vblank > and > > disable_vblank and each encoder's function to be called in accordance > with > > activated crtc. if you think this way is wrong or not good way then do > you > > have any good ideas to resolve this issue without Samsung_drm_fn_encoder, > > the ideas that crtc could access to encoder.? > > Ah yes I see the issue now, that is probably a good reason, we should > think of some way to improve the vblank code to better allow this, > though that can probably wait until the driver is in mainline. > > > > >> 3. Not terribly urgent but is samsung the best name for this? what > >> happens if you bring out another chip, I also think there is a lot of > >> samsung_drm_ in function names that seems not that useful. dropping > >> _drm_ might help. > >> > > As you know, Samsung SoCs have a variety of prefixes such as s3c24xx, > > s3c64xx, s3c, s5p, s5pv210(=s5pc110) and exynos4210(=s5pc210, =s5pv310). > > except old SoCs, our drm driver would support only exynos4210 and later. > so > > we used samsung_ as prefix to represent them. but if you think this > prefix > > is not proper name then we would try to consider other names. And _drm_ > > would help to debug. for instance, we could know who is current > function's > > owner. > > Generally, I'd prefer the name to be closer to the chip than the > manufacturer, so exynos4210 or something like that, I just worry > calling it samsung will lead to a later samsung2. So far the driver > hasn't much specific code in it, but that may change. > I agree with you but Samsung has no naming rule and drm framework for Samsung Soc, drm drv, encoder, crtc, connector and so on, could be used commonly for all Samsung SoCs. actually in case of some drivers common to Samsung SoCs also use samsung prefix. you can refer to drivers/tty/serial/samsung.c also drivers/mtd/onenand/samsung.c. but in case of samsung_drm_fimd.c, I will change prefix to exynos4 because this module, samsung_drm_fimd.c, is specific to exynos4xxx series. if you are ok then we'd like to use so. > > > >> 4 ioctls: drm_samsung_gem_map_off needs explicit padding before the > >> 64-bit, drm_samsung_gem_mmap needs explicit padding before the > >> 64-bit,, though I'm not sure you need these ioctls all now that the > >> dumb interface is upstream, > >> > > I am afraid I don't understand what you mean fully. Could you please > give > > me more details about "drm_samsung_gem_map_off needs explicit padding > before > > the 64-bit, drm_samsung_gem_mmap needs explicit padding before the > > 64-bit,,"? as DRM, I am a novice. drm_samsung_gem_map_off is used to get > > offset that actually this is hash key that this key is used to get a gem > > object corresponding to it at drm_gem_mmap(). and the gem object is > > transferred to page fault handler(samsung_drm_gem_fault) with > > vma->vm_private_data containing it. At this time, page fault handler > gets a > > gem object from vma object and then maps user space to physical memory. > > call patch to drm_gem_mmap is as the following : mmap system call -> > > samsung_drm_gem_mmap -> drm_gem_mmap > > if the comments above are wrong way then feel free to give me your > advices > > and I will be pleased. > > If you have an 32-bit followed by a 64-bit, on different platforms > that will end up aligned differently, > generally we just try to be explicit and add another 32-bit pad after > the first 32-bit. It might not matter on ARM (I'm not sure) > it also might not matter until you move from 32-bit ARM to 64-bit ARM, > but its best to deal with these early. > Thank you. > > Basically gem_dumb_* and gem_* are same operation. The difference > between > > them is that gem_dumb_ needs framebuffer information such width, height > and > > bpp to allocate buffer and gem_ needs only size. in case of gem_dumb_, > size > > would be calculated at kernel side(at samsung_drm_gem_dumb_create). do > you > > think it's better using only gem_dumb_? if so, I will remove > > gem_create_ioctl, gem_map_offset_ioctl and gem_mmap functions. > > I think using the dumb ioctls initially is a good plan, esp as you > have no tiling or acceleration support, the idea > behind the dumb ioctls is to give splash screens and maybe write a > generic X.org driver in the future that can just do sw rendering. > > Like at some point I forsee you needing driver specific ioctls for > alloc/mapping, I'd just rather wait until you have some userspace > available to use them that we can validate them with. > Thank you for your pointing out. I will remove SAMSUNG_GEM_MAP_OFFSET and SAMSUNG_GEM_MAP ioctls except SAMSUNG_GEM_CREATE and SAMSUNG_GEM_MMAP ioctls because these are duplicated with dumb_*. and for alloc/mapping you mentioned above, we have already tested them through user application. This is example code in user level: /* allocation. */ gem.size = 1024 * 600 * 4; ioctl(fd, DRM_IOCTL_SAMSUNG_GEM_CREATE, &gem); /* user space mapping. */ map.handle = gem.handle map.offset = 0; map.size = gem.size; ioctl(fd, DRM_IOCTL_SAMSUNG_GEM_MMAP, &map); /* clear buffer. */ memset(map.mapped, 0x0, map.size); drmModeAddFB(fd, width, height, 32, 32, stride, map.handle, &fb_id); drmModeSetCrtc(fd, ....); /* release. */ munmap(map.mmaped, map.size); gem_close.handle = gem.handle; ioctl(fd, DRM_IOCTL_GEM_CLOSE, &gem_close); for application testing, we use libdrm v2.4.26. and also modetest of libdrm also worked fine. if you want full source code then I will send it. If there is any point I missed then point out and give me your comments or advices please. > >> what is usr_addr in the gem create ioctl for? this seems wrong, it > >> also looks too short for 64-bit addresses, but passing in userspace > >> addr to kernel is generally not a great plan. > >> > > This is my mistake. I will remove usr_addr from drm_samsung_ge_create > > structure. this variable isn't used anywhere. > > Thanks. > > > > >> 5. you probably don't need master_create/set ops. > >> > For h/w lock support, I think these operations should be used. If > > unnecessary, I will remove these operations. and could you please > explain > > the purpose of drm_master?. I didn't understand the master fully. I'd > try to > > find how we could use the master feature after understanding. > > The drm lock is only for old pre-KMS DRI1 drivers, no modern driver > should be using it or initialising it. drm master is the process that > controls authorisation for direct rendering clients on the DRM device. > So if you have an X server + 3D app, the X server is a master, if you > switch users to another X server, that becomes the master. The driver > only really needs this stuff if handles direct rendering and has > command submission ioctls. > Thank you for your explanation. I will remove master_create/master_set functions because our driver supports only S/W rendering yet. > Dave. > > > > > >> Dave. > > > > Thank yo for your comments again. it's been very helpful > > > > Best Regards > > Inki Dae. > > > > Almost parts you pointed out are being applied to our driver. and we have a plan to post next patch updated this week or next week. Best Regards Inki Dae. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Review request to DRM Driver for Samsung SoC Exynos4210. 2011-08-30 12:38 ` Inki Dae @ 2011-08-30 22:16 ` Rob Clark 2011-08-31 4:36 ` Inki Dae 0 siblings, 1 reply; 8+ messages in thread From: Rob Clark @ 2011-08-30 22:16 UTC (permalink / raw) To: Inki Dae; +Cc: eric.y.miao, kyungmin.park, dri-devel, sw0312.kim On Tue, Aug 30, 2011 at 7:38 AM, Inki Dae <inki.dae@samsung.com> wrote: > >> > Basically gem_dumb_* and gem_* are same operation. The difference >> between >> > them is that gem_dumb_ needs framebuffer information such width, height >> and >> > bpp to allocate buffer and gem_ needs only size. in case of gem_dumb_, >> size >> > would be calculated at kernel side(at samsung_drm_gem_dumb_create). do >> you >> > think it's better using only gem_dumb_? if so, I will remove >> > gem_create_ioctl, gem_map_offset_ioctl and gem_mmap functions. >> >> I think using the dumb ioctls initially is a good plan, esp as you >> have no tiling or acceleration support, the idea >> behind the dumb ioctls is to give splash screens and maybe write a >> generic X.org driver in the future that can just do sw rendering. >> >> Like at some point I forsee you needing driver specific ioctls for >> alloc/mapping, I'd just rather wait until you have some userspace >> available to use them that we can validate them with. >> > Thank you for your pointing out. I will remove SAMSUNG_GEM_MAP_OFFSET and > SAMSUNG_GEM_MAP ioctls except SAMSUNG_GEM_CREATE and SAMSUNG_GEM_MMAP ioctls > because these are duplicated with dumb_*. and for alloc/mapping you > mentioned above, we have already tested them through user application. > > This is example code in user level: > > /* allocation. */ > gem.size = 1024 * 600 * 4; > ioctl(fd, DRM_IOCTL_SAMSUNG_GEM_CREATE, &gem); > > /* user space mapping. */ > map.handle = gem.handle > map.offset = 0; > map.size = gem.size; > ioctl(fd, DRM_IOCTL_SAMSUNG_GEM_MMAP, &map); Inki, any reason not to just pass the offset (which looks like you get from _MMAP ioctl) back to mmap() syscall? Rather than having an ioctl that calls do_mmap()? Actually, it may even be enough to combine GEM_CREATE and GEM_MMAP ioctls.. (currently in OMAP DRM driver I have them combined). I *think* (and someone correct me if I am wrong), the only reason to have separate ioctl to get mmap offset is to allow for separate coherent and non-coherent mappings in same process. And this would only work on ARM if you had a proper GART that you were mapping through, otherwise it would be aliased virtual mappings of same physical page. I think that it would be possible to add another ioctl later to generate additional offsets if we ever had hw where this made sense. Until then a single GEM_CREATE that returns a handle and offset (plus GEM_INFO that gives you a way to get the offset in a different process) would be sufficient. BR, -R > > /* clear buffer. */ > memset(map.mapped, 0x0, map.size); > > drmModeAddFB(fd, width, height, 32, 32, stride, map.handle, &fb_id); > > drmModeSetCrtc(fd, ....); > > /* release. */ > munmap(map.mmaped, map.size); > > gem_close.handle = gem.handle; > ioctl(fd, DRM_IOCTL_GEM_CLOSE, &gem_close); ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: Review request to DRM Driver for Samsung SoC Exynos4210. 2011-08-30 22:16 ` Rob Clark @ 2011-08-31 4:36 ` Inki Dae 0 siblings, 0 replies; 8+ messages in thread From: Inki Dae @ 2011-08-31 4:36 UTC (permalink / raw) To: 'Rob Clark'; +Cc: eric.y.miao, kyungmin.park, dri-devel, sw0312.kim Hello Rob. Thank you for your comments. > -----Original Message----- > From: robdclark@gmail.com [mailto:robdclark@gmail.com] On Behalf Of Rob > Clark > Sent: Wednesday, August 31, 2011 7:16 AM > To: Inki Dae > Cc: Dave Airlie; eric.y.miao@gmail.com; sw0312.kim@samsung.com; dri- > devel@lists.freedesktop.org; kyungmin.park@samsung.com > Subject: Re: Review request to DRM Driver for Samsung SoC Exynos4210. > > On Tue, Aug 30, 2011 at 7:38 AM, Inki Dae <inki.dae@samsung.com> wrote: > > > >> > Basically gem_dumb_* and gem_* are same operation. The difference > >> between > >> > them is that gem_dumb_ needs framebuffer information such width, > height > >> and > >> > bpp to allocate buffer and gem_ needs only size. in case of gem_dumb_, > >> size > >> > would be calculated at kernel side(at samsung_drm_gem_dumb_create). > do > >> you > >> > think it's better using only gem_dumb_? if so, I will remove > >> > gem_create_ioctl, gem_map_offset_ioctl and gem_mmap functions. > >> > >> I think using the dumb ioctls initially is a good plan, esp as you > >> have no tiling or acceleration support, the idea > >> behind the dumb ioctls is to give splash screens and maybe write a > >> generic X.org driver in the future that can just do sw rendering. > >> > >> Like at some point I forsee you needing driver specific ioctls for > >> alloc/mapping, I'd just rather wait until you have some userspace > >> available to use them that we can validate them with. > >> > > Thank you for your pointing out. I will remove SAMSUNG_GEM_MAP_OFFSET > and > > SAMSUNG_GEM_MAP ioctls except SAMSUNG_GEM_CREATE and SAMSUNG_GEM_MMAP > ioctls > > because these are duplicated with dumb_*. and for alloc/mapping you > > mentioned above, we have already tested them through user application. > > > > This is example code in user level: > > > > /* allocation. */ > > gem.size = 1024 * 600 * 4; > > ioctl(fd, DRM_IOCTL_SAMSUNG_GEM_CREATE, &gem); > > > > /* user space mapping. */ > > map.handle = gem.handle > > map.offset = 0; > > map.size = gem.size; > > ioctl(fd, DRM_IOCTL_SAMSUNG_GEM_MMAP, &map); > > Inki, any reason not to just pass the offset (which looks like you get > from _MMAP ioctl) back to mmap() syscall? Rather than having an ioctl > that calls do_mmap()? > This is samsung specific ioctls and this feature simplifies to map user space to physical memory. the offset would be sent to driver by user and then samsung_drm_gem_mmap_buffer gets the offset value through vma->vm_pgoff after do_mmap -> do_mmap_pgoff -> mmap_region -> file->f_op->mmap. the offset is physical memory offset to be mapped like this. pfn = (samsung_gem_obj->entry->paddr + vma->vm_pgoff) >> PAGE_SHIFT; remap_pfn_range(..., pfn, ...); if this feature has some possibility to jeopardize drm framework in any case, then I will remove this feature. otherwise I'd like to use one. and I915 also use same feature. you can refer to libdrm/tests/gem_mmap.c for application and linux/drivers/gpu/drm/i915/i915_gem.c for device driver. if there are any points I missed, give me any comments or pointing out please. > Actually, it may even be enough to combine GEM_CREATE and GEM_MMAP > ioctls.. (currently in OMAP DRM driver I have them combined). I > *think* (and someone correct me if I am wrong), the only reason to > have separate ioctl to get mmap offset is to allow for separate > coherent and non-coherent mappings in same process. And this would > only work on ARM if you had a proper GART that you were mapping > through, otherwise it would be aliased virtual mappings of same > physical page. > Yes, we already have the feature you said above also. SAMSUNG_GEM_MAP_OFFSET ioctl is that. but as you know, this feature is duplicated with dumb_* and Dave pointed out before. only the difference between them is to use size only or buffer information such as width, height and bpp to allocate buffer. and so I am going to remove this feature. how do you think about that? I wonder your opinions. > I think that it would be possible to add another ioctl later to > generate additional offsets if we ever had hw where this made sense. > Until then a single GEM_CREATE that returns a handle and offset (plus > GEM_INFO that gives you a way to get the offset in a different > process) would be sufficient. > Thank you for your comments again. :) > BR, > -R > > > > > /* clear buffer. */ > > memset(map.mapped, 0x0, map.size); > > > > drmModeAddFB(fd, width, height, 32, 32, stride, map.handle, &fb_id); > > > > drmModeSetCrtc(fd, ....); > > > > /* release. */ > > munmap(map.mmaped, map.size); > > > > gem_close.handle = gem.handle; > > ioctl(fd, DRM_IOCTL_GEM_CLOSE, &gem_close); ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-08-31 4:36 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-08-29 10:31 Review request to DRM Driver for Samsung SoC Exynos4210 Inki Dae 2011-08-29 14:26 ` Dave Airlie 2011-08-30 0:01 ` Kyungmin Park 2011-08-30 5:27 ` Inki Dae 2011-08-30 9:46 ` Dave Airlie 2011-08-30 12:38 ` Inki Dae 2011-08-30 22:16 ` Rob Clark 2011-08-31 4:36 ` Inki Dae
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.