2013/8/8 Daniel Vetter <daniel@xxxxxxxx>
Just comment a bit on Rob's review with my own opinion.
Yeah, I think the KDS bits and comments need to go first before merginge.
On Wed, Aug 07, 2013 at 12:17:21PM -0400, Rob Clark wrote:
> On Thu, Jul 25, 2013 at 1:17 PM, <tom.cooksey@xxxxxxx> wrote:
> > From: Tom Cooksey <tom.cooksey@xxxxxxx>
> >
> > 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, if this is just a sw queue then I don't think it makes sense to have
> > +/*
> > + * 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).
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.
I'd say that's just backwards, framebuffers are created from backing
>
> > +/*
> > + * 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.
storage objects (which for a gem based driver is a gem object), not the
other way round. What's this exactly used for?
[snip]
The race is real, I have an evil testcase here which Oopses my kernel. I'm
> > +
> > + /*
> > + * 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.
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]
Drivers have no business frobbing around the dma-buf refcount of imported
> > +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.
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]
Yep.
> > +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);
Yeah, I think a new drm_format_is_yuv is asked-for here. Now the bigger
> > +
> > + 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()..
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
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
