Re: [PATCH 1/2] drm: Add DRM_CAP_PRIME_SCANOUT.

[Christopher James Halse Rogers <chris@cooperteam.net>]

On 4 April 2017 6:31:12 pm AEST, Daniel Vetter <daniel@ffwll.ch> wrote:
>On Tue, Apr 04, 2017 at 06:13:20PM +1000, raof@ubuntu.com wrote:
>> From: Christopher James Halse Rogers <raof@ubuntu.com>
>> 
>> Until recently, on (at least) nouveau, radeon, and amdgpu attempting
>to scanout of an
>> imported dma-buf would silently result in the dma-buf sharing being
>broken.
>> 
>> While the hardware is capable of scanning out of imported dma-bufs
>(at least in some circumstances),
>> these drivers do not currently implement it, so attempts to scan out
>of such buffers will never succeed.
>> 
>> Add a userspace-visible drm capability and associated driver_feature
>so that userspace can discover
>> when scanning out of an imported dma-buf can work.
>> 
>> Signed-off-by: Christopher James Halse Rogers
><christopher.halse.rogers@canonical.com>
>
>This seems like an awful specific special case. Why can't you do the
>entire dance upfront, i.e. import buffer, addfb2? None of that has any
>visible effect, and it will also allow you to check runtime constraints
>which can't be covered with this here.
>-Daniel

No, because addfb2 doesn't (or, rather, didn't) actually check any runtime constraints.

The problem only appeared when the buffer is actually *used* in a modeset - otherwise I could do a (reasonably) cheap import/addfb/render on exporter/read out on importer dance to detect it.

To be clear - this is trying to solve the problem “how can I tell if it's safe to addfb/pageflip rather than do a GL copy to a GPU-local buffer and flip from that?”.

If I could guarantee that I'd only ever run on 4.13-or-later kernels (I think that's when the previous patches will land?), then this would indeed be mostly unnecessary. It would save me a bunch of addfb calls that would predictably fail, but they're cheap.

>
>> ---
>>  drivers/gpu/drm/drm_ioctl.c |  3 +++
>>  include/drm/drm_drv.h       |  1 +
>>  include/uapi/drm/drm.h      | 21 +++++++++++++++++++++
>>  3 files changed, 25 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/drm_ioctl.c
>b/drivers/gpu/drm/drm_ioctl.c
>> index a7c61c23685a..79ccf638668e 100644
>> --- a/drivers/gpu/drm/drm_ioctl.c
>> +++ b/drivers/gpu/drm/drm_ioctl.c
>> @@ -285,6 +285,9 @@ static int drm_getcap(struct drm_device *dev,
>void *data, struct drm_file *file_
>>  	case DRM_CAP_ADDFB2_MODIFIERS:
>>  		req->value = dev->mode_config.allow_fb_modifiers;
>>  		break;
>> +	case DRM_CAP_PRIME_SCANOUT:
>> +		req->value = drm_core_check_feature(dev, DRIVER_PRIME_SCANOUT);
>> +		break;
>>  	default:
>>  		return -EINVAL;
>>  	}
>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> index 5699f42195fe..32cc0d956d7e 100644
>> --- a/include/drm/drm_drv.h
>> +++ b/include/drm/drm_drv.h
>> @@ -53,6 +53,7 @@ struct drm_mode_create_dumb;
>>  #define DRIVER_RENDER			0x8000
>>  #define DRIVER_ATOMIC			0x10000
>>  #define DRIVER_KMS_LEGACY_CONTEXT	0x20000
>> +#define DRIVER_PRIME_SCANOUT		0x40000
>>  
>>  /**
>>   * struct drm_driver - DRM driver structure
>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> index b2c52843bc70..c57213cdb702 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -647,6 +647,27 @@ struct drm_gem_open {
>>  #define DRM_CAP_CURSOR_HEIGHT		0x9
>>  #define DRM_CAP_ADDFB2_MODIFIERS	0x10
>>  #define DRM_CAP_PAGE_FLIP_TARGET	0x11
>> +/**
>> + * DRM_CAP_PRIME_SCANOUT
>> + *
>> + * The PRIME_SCANOUT capability returns whether the driver might be
>capable
>> + * of scanning out of an imported PRIME buffer.
>> + *
>> + * This does not guarantee that any imported buffer can be scanned
>out, as
>> + * there may be specific requirements that a given buffer might not
>satisfy.
>> + *
>> + * If this capability is false then imported PRIME buffers will
>definitely
>> + * never be suitable for scanout.
>> + *
>> + * Note: Prior to the introduction of this capability, bugs in
>drivers would
>> + * allow userspace to attempt to scan out of imported PRIME buffers.
>This would
>> + * work once, but move the buffer into an inconsistent state where
>rendering from
>> + * the exporting GPU would no longer be seen by the importing GPU.
>> + *
>> + * It is unsafe for driver-agnostic code to attempt to scan out of
>an imported
>> + * PRIME buffer in the absense of this capability.
>> + */
>> +#define DRM_CAP_PRIME_SCANOUT		0x12
>>  
>>  /** DRM_IOCTL_GET_CAP ioctl argument type */
>>  struct drm_get_cap {
>> -- 
>> 2.11.0
>> 
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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