Re: [PATCH] RFC: omapdrm DRM/KMS driver for TI OMAP platforms

[Rob Clark <rob.clark-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On Wed, Sep 7, 2011 at 1:00 AM, Inki Dae <inki.dae-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> Hello, Rob.
>
[snip]
>> >> +static void page_flip_cb(void *arg)
>> >> +{
>> >> +     struct drm_crtc *crtc = arg;
>> >> +     struct drm_device *dev = crtc->dev;
>> >> +     struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>> >> +     struct drm_pending_vblank_event *event = omap_crtc->event;
>> >> +     struct timeval now;
>> >> +     unsigned long flags;
>> >> +
>> >> +     WARN_ON(!event);
>> >> +
>> >> +     omap_crtc->event = NULL;
>> >> +
>> >> +     update_scanout(crtc);
>> >> +     commit(crtc);
>> >> +
>> >> +     /* wakeup userspace */
>> >> +     // TODO: this should happen *after* flip.. somehow..
>> >> +     if (event) {
>> >> +             spin_lock_irqsave(&dev->event_lock, flags);
>> >> +             event->event.sequence =
>> >> +                             drm_vblank_count_and_time(dev,
>> > omap_crtc->id,
>> >> &now);
>> >> +             event->event.tv_sec = now.tv_sec;
>> >> +             event->event.tv_usec = now.tv_usec;
>> >> +             list_add_tail(&event->base.link,
>> >> +                             &event->base.file_priv->event_list);
>> >> +
> wake_up_interruptible(&event->base.file_priv->event_wait);
>> >> +             spin_unlock_irqrestore(&dev->event_lock, flags);
>> >> +     }
>> >
>> > How about moving codes above into interrupt handler for vblank?
>> >  maybe 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).  I'm
>> not really sure how to handle this best.  I 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 interrupt
> 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 current
> 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.  In these
>> cases we constantly scan-out the buffer until the next page
>> flip+vsync.  And 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..  the page flip can be submitted
>> from userspace while some other hw block (3d, 2d, etc) is still
>> DMA'ing to the buffer.  The 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’s 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 synchronization.
> 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 set
> 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, int
>> y,
>> >> +             void **vaddr, unsigned long *paddr, int *screen_width)
>> >> +{
>> >> +     struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
>> >> +     int bpp = fb->depth / 8;
>> >> +     unsigned long offset;
>> >> +
>> >> +     offset = (x * bpp) + (y * fb->pitch);
>> >> +
>> >> +     if (vaddr) {
>> >> +             if (!omap_fb->vaddr) {
>> >> +                     omap_fb->vaddr = ioremap_wc(omap_fb->paddr,
>> omap_fb-
>> >> >size);
>> >> +             }
>> >> +             *vaddr = omap_fb->vaddr + offset;
>> >> +     }
>> >
>> > 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,  this 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.  So
>> we are ok.  And I think this should still be the case w/ CMA.
>>
>
> I wonder what is the carveout and sacnout memory. carvout is physically non
> 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 and
>> > 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.  I'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.  Although should maybe still be split
>> from what is needed by userspace?  (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?  (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