Re: [RFC PATCH] drm/panfrost: Add initial panfrost driver

[Eric Anholt <eric@anholt.net>]

[-- Attachment #1.1: Type: text/plain, Size: 5665 bytes --]

Rob Herring <robh@kernel.org> writes:

> From: "Marty E. Plummer" <hanetzer@startmail.com>
>
> This adds the initial driver for panfrost which supports Arm Mali
> Midgard and Bifrost family of GPUs. Currently, only the T860 Midgard GPU
> has been tested.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Eric Anholt <eric@anholt.net>
> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Sending this out in the spirit of release early, release often. We're 
> close to parity compared to mesa + the vendor driver. There's a few 
> issues Tomeu is chasing. 
>
> There's still some pieces of the h/w setup we've just hardcoded. Locking 
> in various places is probably missing. Error recovery is non-existent 
> (other than module unload/load). There's some work to add tracepoints 
> and perf counters that's not here yet. Bifrost GPUs are definitely not 
> supported yet other than identifying them. Primarily the MMU setup is 
> missing.
>
> How's performance? Great, because I haven't measured it.
>
> This patch and its dependencies are available here[1]. The mesa support 
> is here[2]. Both are likely to change (daily).

My inclination would be to merge this soon, when basic checkpatch is in
shape.  The UABI looks good, and that's the important part.

> +static void panfrost_unlock_bo_reservations(struct drm_gem_object **bos,
> +					    int bo_count,
> +					    struct ww_acquire_ctx *acquire_ctx)
> +{
> +	int i;
> +
> +	for (i = 0; i < bo_count; i++) {
> +		ww_mutex_unlock(&bos[i]->resv->lock);
> +	}
> +	ww_acquire_fini(acquire_ctx);
> +}
> +
> +/* Takes the reservation lock on all the BOs being referenced, so that
> + * at queue submit time we can update the reservations.
> + *
> + * We don't lock the RCL the tile alloc/state BOs, or overflow memory
> + * (all of which are on exec->unref_list).  They're entirely private
> + * to panfrost, so we don't attach dma-buf fences to them.
> + */
> +static int panfrost_lock_bo_reservations(struct drm_gem_object **bos,
> +					 int bo_count,
> +					 struct ww_acquire_ctx *acquire_ctx)
> +{
> +	int contended_lock = -1;
> +	int i, ret;
> +
> +	ww_acquire_init(acquire_ctx, &reservation_ww_class);
> +
> +retry:
> +	if (contended_lock != -1) {
> +		struct drm_gem_object *bo = bos[contended_lock];
> +
> +		ret = ww_mutex_lock_slow_interruptible(&bo->resv->lock,
> +						       acquire_ctx);
> +		if (ret) {
> +			ww_acquire_done(acquire_ctx);
> +			return ret;
> +		}
> +	}
> +
> +	for (i = 0; i < bo_count; i++) {
> +		if (i == contended_lock)
> +			continue;
> +
> +		ret = ww_mutex_lock_interruptible(&bos[i]->resv->lock,
> +						  acquire_ctx);
> +		if (ret) {
> +			int j;
> +
> +			for (j = 0; j < i; j++)
> +				ww_mutex_unlock(&bos[j]->resv->lock);
> +
> +			if (contended_lock != -1 && contended_lock >= i) {
> +				struct drm_gem_object *bo = bos[contended_lock];
> +
> +				ww_mutex_unlock(&bo->resv->lock);
> +			}
> +
> +			if (ret == -EDEADLK) {
> +				contended_lock = i;
> +				goto retry;
> +			}
> +
> +			ww_acquire_done(acquire_ctx);
> +			return ret;
> +		}
> +	}
> +
> +	ww_acquire_done(acquire_ctx);
> +
> +	/* Reserve space for our shared (read-only) fence references,
> +	 * before we commit the job to the hardware.
> +	 */
> +	for (i = 0; i < bo_count; i++) {
> +		ret = reservation_object_reserve_shared(bos[i]->resv, 1);
> +		if (ret) {
> +			panfrost_unlock_bo_reservations(bos, bo_count,
> +						   acquire_ctx);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}

I just sent out my shared helpers for most of this function, hopefully
you can use them.

It looks like you've got v3d's silliness with the fences -- we reserve a
shared slot, then use excl only anyway.  For v3d I'm planning on moving
to just excl -- only one of my entrypoints has info on write vs
read-only, and I don't know of a usecase where having multiple read-only
consumers of a shared buffer simultaneously matters.

More importantly, I think you also have my bug of not doing implicit
synchronization on buffers, which will break X11 rendering
sometimes. X11's GL requirements are that previously-submitted rendering
by the client fd will execute before X11's rendering on its fd to the
same buffers.  If you're running a single client, X11's copies are cheap
enough that it'll probably work out most of the time.

> --- /dev/null
> +++ b/include/uapi/drm/panfrost_drm.h

> +#define DRM_IOCTL_PANFROST_SUBMIT		DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_SUBMIT, struct drm_panfrost_submit)
> +#define DRM_IOCTL_PANFROST_WAIT_BO		DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_WAIT_BO, struct drm_panfrost_wait_bo)
> +#define DRM_IOCTL_PANFROST_CREATE_BO		DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_CREATE_BO, struct drm_panfrost_create_bo)
> +#define DRM_IOCTL_PANFROST_MMAP_BO		DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_MMAP_BO, struct drm_panfrost_mmap_bo)
> +#define DRM_IOCTL_PANFROST_GET_PARAM		DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_GET_PARAM, struct drm_panfrost_get_param)
> +#define DRM_IOCTL_PANFROST_GET_BO_OFFSET	DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_GET_BO_OFFSET, struct drm_panfrost_get_bo_offset)

SUBMIT and WAIT_BO might be IOR instead of IOWR

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel