On Mon, Jul 09, 2018 at 06:44:26PM +0300, Haneen Mohammed wrote:

This patch add the necessary functions to map GEM backing memory into the kernel's virtual address space.

Signed-off-by: Haneen Mohammed hamohammed.sa@gmail.com

drivers/gpu/drm/vkms/vkms_drv.c | 2 ++ drivers/gpu/drm/vkms/vkms_drv.h | 5 ++++ drivers/gpu/drm/vkms/vkms_gem.c | 50 +++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c index fe93f8c17997..e48c8eeb495a 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.c +++ b/drivers/gpu/drm/vkms/vkms_drv.c @@ -52,9 +52,11 @@ static struct drm_driver vkms_driver = { .driver_features = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM, .release = vkms_release, .fops = &vkms_driver_fops,

• .dumb_create = vkms_dumb_create, .dumb_map_offset = vkms_dumb_map, .gem_vm_ops = &vkms_gem_vm_ops,
• .gem_free_object_unlocked = vkms_gem_free_object,

This is a separate bugfix, fixing a fairly huge memory leak. I think it'd be good to catch this in the future, by adding a

else WARN(1, "no gem free callback, leaking memory\n");

to the end of drm_gem_object_free. Can you pls do that?

Also since this line here is a bugfix separate from the vmap support, can you pls split it out?

.name = DRIVER_NAME, .desc = DRIVER_DESC, diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 76f1720f81a5..d339a8108d85 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -35,6 +35,7 @@ struct vkms_gem_object { struct drm_gem_object gem; struct mutex pages_lock; /* Page lock used in page fault handler */ struct page **pages;

• void *vaddr;

};

int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, @@ -58,4 +59,8 @@ int vkms_dumb_create(struct drm_file *file, struct drm_device *dev, int vkms_dumb_map(struct drm_file *file, struct drm_device *dev, u32 handle, u64 *offset);

+void vkms_gem_free_object(struct drm_gem_object *obj);

+void *vkms_gem_vmap(struct drm_gem_object *obj);

#endif /* _VKMS_DRV_H_ */ diff --git a/drivers/gpu/drm/vkms/vkms_gem.c b/drivers/gpu/drm/vkms/vkms_gem.c index 9f820f56b9e0..249855dded63 100644 --- a/drivers/gpu/drm/vkms/vkms_gem.c +++ b/drivers/gpu/drm/vkms/vkms_gem.c @@ -166,3 +166,53 @@ int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,

return ret; }

+void vkms_gem_free_object(struct drm_gem_object *obj) +{

• struct vkms_gem_object *vkms_obj = container_of(obj,

• 					struct vkms_gem_object,
  

• 					gem);
  

• kvfree(vkms_obj->pages);

We need to put the pages here too if ->pages exists, there's a put_pages helper for that.

Also probably a good idea to vunmap here too.

But I think both cases (i.e. vmap not yet released and pages still around) would indicate a bug in the vkms driver of releasing the object while it's still in use somewhere. So maybe also add a

WARN_ON(obj->pages); WARN_ON(obj->vmap);

here to make sure this doesn't happen? -Daniel

• mutex_destroy(&vkms_obj->pages_lock);
• drm_gem_object_release(obj);
• kfree(vkms_obj);

+}

+struct page **get_pages(struct vkms_gem_object *vkms_obj) +{

• struct drm_gem_object *gem_obj = &vkms_obj->gem;
• struct page **pages = vkms_obj->pages;
• if (!pages) {

• mutex_lock(&vkms_obj->pages_lock);
  

• pages = drm_gem_get_pages(gem_obj);
  

• if (IS_ERR(pages)) {
  

• 	mutex_unlock(&vkms_obj->pages_lock);
  

• 	return pages;
  

• }
  

• vkms_obj->pages = pages;
  

• mutex_unlock(&vkms_obj->pages_lock);
  

• }
• return pages;

+}

+void *vkms_gem_vmap(struct drm_gem_object *obj) +{

• void *vaddr = NULL;
• struct vkms_gem_object *vkms_obj = container_of(obj,

• 					struct vkms_gem_object,
  

• 					gem);
  

• unsigned int n_pages = obj->size >> PAGE_SHIFT;
• struct page **pages = get_pages(vkms_obj);
• if (IS_ERR(pages)) {

• DRM_INFO("pages allocation failed %ld\n", PTR_ERR(pages));
  

• return vaddr;
  

• }
• vaddr = vmap(pages, n_pages, VM_MAP, PAGE_KERNEL);
• return vaddr;

+}

2.17.1

On Mon, Jul 09, 2018 at 06:46:49PM +0300, Haneen Mohammed wrote:

This patch map/unmap GEM backing memory to kernel address space in prepare/cleanup_fb respectively and cache the virtual address for later use.

Signed-off-by: Haneen Mohammed hamohammed.sa@gmail.com

drivers/gpu/drm/vkms/vkms_plane.c | 41 +++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c index f7f63143f6d0..f464eb0e05dd 100644 --- a/drivers/gpu/drm/vkms/vkms_plane.c +++ b/drivers/gpu/drm/vkms/vkms_plane.c @@ -9,6 +9,7 @@ #include "vkms_drv.h" #include <drm/drm_plane_helper.h> #include <drm/drm_atomic_helper.h> +#include <drm/drm_gem_framebuffer_helper.h>

static const struct drm_plane_funcs vkms_plane_funcs = { .update_plane = drm_atomic_helper_update_plane, @@ -30,9 +31,49 @@ static void vkms_primary_plane_update(struct drm_plane *plane, { }

+static int vkms_prepare_fb(struct drm_plane *plane,

• 	   struct drm_plane_state *state)
  

+{

• struct drm_gem_object *gem_obj;
• struct vkms_gem_object *vkms_obj;
• if (!state->fb)

• return 0;
  

• gem_obj = drm_gem_fb_get_obj(state->fb, 0);
• vkms_obj = container_of(gem_obj, struct vkms_gem_object, gem);
• vkms_obj->vaddr = vkms_gem_vmap(gem_obj);
• if (!vkms_obj->vaddr)

• DRM_INFO("vmap failed\n");
  

• return drm_gem_fb_prepare_fb(plane, state);

+}

+static void vkms_cleanup_fb(struct drm_plane *plane,

• 	    struct drm_plane_state *old_state)
  

+{

• struct drm_gem_object *gem_obj;
• struct vkms_gem_object *vkms_obj;
• if (!old_state->fb)

• return;
  

• gem_obj = drm_gem_fb_get_obj(old_state->fb, 0);
• vkms_obj = container_of(gem_obj, struct vkms_gem_object, gem);
• if (vkms_obj && vkms_obj->pages) {

• vunmap(vkms_obj->vaddr);
  

Please also clear obj->vaddr here.

• drm_gem_put_pages(gem_obj, vkms_obj->pages, false, true);
  

• vkms_obj->pages = NULL;
  

I think it'd be good to also have a vkms_gem_vunmap helper for this, for symmetry and prettier code.

There's another issue: cleanup/prepare_fb can be called multiple times on the same fb. This doesn't really happen for the current vkms case where we only have 1 plane, but if you have more than one plane you can attach the same fb to all of them. This means we need a vmap counter (probably best put into the vkms_gem_v(un)map functions), and only create the actual vmap when the counter goes from 0 -> 1, and release it only when it goes down to 0 finally. Usually it's called vmap_use_count or so.

Ofc that needs to be protected by the buffer lock, so you need to rework your locking a bit and pull it out into the vmap/vunmap functions.

Cheers, Daniel

• }

+}

static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = { .atomic_check = vkms_plane_atomic_check, .atomic_update = vkms_primary_plane_update,

• .prepare_fb = vkms_prepare_fb,
• .cleanup_fb = vkms_cleanup_fb,

};

struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev)

2.17.1

On Mon, Jul 09, 2018 at 06:47:34PM +0300, Haneen Mohammed wrote:

Call atomic_helper_check_plane_state to clip plane coordinates.

Signed-off-by: Haneen Mohammed hamohammed.sa@gmail.com

drivers/gpu/drm/vkms/vkms_plane.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c index f464eb0e05dd..bf37aebac0fb 100644 --- a/drivers/gpu/drm/vkms/vkms_plane.c +++ b/drivers/gpu/drm/vkms/vkms_plane.c @@ -7,9 +7,10 @@ */

#include "vkms_drv.h" -#include <drm/drm_plane_helper.h> +#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_gem_framebuffer_helper.h> +#include <drm/drm_plane_helper.h>

static const struct drm_plane_funcs vkms_plane_funcs = { .update_plane = drm_atomic_helper_update_plane, @@ -23,7 +24,20 @@ static const struct drm_plane_funcs vkms_plane_funcs = { static int vkms_plane_atomic_check(struct drm_plane *plane, struct drm_plane_state *state) {

• return 0;

• struct drm_crtc_state *crtc_state;
• if (!state->fb || !state->crtc)

• return 0;
  

• crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);
• if (IS_ERR(crtc_state))

• return PTR_ERR(crtc_state);
  

• return drm_atomic_helper_check_plane_state(state, crtc_state,

• 				   DRM_PLANE_HELPER_NO_SCALING,
  

• 				   DRM_PLANE_HELPER_NO_SCALING,
  

• 				   false, true);
  

So one additional thing we need to check here is plane_state->visible (which is computed by the check_plane_state) function. For now we can't handle a disabled primary plane (we'd need to change our crc function to compute the crc of an all-black screen), hence need to reject this: ret = drm_atomic_helper_check_plane_state(state, crtc_state, DRM_PLANE_HELPER_NO_SCALING, DRM_PLANE_HELPER_NO_SCALING, false, true); if (ret != 0) return ret;

/* for now primary plane must be visible and full screen */ if (!plane_state->visible) return -EINVAL;

return 0;

otherwise lgtm. -Daniel

}

static void vkms_primary_plane_update(struct drm_plane *plane,

2.17.1

On Mon, Jul 09, 2018 at 06:48:36PM +0300, Haneen Mohammed wrote:

Subclass CRTC state struct to enable storing driver's private state. This patch only adds the base drm_crtc_state struct and the atomic functions that handle it.

Signed-off-by: Haneen Mohammed hamohammed.sa@gmail.com

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/vkms/vkms_crtc.c | 55 ++++++++++++++++++++++++++++++-- drivers/gpu/drm/vkms/vkms_drv.h | 8 +++++ 2 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c index 84cc05506b09..56206437647d 100644 --- a/drivers/gpu/drm/vkms/vkms_crtc.c +++ b/drivers/gpu/drm/vkms/vkms_crtc.c @@ -10,13 +10,62 @@ #include <drm/drm_atomic_helper.h> #include <drm/drm_crtc_helper.h>

+static void vkms_crtc_reset(struct drm_crtc *crtc) +{

• struct vkms_crtc_state *state = NULL;
• if (crtc->state) {

• state = container_of(crtc->state, struct vkms_crtc_state,
  

• 		     base);
  

• __drm_atomic_helper_crtc_destroy_state(crtc->state);
  

• kfree(state);
  

• crtc->state = NULL;
  

• }
• state = kzalloc(sizeof(*state), GFP_KERNEL);

Hm, using kzalloc here is a good idea instead of kmalloc like in the default functions. Care to write a patch for drm_atomic_helper to replace all the kmalloc with kzalloc?

Thanks, Daniel

• if (!state)

• return;
  

• crtc->state = &state->base;
• crtc->state->crtc = crtc;

+}

+static struct drm_crtc_state * +vkms_crtc_duplicate_state(struct drm_crtc *crtc) +{

• struct vkms_crtc_state *state;
• if (WARN_ON(!crtc->state))

• return NULL;
  

• state = kzalloc(sizeof(*state), GFP_KERNEL);
• if (!state)

• return NULL;
  

• __drm_atomic_helper_crtc_duplicate_state(crtc, &state->base);
• return &state->base;

+}

+static void vkms_crtc_destroy_state(struct drm_crtc *crtc,

• 		    struct drm_crtc_state *state)
  

+{

• struct vkms_crtc_state *vkms_state;
• vkms_state = container_of(state, struct vkms_crtc_state, base);
• __drm_atomic_helper_crtc_destroy_state(state);
• kfree(vkms_state);

+}

static const struct drm_crtc_funcs vkms_crtc_funcs = { .set_config = drm_atomic_helper_set_config, .destroy = drm_crtc_cleanup, .page_flip = drm_atomic_helper_page_flip,

• .reset = drm_atomic_helper_crtc_reset,
• .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
• .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,

• .reset = vkms_crtc_reset,
• .atomic_duplicate_state = vkms_crtc_duplicate_state,
• .atomic_destroy_state = vkms_crtc_destroy_state,

};

static int vkms_crtc_atomic_check(struct drm_crtc *crtc, diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index d339a8108d85..61e367d32308 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -19,6 +19,14 @@ static const u32 vkms_formats[] = { DRM_FORMAT_XRGB8888, };

+/**

• • vkms_crtc_state - Driver specific CRTC state
• • @base: base CRTC state
• */

+struct vkms_crtc_state {

• struct drm_crtc_state base;

+};

struct vkms_output { struct drm_crtc crtc; struct drm_encoder encoder; -- 2.17.1