Re: [PATCH v2 1/6] drm/client: Support unmapping of DRM client buffers
Den 05.07.2019 11.26, skrev Thomas Zimmermann:
> DRM clients, such as the fbdev emulation, have their buffer objects
> mapped by default. Mapping a buffer implicitly prevents its relocation.
> Hence, the buffer may permanently consume video memory while it's
> allocated. This is a problem for drivers of low-memory devices, such as
> ast, mgag200 or older framebuffer hardware, which will then not have
> enough memory to display other content (e.g., X11).
>
> This patch introduces drm_client_buffer_vmap() and _vunmap(). Internal
> DRM clients can use these functions to unmap and remap buffer objects
> as needed.
>
> There's no reference counting for vmap operations. Callers are expected
> to either keep buffers mapped (as it is now), or call vmap and vunmap
> in pairs around code that accesses the mapped memory.
>
> v2:
> * remove several duplicated NULL-pointer checks
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_client.c | 67 ++--
> include/drm/drm_client.h | 3 ++
> 2 files changed, 60 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index 410572f14257..66d8d645ac79 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -281,6 +281,43 @@ drm_client_buffer_create(struct drm_client_dev *client,
> u32 width, u32 height, u
>
> buffer->gem = obj;
>
> + vaddr = drm_client_buffer_vmap(buffer);
> + if (IS_ERR(vaddr)) {
> + ret = PTR_ERR(vaddr);
> + goto err_delete;
> + }
> +
> + return buffer;
> +
> +err_delete:
> + drm_client_buffer_delete(buffer);
> +
> + return ERR_PTR(ret);
> +}
> +
> +/**
> + * drm_client_buffer_vmap - Map DRM client buffer into address space
> + * @buffer: DRM client buffer
> + *
> + * This function maps a client buffer into kernel address space. If the
> + * buffer is already mapped, it returns the mapping's address.
> + *
> + * Client buffer mappings are not ref'counted. Each call to
> + * drm_client_buffer_vmap() should be followed by a call to
> + * drm_client_buffer_vunmap(); or the client buffer should be mapped
> + * throughout its lifetime. The latter is the default.
> + *
> + * Returns:
> + * The mapped memory's address
> + */
> +void *
> +drm_client_buffer_vmap(struct drm_client_buffer *buffer)
I prefer to have this on one line.
> +{
> + void *vaddr;
> +
> + if (buffer->vaddr)
> + return buffer->vaddr;
> +
> /*
>* FIXME: The dependency on GEM here isn't required, we could
>* convert the driver handle to a dma-buf instead and use the
> @@ -289,21 +326,31 @@ drm_client_buffer_create(struct drm_client_dev *client,
> u32 width, u32 height, u
>* fd_install step out of the driver backend hooks, to make that
>* final step optional for internal users.
>*/
> - vaddr = drm_gem_vmap(obj);
> - if (IS_ERR(vaddr)) {
> - ret = PTR_ERR(vaddr);
> - goto err_delete;
> - }
> + vaddr = drm_gem_vmap(buffer->gem);
> + if (IS_ERR(vaddr))
> + return vaddr;
>
> buffer->vaddr = vaddr;
>
> - return buffer;
> -
> -err_delete:
> - drm_client_buffer_delete(buffer);
> + return vaddr;
> +}
> +EXPORT_SYMBOL(drm_client_buffer_vmap);
>
> - return ERR_PTR(ret);
> +/**
> + * drm_client_buffer_vunmap - Unmap DRM client buffer
> + * @buffer: DRM client buffer
> + *
> + * This function removes a client buffer's memory mmapping. This
s/mmapping/mapping/
> + * function is only required by clients that manage their buffers
> + * by themselves. By default, DRM client buffers are mapped throughout
> + * their entire lifetime.
> + */
> +void drm_client_buffer_vunmap(struct drm_client_buffer *buffer)
> +{
> + drm_gem_vunmap(buffer->gem, buffer->vaddr);
> + buffer->vaddr = NULL;
> }
> +EXPORT_SYMBOL(drm_client_buffer_vunmap);
>
> static void drm_client_buffer_rmfb(struct drm_client_buffer *buffer)
> {
> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
> index 72d51d1e9dd9..e1db1d9da0bf 100644
> --- a/include/drm/drm_client.h
> +++ b/include/drm/drm_client.h
> @@ -149,6 +149,9 @@ struct drm_client_buffer {
> struct drm_client_buffer *
> drm_client_framebuffer_create(struct drm_client_dev *client, u32 width, u32
> height, u32 format);
> void drm_client_framebuffer_delete(struct drm_client_buffer *buffer);
> +void *
> +drm_client_buffer_vmap(struct drm_client_buffer *buffer);
Prefer to have this on one line.
Reviewed-by: Noralf Trønnes
> +void drm_client_buffer_vunmap(struct drm_client_buffer *buffer);
>
> int drm_client_modeset_create(struct drm_client_dev *client);
> void drm_client_modeset_free(struct drm_client_dev *client);
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/6] drm/fb-helper: Map DRM client buffer only when required
Den 05.07.2019 11.26, skrev Thomas Zimmermann: > This patch changes DRM clients to not map the buffer by default. The > buffer, like any buffer object, should be mapped and unmapped when > needed. > > An unmapped buffer object can be evicted to system memory and does > not consume video ram until displayed. This allows to use generic fbdev > emulation with drivers for low-memory devices, such as ast and mgag200. > > This change affects the generic framebuffer console. HW-based consoles > map their console buffer once and keep it mapped. Userspace can mmap this > buffer into its address space. The shadow-buffered framebuffer console > only needs the buffer object to be mapped during updates. While not being > updated from the shadow buffer, the buffer object can remain unmapped. > Userspace will always mmap the shadow buffer. > > v2: > * change DRM client to not map buffer by default > * manually map client buffer for fbdev with HW framebuffer > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Noralf Trønnes ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 3/6] drm/fb-helper: Instanciate shadow FB if configured in device's mode_config
Den 05.07.2019 11.26, skrev Thomas Zimmermann:
> Generic framebuffer emulation uses a shadow buffer for framebuffers with
> dirty() function. If drivers want to use the shadow FB without such a
> function, they can now set prefer_shadow or prefer_shadow_fbdev in their
> mode_config structures. The former flag is exported to userspace, the latter
> flag is fbdev-only.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_fb_helper.c | 19 ++-
> include/drm/drm_mode_config.h | 5 +
> 2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 7ba6a0255821..56ef169e1814 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -421,7 +421,9 @@ static void drm_fb_helper_dirty_work(struct work_struct
> *work)
> return;
> drm_fb_helper_dirty_blit_real(helper, &clip_copy);
> }
> - helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1);
> + if (helper->fb->funcs->dirty)
> + helper->fb->funcs->dirty(helper->fb, NULL, 0, 0,
> + &clip_copy, 1);
>
> if (helper->buffer)
> drm_client_buffer_vunmap(helper->buffer);
> @@ -620,9 +622,6 @@ static void drm_fb_helper_dirty(struct fb_info *info, u32
> x, u32 y,
> struct drm_clip_rect *clip = &helper->dirty_clip;
> unsigned long flags;
>
> - if (!helper->fb->funcs->dirty)
> - return;
drm_fb_helper_dirty() is called unconditionally by
drm_fb_helper_sys_imageblit() et al, so we need check with
drm_fbdev_use_shadow_fb() here.
> -
> spin_lock_irqsave(&helper->dirty_lock, flags);
> clip->x1 = min_t(u32, clip->x1, x);
> clip->y1 = min_t(u32, clip->y1, y);
> @@ -2166,6 +2165,16 @@ static struct fb_deferred_io drm_fbdev_defio = {
> .deferred_io= drm_fb_helper_deferred_io,
> };
>
> +static bool drm_fbdev_use_shadow_fb(struct drm_fb_helper *fb_helper)
> +{
> + struct drm_device *dev = fb_helper->dev;
> + struct drm_framebuffer *fb = fb_helper->fb;
> +
> + return dev->mode_config.prefer_shadow_fbdev |
> +dev->mode_config.prefer_shadow |
Use logical OR here
> +!!fb->funcs->dirty;
and you can drop the the double NOT here.
> +}
> +
> /**
> * drm_fb_helper_generic_probe - Generic fbdev emulation probe helper
> * @fb_helper: fbdev helper structure
> @@ -2213,7 +,7 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper
> *fb_helper,
>
> drm_fb_helper_fill_info(fbi, fb_helper, sizes);
>
> - if (fb->funcs->dirty) {
> + if (drm_fbdev_use_shadow_fb(fb_helper)) {
> struct fb_ops *fbops;
> void *shadow;
>
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 759d462d028b..e1c751aca353 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -347,6 +347,8 @@ struct drm_mode_config_funcs {
> * @output_poll_work: delayed work for polling in process context
> * @preferred_depth: preferred RBG pixel depth, used by fb helpers
> * @prefer_shadow: hint to userspace to prefer shadow-fb rendering
> + * @prefer_shadow_fbdev: hint to framebuffer emulation to prefer shadow-fb \
> + rendering
It's preferred to have the doc together with the struct member. This way
it's less likely to be forgotten when things change. And we don't use
line cont. when the doc line is too long. Just continue on the next line
after an asterix.
> * @cursor_width: hint to userspace for max cursor width
> * @cursor_height: hint to userspace for max cursor height
> * @helper_private: mid-layer private data
> @@ -852,6 +854,9 @@ struct drm_mode_config {
> /* dumb ioctl parameters */
> uint32_t preferred_depth, prefer_shadow;
>
> + /* fbdev parameters */
No need for this comment.
Doc can look like this, I've done s/framebuffer/fbdev/:
/**
* @prefer_shadow_fbdev:
*
* Hint to fbdev emulation to prefer shadow-fb rendering.
*/
> + uint32_t prefer_shadow_fbdev;
Use bool here.
With that:
Reviewed-by: Noralf Trønnes
I have tested this on 2 drivers that use generic fbdev: vc4 (no shadow
buf) and mi0283qt which has a dirty callback.
Tested-by: Noralf Trønnes
> +
> /**
>* @quirk_addfb_prefer_xbgr_30bpp:
>*
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 3/6] drm/fb-helper: Instanciate shadow FB if configured in device's mode_config
Hi
Am 07.07.19 um 16:37 schrieb Noralf Trønnes:
>
>
> Den 05.07.2019 11.26, skrev Thomas Zimmermann:
>> Generic framebuffer emulation uses a shadow buffer for framebuffers with
>> dirty() function. If drivers want to use the shadow FB without such a
>> function, they can now set prefer_shadow or prefer_shadow_fbdev in their
>> mode_config structures. The former flag is exported to userspace, the latter
>> flag is fbdev-only.
>>
>> Signed-off-by: Thomas Zimmermann
>> ---
>> drivers/gpu/drm/drm_fb_helper.c | 19 ++-
>> include/drm/drm_mode_config.h | 5 +
>> 2 files changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>> b/drivers/gpu/drm/drm_fb_helper.c
>> index 7ba6a0255821..56ef169e1814 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -421,7 +421,9 @@ static void drm_fb_helper_dirty_work(struct work_struct
>> *work)
>> return;
>> drm_fb_helper_dirty_blit_real(helper, &clip_copy);
>> }
>> -helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1);
>> +if (helper->fb->funcs->dirty)
>> +helper->fb->funcs->dirty(helper->fb, NULL, 0, 0,
>> + &clip_copy, 1);
>>
>> if (helper->buffer)
>> drm_client_buffer_vunmap(helper->buffer);
>> @@ -620,9 +622,6 @@ static void drm_fb_helper_dirty(struct fb_info *info,
>> u32 x, u32 y,
>> struct drm_clip_rect *clip = &helper->dirty_clip;
>> unsigned long flags;
>>
>> -if (!helper->fb->funcs->dirty)
>> -return;
>
> drm_fb_helper_dirty() is called unconditionally by
> drm_fb_helper_sys_imageblit() et al, so we need check with
> drm_fbdev_use_shadow_fb() here.
>
>> -
>> spin_lock_irqsave(&helper->dirty_lock, flags);
>> clip->x1 = min_t(u32, clip->x1, x);
>> clip->y1 = min_t(u32, clip->y1, y);
>> @@ -2166,6 +2165,16 @@ static struct fb_deferred_io drm_fbdev_defio = {
>> .deferred_io= drm_fb_helper_deferred_io,
>> };
>>
>> +static bool drm_fbdev_use_shadow_fb(struct drm_fb_helper *fb_helper)
>> +{
>> +struct drm_device *dev = fb_helper->dev;
>> +struct drm_framebuffer *fb = fb_helper->fb;
>> +
>> +return dev->mode_config.prefer_shadow_fbdev |
>> + dev->mode_config.prefer_shadow |
>
> Use logical OR here
>
>> + !!fb->funcs->dirty;
>
> and you can drop the the double NOT here.
>
>> +}
>> +
>> /**
>> * drm_fb_helper_generic_probe - Generic fbdev emulation probe helper
>> * @fb_helper: fbdev helper structure
>> @@ -2213,7 +,7 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper
>> *fb_helper,
>>
>> drm_fb_helper_fill_info(fbi, fb_helper, sizes);
>>
>> -if (fb->funcs->dirty) {
>> +if (drm_fbdev_use_shadow_fb(fb_helper)) {
>> struct fb_ops *fbops;
>> void *shadow;
>>
>> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
>> index 759d462d028b..e1c751aca353 100644
>> --- a/include/drm/drm_mode_config.h
>> +++ b/include/drm/drm_mode_config.h
>> @@ -347,6 +347,8 @@ struct drm_mode_config_funcs {
>> * @output_poll_work: delayed work for polling in process context
>> * @preferred_depth: preferred RBG pixel depth, used by fb helpers
>> * @prefer_shadow: hint to userspace to prefer shadow-fb rendering
>> + * @prefer_shadow_fbdev: hint to framebuffer emulation to prefer shadow-fb \
>> +rendering
>
> It's preferred to have the doc together with the struct member.
I just tried to follow the file's existing style, but OK, I don't mind.
> it's less likely to be forgotten when things change. And we don't use
> line cont. when the doc line is too long. Just continue on the next line
> after an asterix.
>
>> * @cursor_width: hint to userspace for max cursor width
>> * @cursor_height: hint to userspace for max cursor height
>> * @helper_private: mid-layer private data
>> @@ -852,6 +854,9 @@ struct drm_mode_config {
>> /* dumb ioctl parameters */
>> uint32_t preferred_depth, prefer_shadow;
>>
>> +/* fbdev parameters */
>
> No need for this comment.
>
> Doc can look like this, I've done s/framebuffer/fbdev/:
> /**
>* @prefer_shadow_fbdev:
>*
>* Hint to fbdev emulation to prefer shadow-fb rendering.
>*/
>
>> +uint32_t prefer_shadow_fbdev;
>
> Use bool here.
>
> With that:
>
> Reviewed-by: Noralf Trønnes
>
> I have tested this on 2 drivers that use generic fbdev: vc4 (no shadow
> buf) and mi0283qt which has a dirty callback.
>
> Tested-by: Noralf Trønnes
Thanks for reviewing and testing the patches.
Best regards
Thomas
>
>> +
>> /**
>> * @quirk_addfb_prefer_xbgr_30bpp:
>> *
>>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/li
Re: [PATCH v15 6/7] ext4: disable map_sync for async flush
On Fri, Jul 05, 2019 at 07:33:27PM +0530, Pankaj Gupta wrote: > Dont support 'MAP_SYNC' with non-DAX files and DAX files > with asynchronous dax_device. Virtio pmem provides > asynchronous host page cache flush mechanism. We don't > support 'MAP_SYNC' with virtio pmem and ext4. > > Signed-off-by: Pankaj Gupta > Reviewed-by: Jan Kara Acked-by: Theodore Ts'o ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC v2] vhost: introduce mdev based hardware vhost backend
On Fri, Jul 05, 2019 at 08:49:46AM -0600, Alex Williamson wrote:
> On Thu, 4 Jul 2019 14:21:34 +0800
> Tiwei Bie wrote:
> > On Thu, Jul 04, 2019 at 12:31:48PM +0800, Jason Wang wrote:
> > > On 2019/7/3 下午9:08, Tiwei Bie wrote:
> > > > On Wed, Jul 03, 2019 at 08:16:23PM +0800, Jason Wang wrote:
> > > > > On 2019/7/3 下午7:52, Tiwei Bie wrote:
> > > > > > On Wed, Jul 03, 2019 at 06:09:51PM +0800, Jason Wang wrote:
> > > > > > > On 2019/7/3 下午5:13, Tiwei Bie wrote:
> > > > > > > > Details about this can be found here:
> > > > > > > >
> > > > > > > > https://lwn.net/Articles/750770/
> > > > > > > >
> > > > > > > > What's new in this version
> > > > > > > > ==
> > > > > > > >
> > > > > > > > A new VFIO device type is introduced - vfio-vhost. This
> > > > > > > > addressed
> > > > > > > > some comments from
> > > > > > > > here:https://patchwork.ozlabs.org/cover/984763/
> > > > > > > >
> > > > > > > > Below is the updated device interface:
> > > > > > > >
> > > > > > > > Currently, there are two regions of this device: 1)
> > > > > > > > CONFIG_REGION
> > > > > > > > (VFIO_VHOST_CONFIG_REGION_INDEX), which can be used to setup the
> > > > > > > > device; 2) NOTIFY_REGION (VFIO_VHOST_NOTIFY_REGION_INDEX), which
> > > > > > > > can be used to notify the device.
> > > > > > > >
> > > > > > > > 1. CONFIG_REGION
> > > > > > > >
> > > > > > > > The region described by CONFIG_REGION is the main control
> > > > > > > > interface.
> > > > > > > > Messages will be written to or read from this region.
> > > > > > > >
> > > > > > > > The message type is determined by the `request` field in message
> > > > > > > > header. The message size is encoded in the message header too.
> > > > > > > > The message format looks like this:
> > > > > > > >
> > > > > > > > struct vhost_vfio_op {
> > > > > > > > __u64 request;
> > > > > > > > __u32 flags;
> > > > > > > > /* Flag values: */
> > > > > > > > #define VHOST_VFIO_NEED_REPLY 0x1 /* Whether need reply */
> > > > > > > > __u32 size;
> > > > > > > > union {
> > > > > > > > __u64 u64;
> > > > > > > > struct vhost_vring_state state;
> > > > > > > > struct vhost_vring_addr addr;
> > > > > > > > } payload;
> > > > > > > > };
> > > > > > > >
> > > > > > > > The existing vhost-kernel ioctl cmds are reused as the message
> > > > > > > > requests in above structure.
> > > > > > > Still a comments like V1. What's the advantage of inventing a new
> > > > > > > protocol?
> > > > > > I'm trying to make it work in VFIO's way..
> > > > > >
> > > > > > > I believe either of the following should be better:
> > > > > > >
> > > > > > > - using vhost ioctl, we can start from
> > > > > > > SET_VRING_KICK/SET_VRING_CALL and
> > > > > > > extend it with e.g notify region. The advantages is that all
> > > > > > > exist userspace
> > > > > > > program could be reused without modification (or minimal
> > > > > > > modification). And
> > > > > > > vhost API hides lots of details that is not necessary to be
> > > > > > > understood by
> > > > > > > application (e.g in the case of container).
> > > > > > Do you mean reusing vhost's ioctl on VFIO device fd directly,
> > > > > > or introducing another mdev driver (i.e. vhost_mdev instead of
> > > > > > using the existing vfio_mdev) for mdev device?
> > > > > Can we simply add them into ioctl of mdev_parent_ops?
> > > > Right, either way, these ioctls have to be and just need to be
> > > > added in the ioctl of the mdev_parent_ops. But another thing we
> > > > also need to consider is that which file descriptor the userspace
> > > > will do the ioctl() on. So I'm wondering do you mean let the
> > > > userspace do the ioctl() on the VFIO device fd of the mdev
> > > > device?
> > > >
> > >
> > > Yes.
> >
> > Got it! I'm not sure what's Alex opinion on this. If we all
> > agree with this, I can do it in this way.
> >
> > > Is there any other way btw?
> >
> > Just a quick thought.. Maybe totally a bad idea. I was thinking
> > whether it would be odd to do non-VFIO's ioctls on VFIO's device
> > fd. So I was wondering whether it's possible to allow binding
> > another mdev driver (e.g. vhost_mdev) to the supported mdev
> > devices. The new mdev driver, vhost_mdev, can provide similar
> > ways to let userspace open the mdev device and do the vhost ioctls
> > on it. To distinguish with the vfio_mdev compatible mdev devices,
> > the device API of the new vhost_mdev compatible mdev devices
> > might be e.g. "vhost-net" for net?
> >
> > So in VFIO case, the device will be for passthru directly. And
> > in VHOST case, the device can be used to accelerate the existing
> > virtualized devices.
> >
> > How do you think?
>
> VFIO really can't prevent vendor specific ioctls on the device file
> descriptor for mdevs, but a) we'd want to be sure the ioctl address
> space can't collide with ioctls we'd use
