From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rob Clark Subject: Re: [PATCH] RFC: omapdrm DRM/KMS driver for TI OMAP platforms Date: Wed, 7 Sep 2011 13:43:54 -0500 Message-ID: References: <1314994047-4101-1-git-send-email-rob.clark@linaro.org> <002201cc6bb2$61779780$2466c680$%dae@samsung.com> <004e01cc6d23$78c151c0$6a43f540$%dae@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <004e01cc6d23$78c151c0$6a43f540$%dae-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linaro-dev-bounces-cunTk1MwBs8s++Sfvej+rw@public.gmane.org Errors-To: linaro-dev-bounces-cunTk1MwBs8s++Sfvej+rw@public.gmane.org To: Inki Dae Cc: linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org List-Id: dri-devel@lists.freedesktop.org On Wed, Sep 7, 2011 at 1:00 AM, Inki Dae wrote: > Hello, Rob. > [snip] >> >> +static void page_flip_cb(void *arg) >> >> +{ >> >> + =A0 =A0 struct drm_crtc *crtc =3D arg; >> >> + =A0 =A0 struct drm_device *dev =3D crtc->dev; >> >> + =A0 =A0 struct omap_crtc *omap_crtc =3D to_omap_crtc(crtc); >> >> + =A0 =A0 struct drm_pending_vblank_event *event =3D omap_crtc->event; >> >> + =A0 =A0 struct timeval now; >> >> + =A0 =A0 unsigned long flags; >> >> + >> >> + =A0 =A0 WARN_ON(!event); >> >> + >> >> + =A0 =A0 omap_crtc->event =3D NULL; >> >> + >> >> + =A0 =A0 update_scanout(crtc); >> >> + =A0 =A0 commit(crtc); >> >> + >> >> + =A0 =A0 /* wakeup userspace */ >> >> + =A0 =A0 // TODO: this should happen *after* flip.. somehow.. >> >> + =A0 =A0 if (event) { >> >> + =A0 =A0 =A0 =A0 =A0 =A0 spin_lock_irqsave(&dev->event_lock, flags); >> >> + =A0 =A0 =A0 =A0 =A0 =A0 event->event.sequence =3D >> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 drm_vblank_= count_and_time(dev, >> > omap_crtc->id, >> >> &now); >> >> + =A0 =A0 =A0 =A0 =A0 =A0 event->event.tv_sec =3D now.tv_sec; >> >> + =A0 =A0 =A0 =A0 =A0 =A0 event->event.tv_usec =3D now.tv_usec; >> >> + =A0 =A0 =A0 =A0 =A0 =A0 list_add_tail(&event->base.link, >> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 &event->bas= e.file_priv->event_list); >> >> + > wake_up_interruptible(&event->base.file_priv->event_wait); >> >> + =A0 =A0 =A0 =A0 =A0 =A0 spin_unlock_irqrestore(&dev->event_lock, fl= ags); >> >> + =A0 =A0 } >> > >> > How about moving codes above into interrupt handler for vblank? >> > =A0maybe there >> >> I should mention a couple of things: >> >> 1) drm vblank stuff isn't really used currently.. it is actually DSS2 >> layer that is registering for the display related interrupt(s). =A0I'm >> not really sure how to handle this best. =A0I suppose the DRM driver >> could *also* register for these interrupts, but that seems a bit odd.. >> > > DRM Framework supports only one interrupt handler. this issue should be > resolved. and currently I made our driver to use its own request_irq, not > DRM Framework side. we only sets drm_device->irq_enabled to 1 and interru= pt > handler is registered at display controller or hdmi driver respectively. = but > I'm not sure that this way is best so I will look over more. Anyway curre= nt > drm framework should be fixed to be considered with multiple irq. Or perhaps even callbacks (some other driver handling the irq's directly)? >> Also, I guess it is also worth mentioning.. when it comes to vblank, >> there are two fundamentally different sorts of displays we deal with. >> Something like DVI/HDMI/etc which need constant refreshing. =A0In these >> cases we constantly scan-out the buffer until the next page >> flip+vsync. =A0And smart panels, where they get scanned out once and >> then DSS is no longer reading the scanout buffer until we manually >> trigger an update. >> > > Is the Smart panel CPU interface based lcd panel that has its own > framebuffer internally.? yes [snip] >> >> The main reason for the page-flip cb is actually not vsync >> synchronization, but rather synchronizing with other hw blocks that >> might be rendering to the buffer.. =A0the page flip can be submitted >> from userspace while some other hw block (3d, 2d, etc) is still >> DMA'ing to the buffer. =A0The sync-obj is intended to give a way to >> defer the (in this case) page flip until other hw blocks are done >> writing to the buffer. > > I thought page-flip is used to change buffer register value of display > controller into another one like the Pan Display feature of linux > framebuffer. actually page flip interface of libdrm library, > page_flip_handler, is called with new framebuffer id that has its own > buffer. and then the controller using current crtc would be set to buffer > address of new framebuffer. and I understood that for other blocks such as > 2d/3d accelerators, rendering synchronization you already mentioned above, > is not depend on page flip action. It=92s a sequence with my understanding > below. > > 1. allocate a new buffer through drm interface such as DUMB_* or SPECIFIC > IOCTLs. > 2. render something on this buffer through DRM interfaces that handle 2D/= 3D > Accelerators and also it would use their own interfaces for synchronizati= on. > 3. allocate a new framebuffer and attach the buffer to this framebuffer. > 4. call page flip to change current framebuffer to the framebuffer above = at > vblank moment. at this time, buffer address of the framebuffer would be s= et > to a controller. > > finally, we can see some image rendered on diplay. thus, I think page flip > and rendering synchronization have no any relationship. if I missed any > points, please give me your comments. Well, I guess it depends on whether synchronization of the 3d/2d render is done in userspace, or whether you just submit all the render commands and then immediately submit the page-flip. The actual page-flip should be a fairly light operation (writing a few registers) and could be done directly from some sort of render-complete interrupt from 2d/3d core. [snip] >> >> +int omap_framebuffer_get_buffer(struct drm_framebuffer *fb, int x, i= nt >> y, >> >> + =A0 =A0 =A0 =A0 =A0 =A0 void **vaddr, unsigned long *paddr, int *sc= reen_width) >> >> +{ >> >> + =A0 =A0 struct omap_framebuffer *omap_fb =3D to_omap_framebuffer(fb= ); >> >> + =A0 =A0 int bpp =3D fb->depth / 8; >> >> + =A0 =A0 unsigned long offset; >> >> + >> >> + =A0 =A0 offset =3D (x * bpp) + (y * fb->pitch); >> >> + >> >> + =A0 =A0 if (vaddr) { >> >> + =A0 =A0 =A0 =A0 =A0 =A0 if (!omap_fb->vaddr) { >> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 omap_fb->vaddr =3D ioremap_= wc(omap_fb->paddr, >> omap_fb- >> >> >size); >> >> + =A0 =A0 =A0 =A0 =A0 =A0 } >> >> + =A0 =A0 =A0 =A0 =A0 =A0 *vaddr =3D omap_fb->vaddr + offset; >> >> + =A0 =A0 } >> > >> > Did you use ioremap_wc() to map physical memory(reserved memory) that >> kernel >> > doesn't aware of to kernel space.? if not so, the memory region mapped >> to >> > kernel space as 1:1, =A0this way would be faced with duplicated cache >> > attribute issue mentioned by Russel King before. 1:1 mapping region is >> > mapped to kernel space with cachable attribute. >> >> Today the carveout memory does not have a kernel virtual mapping. =A0So >> we are ok. =A0And I think this should still be the case w/ CMA. >> > > I wonder what is the carveout and sacnout memory. carvout is physically n= on > continuous memory and scanout is physically continuous memory? today, scanout buffers are required to be contiguous. There is the possibility to remap non-contiguous buffers in TILER/DMM (which you can think of as a sort of IOMMU). Although adding support for this is still on my TODO list. So today, carveout is used to allocate scanout buffers to ensure physically contiguous. [snip] >> > >> > Remove device specific functions from linux/include/linux/omap_drm.h a= nd >> > move them to driver folder. I was told that include/linux/*.h file >> should >> > contain only interfaces for user process from Dave. >> >> >> fwiw, the functions in this header are already only ones used by other >> kernel drivers.. everything else internal to omapdrm is already in >> omap_drv.h. =A0I'll remove these for now (see discussion about plugin >> API), although when it is re-introduced they need to be in a header >> accessible from other drivers. =A0Although should maybe still be split >> from what is needed by userspace? =A0(Something like omapdrm_plugin.h?) >> >> > > I'm afraid I don't understand what you mean, "Although should maybe still= be > split from what is needed by userspace? =A0(Something like > omapdrm_plugin.h?)", could you please clarify that again? I mean one header file for userspace facing API, and a separate one for kernel facing API used by other drivers/modules.. (In the first pass, that second header would not exist because I'll remove the plugin API until we have one w/ open userspace) BR, -R