[RFC 1/1] drm/pl111: Initial drm/kms driver for pl111
Inki Dae
inki.dae at samsung.com
Fri Aug 9 21:56:56 PDT 2013
2013/8/8 Daniel Vetter <daniel at ffwll.ch>
> Just comment a bit on Rob's review with my own opinion.
>
> On Wed, Aug 07, 2013 at 12:17:21PM -0400, Rob Clark wrote:
> > On Thu, Jul 25, 2013 at 1:17 PM, <tom.cooksey at arm.com> wrote:
> > > From: Tom Cooksey <tom.cooksey at arm.com>
> > >
> > > This is a mode-setting driver for the pl111 CLCD display controller
> > > found on various ARM reference platforms such as the Versatile
> > > Express. The driver supports setting of a single mode (640x480) and
> > > has only been tested on Versatile Express with a Cortex-A9 core tile.
> > >
> > > Known issues:
> > > * It still includes code to use KDS, which is not going upstream.
> >
> > review's on
> http://lists.freedesktop.org/archives/dri-devel/2013-July/042462.html
> > can't hurt
> >
> > although you might consider submitting a reduced functionality driver
> > w/ KDS bits removed in the mean time.. then when the fence stuff is
> > merged it is just an incremental patch rather than a whole driver ;-)
>
> Yeah, I think the KDS bits and comments need to go first before merginge.
>
>
> > > +/*
> > > + * Number of flips allowed in flight at any one time. Any more flips
> requested
> > > + * beyond this value will cause the caller to block until earlier
> flips have
> > > + * completed.
> > > + *
> > > + * For performance reasons, this must be greater than the number of
> buffers
> > > + * used in the rendering pipeline. Note that the rendering pipeline
> can contain
> > > + * different types of buffer, e.g.:
> > > + * - 2 final framebuffers
> > > + * - >2 geometry buffers for GPU use-cases
> > > + * - >2 vertex buffers for GPU use-cases
> > > + *
> > > + * For example, a system using 5 geometry buffers could have 5 flips
> in flight,
> > > + * and so NR_FLIPS_IN_FLIGHT_THRESHOLD must be 5 or greater.
> > > + *
> > > + * Whilst there may be more intermediate buffers (such as
> vertex/geometry) than
> > > + * final framebuffers, KDS is used to ensure that GPU rendering waits
> for the
> > > + * next off-screen buffer, so it doesn't overwrite an on-screen
> buffer and
> > > + * produce tearing.
> > > + */
> > > +
> >
> > fwiw, this is at least different from how other drivers do triple (or
> > > double) buffering. In other drivers (intel, omap, and
> > msm/freedreno, that I know of, maybe others too) the xorg driver dri2
> > bits implement the double buffering (ie. send flip event back to
> > client immediately and queue up the flip and call page-flip after the
> > pageflip event back from kernel.
> >
> > I'm not saying not to do it this way, I guess I'd like to hear what
> > other folks think. I kinda prefer doing this in userspace as it keeps
> > the kernel bits simpler (plus it would then work properly on exynosdrm
> > or other kms drivers).
>
> Yeah, if this is just a sw queue then I don't think it makes sense to have
> it in the kernel. Afaik the current pageflip interface drm exposes allows
> one oustanding flip only, and you _must_ wait for the flip complete event
> before you can submit the second one.
>
Agree. Tizen platform using exynos drm driver also does in same way. And
there is another issue we are facing with. Please, assume CPU and GPU are
sharing a same buffer. The issue is that in case of using glFlush GL API,
3d app cannot be aware of when GPU access to the buffer is
completed: the completion event is sent only to GPU specific API; in our
case, MALI specific DDK, so the buffer could be broken if CPU accesses the
buffer at once after glFlush. Of course we can use glFinish instead of
glFlush but glFinish makes GPU more idle: CPU should wait for the
completion of GPU access to the buffer so CPU cannot do anything until that
time. So I'd like to know how other folks take care of this issue.
In our case, we are using dmabuf sync framework I posted before because
I thought we may need buffer access control between CPU and DMA: the user
land interfaces are fcntl and select system calls so no having any new
additional api. with this feature, 3d app, only using standard GL API, can
be aware of the completion event from GPU driver without DRM or other
driver API. For this, I will introduce the more stable patch set soon with
more features.
For this, I'd happy to give me other opinions and advices if there is my
missing point.
Thanks,
Inki Dae
>
> Ofc if your hardware as a hw-based flip queue (maybe even with frame
> targets) that's a different matter, but currently we don't have a drm
> interface to expose this. I'd say for merging the basic driver first we
> should go with the existing simple pageflip semantics.
>
> And tbh I don't understand why the amount of buffers you keep in the
> render pipeline side of things matters here at all. But I also haven't
> read the details of your driver code.
>
> >
> > > +/*
> > > + * Here, we choose a conservative value. A lower value is most likely
> > > + * suitable for GPU use-cases.
> > > + */
> > > +#define NR_FLIPS_IN_FLIGHT_THRESHOLD 16
> > > +
> > > +#define CLCD_IRQ_NEXTBASE_UPDATE (1u<<2)
> > > +
> > > +struct pl111_drm_flip_resource;
> > > +struct pl111_drm_cursor_plane;
> > > +
> > > +enum pl111_bo_type {
> > > + PL111_BOT_DMA,
> > > + PL111_BOT_SHM
> > > +};
> > > +
> > > +struct pl111_gem_bo_dma {
> > > + dma_addr_t fb_dev_addr;
> > > + void *fb_cpu_addr;
> > > +};
> > > +
> > > +struct pl111_gem_bo_shm {
> > > + struct page **pages;
> > > + dma_addr_t *dma_addrs;
> > > +};
> > > +
> > > +struct pl111_gem_bo {
> > > + struct drm_gem_object gem_object;
> > > + enum pl111_bo_type type;
> > > + union {
> > > + struct pl111_gem_bo_dma dma;
> > > + struct pl111_gem_bo_shm shm;
> > > + } backing_data;
> > > + struct drm_framebuffer *fb;
> >
> > this is at least a bit odd.. normally the fb has ref to the bo(s) and
> > not the other way around. And the same bo could be referenced by
> > multiple fb's which would kinda fall down with this approach.
>
> I'd say that's just backwards, framebuffers are created from backing
> storage objects (which for a gem based driver is a gem object), not the
> other way round. What's this exactly used for?
>
> [snip]
>
> > > +
> > > + /*
> > > + * Used to prevent race between pl111_dma_buf_release and
> > > + * drm_gem_prime_handle_to_fd
> > > + */
> > > + struct mutex export_dma_buf_lock;
> >
> > hmm, seems a bit suspicious.. the handle reference should keep the
> > object live. Ie. either drm_gem_object_lookup() will fail because the
> > object is gone (userspace has closed it's handle ref and
> > dmabuf->release() already dropped it's ref) or it will succeed and
> > you'll have a reference to the bo keeping it from going away if the
> > release() comes after.
>
> The race is real, I have an evil testcase here which Oopses my kernel. I'm
> working on a fix (v1 of my patches is submitted a few weeks back, awaiting
> review), but I need to rework a few things since now I've also spotted a
> leak or two ;-)
>
> [snip]
>
> > > +static void vsync_worker(struct work_struct *work)
> > > +{
> > > + struct pl111_drm_flip_resource *flip_res;
> > > + struct pl111_gem_bo *bo;
> > > + struct pl111_drm_crtc *pl111_crtc;
> > > + struct drm_device *dev;
> > > + int flips_in_flight;
> > > + flip_res =
> > > + container_of(work, struct pl111_drm_flip_resource,
> vsync_work);
> > > +
> > > + pl111_crtc = to_pl111_crtc(flip_res->crtc);
> > > + dev = pl111_crtc->crtc.dev;
> > > +
> > > + DRM_DEBUG_KMS("DRM Finalizing flip_res=%p\n", flip_res);
> > > +
> > > + bo = PL111_BO_FROM_FRAMEBUFFER(flip_res->fb);
> > > +#ifdef CONFIG_DMA_SHARED_BUFFER_USES_KDS
> > > + if (flip_res->worker_release_kds == true) {
> > > + spin_lock(&pl111_crtc->current_displaying_lock);
> > > + release_kds_resource_and_display(flip_res);
> > > + spin_unlock(&pl111_crtc->current_displaying_lock);
> > > + }
> > > +#endif
> > > + /* Release DMA buffer on this flip */
> > > + if (bo->gem_object.export_dma_buf != NULL)
> > > + dma_buf_put(bo->gem_object.export_dma_buf);
> >
> > I think you just want to unref the outgoing bo, and let it drop the
> > dmabuf ref when the file ref of the imported bo goes. Or actually, it
> > would be better to hold/drop ref's to the fb, rather than the bo. At
> > least this will make things simpler if you ever have multi-planar
> > support.
>
> Drivers have no business frobbing around the dma-buf refcount of imported
> objects imo, at least if they use all the standard drm prime
> infrastructure. And if they're bugs they need to be fixed there, not in
> drivers.
>
> [snip]
>
> > > +struct drm_framebuffer *pl111_fb_create(struct drm_device *dev,
> > > + struct drm_file *file_priv,
> > > + struct drm_mode_fb_cmd2
> *mode_cmd)
> > > +{
> > > + struct pl111_drm_framebuffer *pl111_fb = NULL;
> > > + struct drm_framebuffer *fb = NULL;
> > > + struct drm_gem_object *gem_obj;
> > > + struct pl111_gem_bo *bo;
> > > +
> > > + pr_info("DRM %s\n", __func__);
> > > + gem_obj = drm_gem_object_lookup(dev, file_priv,
> mode_cmd->handles[0]);
> > > + if (gem_obj == NULL) {
> > > + DRM_ERROR("Could not get gem obj from handle to create
> fb\n");
> > > + goto out;
> > > + }
> > > +
> > > + bo = PL111_BO_FROM_GEM(gem_obj);
> > > + /* Don't even attempt PL111_BOT_SHM, it's not contiguous */
> > > + BUG_ON(bo->type != PL111_BOT_DMA);
> >
> > umm, no BUG_ON() is not really a good way to validate userspace input..
> >
> > if (bo->type != ...)
> > return ERR_PTR(-EINVAL);
>
> Yep.
>
> > > +
> > > + switch ((char)(mode_cmd->pixel_format & 0xFF)) {
> > > + case 'Y':
> > > + case 'U':
> > > + case 'V':
> > > + case 'N':
> > > + case 'T':
> >
> > perhaps we should instead add a drm_format_is_yuv().. or you could
> > (ab)use drm_fb_get_bpp_depth()..
>
> Yeah, I think a new drm_format_is_yuv is asked-for here. Now the bigger
> question is why you need this, since the drm core should filter out
> formats not in your list of supported ones. Or at least it should ...
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130810/ab0ce55c/attachment-0001.html>
More information about the dri-devel
mailing list