from:"Oleksandr Andrushchenko"


On 8/13/20 6:13 PM, Jürgen Groß wrote:
> On 13.08.20 17:10, Oleksandr Andrushchenko wrote:
>>
>> On 8/13/20 6:02 PM, Jürgen Groß wrote:
>>> On 13.08.20 08:21, Oleksandr Andrushchenko wrote:
>>>> From: Oleksandr Andrushchenko 
>>>
>>> Series pushed to:
>>>
>>> xen/tip.git for-linus-5.9
>>>
>> The top patch has strange title though:
>>
>> "Subject: [PATCH v2 5/5] drm/xen-front: Pass dumb buffer data offset to the 
>> backend"
>
> Oh, indeed. I had to repair it manually as it seems some mail system
> (probably on my end) mangled it a little bit.
>
> Will repair it.
>
Now everything is great,

Thank you!

>
> Juergen

Re: [PATCH v2 0/5] Fixes and improvements for Xen pvdrm


On 8/13/20 6:02 PM, Jürgen Groß wrote:
> On 13.08.20 08:21, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko 
>
> Series pushed to:
>
> xen/tip.git for-linus-5.9
>
The top patch has strange title though:

"Subject: [PATCH v2 5/5] drm/xen-front: Pass dumb buffer data offset to the 
backend"

>
> Juergen

Thank you,

Oleksandr

Re: [PATCH v2 0/5] Fixes and improvements for Xen pvdrm

Juergen, Boris,

can we please merge these via Xen Linux tree as I have collected enough Ack/R-b?

The series has DRM patches, but those anyway are Xen related, so I think

this should be fine from DRI point of view.

Thank you,

Oleksandr

On 8/13/20 9:21 AM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
>
> Hello,
>
> This series contains an assorted set of fixes and improvements for
> the Xen para-virtualized display driver and grant device driver which
> I have collected over the last couple of months:
>
> 1. Minor fixes to grant device driver and drm/xen-front.
>
> 2. New format (YUYV) added to the list of the PV DRM supported formats
> which allows the driver to be used in zero-copying use-cases when
> a camera device is the source of the dma-bufs.
>
> 3. Synchronization with the latest para-virtualized protocol definition
> in Xen [1].
>
> 4. SGT offset is now propagated to the backend: while importing a dmabuf
> it is possible that the data of the buffer is put with offset which is
> indicated by the SGT offset. This is needed for some GPUs which have
> non-zero offset.
>
> Thank you,
> Oleksandr Andrushchenko
>
> [1] 
> https://urldefense.com/v3/__https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=c27a184225eab54d20435c8cab5ad0ef384dc2c0__;!!GF_29dbcQIUBPA!iAHOdk4M167VNM1AypMGVmyKJu-iqC9e5cXyu6N595Np3iyIZDnZl0MIBX3IROJSD1GSMX_GfQ$
>  [xenbits[.]xen[.]org]
>
> Changes since v1:
> =
>
> 1. Removed patch which adds EDID to PV DRM as it needs more time for review:
> "5. Version 2 of the Xen displif protocol adds XENDISPL_OP_GET_EDID
> request which allows frontends to request EDID structure per
> connector. This request is optional and if not supported by the
> backend then visible area is still defined by the relevant
> XenStore's "resolution" property.
> If backend provides EDID with XENDISPL_OP_GET_EDID request then
> its values must take precedence over the resolutions defined in
> XenStore."
> I will send this as a dedicated patch.
>
> 2. Added missing CC stable for the patches with fixes
>
> Oleksandr Andrushchenko (5):
>xen/gntdev: Fix dmabuf import with non-zero sgt offset
>drm/xen-front: Fix misused IS_ERR_OR_NULL checks
>drm/xen-front: Add YUYV to supported formats
>xen: Sync up with the canonical protocol definition in Xen
>drm/xen-front: Pass dumb buffer data offset to the backend
>
>   drivers/gpu/drm/xen/xen_drm_front.c  | 10 +--
>   drivers/gpu/drm/xen/xen_drm_front.h  |  2 +-
>   drivers/gpu/drm/xen/xen_drm_front_conn.c |  1 +
>   drivers/gpu/drm/xen/xen_drm_front_gem.c  | 11 +--
>   drivers/gpu/drm/xen/xen_drm_front_kms.c  |  2 +-
>   drivers/xen/gntdev-dmabuf.c  |  8 +++
>   include/xen/interface/io/displif.h   | 91 +++-
>   7 files changed, 111 insertions(+), 14 deletions(-)
>

Re: [PATCH v2 0/5] Fixes and improvements for Xen pvdrm


On 8/13/20 10:05 AM, Jürgen Groß wrote:
> On 13.08.20 08:32, Oleksandr Andrushchenko wrote:
>> Juergen, Boris,
>>
>> can we please merge these via Xen Linux tree as I have collected enough 
>> Ack/R-b?
>>
>> The series has DRM patches, but those anyway are Xen related, so I think
>>
>> this should be fine from DRI point of view.
>
> Yes, fine with me.
Great, thank you
>
>
> Juergen
>
>>
>> Thank you,
>>
>> Oleksandr
>>
>> On 8/13/20 9:21 AM, Oleksandr Andrushchenko wrote:
>>> From: Oleksandr Andrushchenko 
>>>
>>> Hello,
>>>
>>> This series contains an assorted set of fixes and improvements for
>>> the Xen para-virtualized display driver and grant device driver which
>>> I have collected over the last couple of months:
>>>
>>> 1. Minor fixes to grant device driver and drm/xen-front.
>>>
>>> 2. New format (YUYV) added to the list of the PV DRM supported formats
>>> which allows the driver to be used in zero-copying use-cases when
>>> a camera device is the source of the dma-bufs.
>>>
>>> 3. Synchronization with the latest para-virtualized protocol definition
>>> in Xen [1].
>>>
>>> 4. SGT offset is now propagated to the backend: while importing a dmabuf
>>> it is possible that the data of the buffer is put with offset which is
>>> indicated by the SGT offset. This is needed for some GPUs which have
>>> non-zero offset.
>>>
>>> Thank you,
>>> Oleksandr Andrushchenko
>>>
>>> [1] 
>>> https://urldefense.com/v3/__https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=c27a184225eab54d20435c8cab5ad0ef384dc2c0__;!!GF_29dbcQIUBPA!iAHOdk4M167VNM1AypMGVmyKJu-iqC9e5cXyu6N595Np3iyIZDnZl0MIBX3IROJSD1GSMX_GfQ$
>>>  [xenbits[.]xen[.]org]
>>>
>>> Changes since v1:
>>> =
>>>
>>> 1. Removed patch which adds EDID to PV DRM as it needs more time for review:
>>> "5. Version 2 of the Xen displif protocol adds XENDISPL_OP_GET_EDID
>>> request which allows frontends to request EDID structure per
>>> connector. This request is optional and if not supported by the
>>> backend then visible area is still defined by the relevant
>>> XenStore's "resolution" property.
>>> If backend provides EDID with XENDISPL_OP_GET_EDID request then
>>> its values must take precedence over the resolutions defined in
>>> XenStore."
>>> I will send this as a dedicated patch.
>>>
>>> 2. Added missing CC stable for the patches with fixes
>>>
>>> Oleksandr Andrushchenko (5):
>>>     xen/gntdev: Fix dmabuf import with non-zero sgt offset
>>>     drm/xen-front: Fix misused IS_ERR_OR_NULL checks
>>>     drm/xen-front: Add YUYV to supported formats
>>>     xen: Sync up with the canonical protocol definition in Xen
>>>     drm/xen-front: Pass dumb buffer data offset to the backend
>>>
>>>    drivers/gpu/drm/xen/xen_drm_front.c  | 10 +--
>>>    drivers/gpu/drm/xen/xen_drm_front.h  |  2 +-
>>>    drivers/gpu/drm/xen/xen_drm_front_conn.c |  1 +
>>>    drivers/gpu/drm/xen/xen_drm_front_gem.c  | 11 +--
>>>    drivers/gpu/drm/xen/xen_drm_front_kms.c  |  2 +-
>>>    drivers/xen/gntdev-dmabuf.c  |  8 +++
>>>    include/xen/interface/io/displif.h   | 91 +++-
>>>    7 files changed, 111 insertions(+), 14 deletions(-)
>>>
>>
>

[PATCH v2 0/5] Fixes and improvements for Xen pvdrm

From: Oleksandr Andrushchenko 

Hello,

This series contains an assorted set of fixes and improvements for
the Xen para-virtualized display driver and grant device driver which
I have collected over the last couple of months:

1. Minor fixes to grant device driver and drm/xen-front.

2. New format (YUYV) added to the list of the PV DRM supported formats
which allows the driver to be used in zero-copying use-cases when
a camera device is the source of the dma-bufs.

3. Synchronization with the latest para-virtualized protocol definition
in Xen [1].

4. SGT offset is now propagated to the backend: while importing a dmabuf
it is possible that the data of the buffer is put with offset which is
indicated by the SGT offset. This is needed for some GPUs which have
non-zero offset.

Thank you,
Oleksandr Andrushchenko

[1] 
https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=c27a184225eab54d20435c8cab5ad0ef384dc2c0

Changes since v1:
=

1. Removed patch which adds EDID to PV DRM as it needs more time for review:
"5. Version 2 of the Xen displif protocol adds XENDISPL_OP_GET_EDID
request which allows frontends to request EDID structure per
connector. This request is optional and if not supported by the
backend then visible area is still defined by the relevant
XenStore's "resolution" property.
If backend provides EDID with XENDISPL_OP_GET_EDID request then
its values must take precedence over the resolutions defined in
XenStore."
I will send this as a dedicated patch.

2. Added missing CC stable for the patches with fixes

Oleksandr Andrushchenko (5):
  xen/gntdev: Fix dmabuf import with non-zero sgt offset
  drm/xen-front: Fix misused IS_ERR_OR_NULL checks
  drm/xen-front: Add YUYV to supported formats
  xen: Sync up with the canonical protocol definition in Xen
  drm/xen-front: Pass dumb buffer data offset to the backend

 drivers/gpu/drm/xen/xen_drm_front.c  | 10 +--
 drivers/gpu/drm/xen/xen_drm_front.h  |  2 +-
 drivers/gpu/drm/xen/xen_drm_front_conn.c |  1 +
 drivers/gpu/drm/xen/xen_drm_front_gem.c  | 11 +--
 drivers/gpu/drm/xen/xen_drm_front_kms.c  |  2 +-
 drivers/xen/gntdev-dmabuf.c  |  8 +++
 include/xen/interface/io/displif.h   | 91 +++-
 7 files changed, 111 insertions(+), 14 deletions(-)

-- 
2.17.1

[PATCH v2 4/5] xen: Sync up with the canonical protocol definition in Xen

From: Oleksandr Andrushchenko 

This is the sync up with the canonical definition of the
display protocol in Xen.

1. Add protocol version as an integer

Version string, which is in fact an integer, is hard to handle in the
code that supports different protocol versions. To simplify that
also add the version as an integer.

2. Pass buffer offset with XENDISPL_OP_DBUF_CREATE

There are cases when display data buffer is created with non-zero
offset to the data start. Handle such cases and provide that offset
while creating a display buffer.

3. Add XENDISPL_OP_GET_EDID command

Add an optional request for reading Extended Display Identification
Data (EDID) structure which allows better configuration of the
display connectors over the configuration set in XenStore.
With this change connectors may have multiple resolutions defined
with respect to detailed timing definitions and additional properties
normally provided by displays.

If this request is not supported by the backend then visible area
is defined by the relevant XenStore's "resolution" property.

If backend provides extended display identification data (EDID) with
XENDISPL_OP_GET_EDID request then EDID values must take precedence
over the resolutions defined in XenStore.

4. Bump protocol version to 2.

Signed-off-by: Oleksandr Andrushchenko 
Reviewed-by: Juergen Gross 
---
 include/xen/interface/io/displif.h | 91 +-
 1 file changed, 88 insertions(+), 3 deletions(-)

diff --git a/include/xen/interface/io/displif.h 
b/include/xen/interface/io/displif.h
index fdc279dc4a88..d43ca0361f86 100644
--- a/include/xen/interface/io/displif.h
+++ b/include/xen/interface/io/displif.h
@@ -38,7 +38,8 @@
  *   Protocol version
  **
  */
-#define XENDISPL_PROTOCOL_VERSION  "1"
+#define XENDISPL_PROTOCOL_VERSION  "2"
+#define XENDISPL_PROTOCOL_VERSION_INT   2
 
 /*
  **
@@ -202,6 +203,9 @@
  *  Width and height of the connector in pixels separated by
  *  XENDISPL_RESOLUTION_SEPARATOR. This defines visible area of the
  *  display.
+ *  If backend provides extended display identification data (EDID) with
+ *  XENDISPL_OP_GET_EDID request then EDID values must take precedence
+ *  over the resolutions defined here.
  *
  *-- Connector Request Transport Parameters ---
  *
@@ -349,6 +353,8 @@
 #define XENDISPL_OP_FB_DETACH  0x13
 #define XENDISPL_OP_SET_CONFIG 0x14
 #define XENDISPL_OP_PG_FLIP0x15
+/* The below command is available in protocol version 2 and above. */
+#define XENDISPL_OP_GET_EDID   0x16
 
 /*
  **
@@ -377,6 +383,10 @@
 #define XENDISPL_FIELD_BE_ALLOC"be-alloc"
 #define XENDISPL_FIELD_UNIQUE_ID   "unique-id"
 
+#define XENDISPL_EDID_BLOCK_SIZE   128
+#define XENDISPL_EDID_BLOCK_COUNT  256
+#define XENDISPL_EDID_MAX_SIZE (XENDISPL_EDID_BLOCK_SIZE * 
XENDISPL_EDID_BLOCK_COUNT)
+
 /*
  **
  *  STATUS RETURN CODES
@@ -451,7 +461,9 @@
  * +++++
  * |   gref_directory  | 40
  * +++++
- * | reserved  | 44
+ * | data_ofs  | 44
+ * +++++
+ * | reserved  | 48
  * +++++
  * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
  * +++++
@@ -494,6 +506,7 @@
  *   buffer size (buffer_sz) exceeds what can be addressed by this single page,
  *   then reference to the next page must be supplied (see gref_dir_next_page
  *   below)
+ * data_ofs - uint32_t, offset of the data in the buffer, octets
  */
 
 #define XENDISPL_DBUF_FLG_REQ_ALLOC(1 << 0)
@@ -506,6 +519,7 @@ struct xendispl_dbuf_create_req {
uint32_t buffer_sz;
uint32_t flags;
grant_ref_t gref_directory;
+   uint32_t data_ofs;
 };
 
 /*
@@ -731,6 +745,44 @@ struct xendispl_page_flip_req {
uint64_t fb_cookie;
 };
 
+/*
+ * Request EDID - request EDID describing current connector:
+ * 01 2   3octet
+ * +++++
+ * |

[PATCH v2 5/5] drm/xen-front: Pass dumb buffer data offset to the backend

From: Oleksandr Andrushchenko 

While importing a dmabuf it is possible that the data of the buffer
is put with offset which is indicated by the SGT offset.
Respect the offset value and forward it to the backend.

Signed-off-by: Oleksandr Andrushchenko 
Acked-by: Noralf Trønnes 
---
 drivers/gpu/drm/xen/xen_drm_front.c | 6 --
 drivers/gpu/drm/xen/xen_drm_front.h | 2 +-
 drivers/gpu/drm/xen/xen_drm_front_gem.c | 3 ++-
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front.c 
b/drivers/gpu/drm/xen/xen_drm_front.c
index 51818e76facd..3dd56794593a 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.c
+++ b/drivers/gpu/drm/xen/xen_drm_front.c
@@ -157,7 +157,8 @@ int xen_drm_front_mode_set(struct 
xen_drm_front_drm_pipeline *pipeline,
 
 int xen_drm_front_dbuf_create(struct xen_drm_front_info *front_info,
  u64 dbuf_cookie, u32 width, u32 height,
- u32 bpp, u64 size, struct page **pages)
+ u32 bpp, u64 size, u32 offset,
+ struct page **pages)
 {
struct xen_drm_front_evtchnl *evtchnl;
struct xen_drm_front_dbuf *dbuf;
@@ -194,6 +195,7 @@ int xen_drm_front_dbuf_create(struct xen_drm_front_info 
*front_info,
req->op.dbuf_create.gref_directory =
xen_front_pgdir_shbuf_get_dir_start(>shbuf);
req->op.dbuf_create.buffer_sz = size;
+   req->op.dbuf_create.data_ofs = offset;
req->op.dbuf_create.dbuf_cookie = dbuf_cookie;
req->op.dbuf_create.width = width;
req->op.dbuf_create.height = height;
@@ -408,7 +410,7 @@ static int xen_drm_drv_dumb_create(struct drm_file *filp,
ret = xen_drm_front_dbuf_create(drm_info->front_info,
xen_drm_front_dbuf_to_cookie(obj),
args->width, args->height, args->bpp,
-   args->size,
+   args->size, 0,
xen_drm_front_gem_get_pages(obj));
if (ret)
goto fail_backend;
diff --git a/drivers/gpu/drm/xen/xen_drm_front.h 
b/drivers/gpu/drm/xen/xen_drm_front.h
index f92c258350ca..54486d89650e 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.h
+++ b/drivers/gpu/drm/xen/xen_drm_front.h
@@ -145,7 +145,7 @@ int xen_drm_front_mode_set(struct 
xen_drm_front_drm_pipeline *pipeline,
 
 int xen_drm_front_dbuf_create(struct xen_drm_front_info *front_info,
  u64 dbuf_cookie, u32 width, u32 height,
- u32 bpp, u64 size, struct page **pages);
+ u32 bpp, u64 size, u32 offset, struct page 
**pages);
 
 int xen_drm_front_fb_attach(struct xen_drm_front_info *front_info,
u64 dbuf_cookie, u64 fb_cookie, u32 width,
diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
b/drivers/gpu/drm/xen/xen_drm_front_gem.c
index 4ec8a49241e1..39ff95b75357 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -210,7 +210,8 @@ xen_drm_front_gem_import_sg_table(struct drm_device *dev,
 
ret = xen_drm_front_dbuf_create(drm_info->front_info,

xen_drm_front_dbuf_to_cookie(_obj->base),
-   0, 0, 0, size, xen_obj->pages);
+   0, 0, 0, size, sgt->sgl->offset,
+   xen_obj->pages);
if (ret < 0)
return ERR_PTR(ret);
 
-- 
2.17.1

[PATCH v2 2/5] drm/xen-front: Fix misused IS_ERR_OR_NULL checks

From: Oleksandr Andrushchenko 

The patch c575b7eeb89f: "drm/xen-front: Add support for Xen PV
display frontend" from Apr 3, 2018, leads to the following static
checker warning:

drivers/gpu/drm/xen/xen_drm_front_gem.c:140 xen_drm_front_gem_create()
warn: passing zero to 'ERR_CAST'

drivers/gpu/drm/xen/xen_drm_front_gem.c
   133  struct drm_gem_object *xen_drm_front_gem_create(struct drm_device *dev,
   134  size_t size)
   135  {
   136  struct xen_gem_object *xen_obj;
   137
   138  xen_obj = gem_create(dev, size);
   139  if (IS_ERR_OR_NULL(xen_obj))
   140  return ERR_CAST(xen_obj);

Fix this and the rest of misused places with IS_ERR_OR_NULL in the
driver.

Fixes:  c575b7eeb89f: "drm/xen-front: Add support for Xen PV display frontend"

Signed-off-by: Oleksandr Andrushchenko 
Reported-by: Dan Carpenter 
Reviewed-by: Dan Carpenter 
Cc: 
---
 drivers/gpu/drm/xen/xen_drm_front.c | 4 ++--
 drivers/gpu/drm/xen/xen_drm_front_gem.c | 8 
 drivers/gpu/drm/xen/xen_drm_front_kms.c | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front.c 
b/drivers/gpu/drm/xen/xen_drm_front.c
index 1fd458e877ca..51818e76facd 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.c
+++ b/drivers/gpu/drm/xen/xen_drm_front.c
@@ -400,8 +400,8 @@ static int xen_drm_drv_dumb_create(struct drm_file *filp,
args->size = args->pitch * args->height;
 
obj = xen_drm_front_gem_create(dev, args->size);
-   if (IS_ERR_OR_NULL(obj)) {
-   ret = PTR_ERR_OR_ZERO(obj);
+   if (IS_ERR(obj)) {
+   ret = PTR_ERR(obj);
goto fail;
}
 
diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
b/drivers/gpu/drm/xen/xen_drm_front_gem.c
index f0b85e094111..4ec8a49241e1 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -83,7 +83,7 @@ static struct xen_gem_object *gem_create(struct drm_device 
*dev, size_t size)
 
size = round_up(size, PAGE_SIZE);
xen_obj = gem_create_obj(dev, size);
-   if (IS_ERR_OR_NULL(xen_obj))
+   if (IS_ERR(xen_obj))
return xen_obj;
 
if (drm_info->front_info->cfg.be_alloc) {
@@ -117,7 +117,7 @@ static struct xen_gem_object *gem_create(struct drm_device 
*dev, size_t size)
 */
xen_obj->num_pages = DIV_ROUND_UP(size, PAGE_SIZE);
xen_obj->pages = drm_gem_get_pages(_obj->base);
-   if (IS_ERR_OR_NULL(xen_obj->pages)) {
+   if (IS_ERR(xen_obj->pages)) {
ret = PTR_ERR(xen_obj->pages);
xen_obj->pages = NULL;
goto fail;
@@ -136,7 +136,7 @@ struct drm_gem_object *xen_drm_front_gem_create(struct 
drm_device *dev,
struct xen_gem_object *xen_obj;
 
xen_obj = gem_create(dev, size);
-   if (IS_ERR_OR_NULL(xen_obj))
+   if (IS_ERR(xen_obj))
return ERR_CAST(xen_obj);
 
return _obj->base;
@@ -194,7 +194,7 @@ xen_drm_front_gem_import_sg_table(struct drm_device *dev,
 
size = attach->dmabuf->size;
xen_obj = gem_create_obj(dev, size);
-   if (IS_ERR_OR_NULL(xen_obj))
+   if (IS_ERR(xen_obj))
return ERR_CAST(xen_obj);
 
ret = gem_alloc_pages_array(xen_obj, size);
diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c 
b/drivers/gpu/drm/xen/xen_drm_front_kms.c
index 78096bbcd226..ef11b1e4de39 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_kms.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c
@@ -60,7 +60,7 @@ fb_create(struct drm_device *dev, struct drm_file *filp,
int ret;
 
fb = drm_gem_fb_create_with_funcs(dev, filp, mode_cmd, _funcs);
-   if (IS_ERR_OR_NULL(fb))
+   if (IS_ERR(fb))
return fb;
 
gem_obj = fb->obj[0];
-- 
2.17.1

[PATCH v2 1/5] xen/gntdev: Fix dmabuf import with non-zero sgt offset

From: Oleksandr Andrushchenko 

It is possible that the scatter-gather table during dmabuf import has
non-zero offset of the data, but user-space doesn't expect that.
Fix this by failing the import, so user-space doesn't access wrong data.

Fixes: bf8dc55b1358 ("xen/gntdev: Implement dma-buf import functionality")

Signed-off-by: Oleksandr Andrushchenko 
Acked-by: Juergen Gross 
Cc: 
---
 drivers/xen/gntdev-dmabuf.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
index 75d3bb948bf3..b1b6eebafd5d 100644
--- a/drivers/xen/gntdev-dmabuf.c
+++ b/drivers/xen/gntdev-dmabuf.c
@@ -613,6 +613,14 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct 
device *dev,
goto fail_detach;
}
 
+   /* Check that we have zero offset. */
+   if (sgt->sgl->offset) {
+   ret = ERR_PTR(-EINVAL);
+   pr_debug("DMA buffer has %d bytes offset, user-space expects 
0\n",
+sgt->sgl->offset);
+   goto fail_unmap;
+   }
+
/* Check number of pages that imported buffer has. */
if (attach->dmabuf->size != gntdev_dmabuf->nr_pages << PAGE_SHIFT) {
ret = ERR_PTR(-EINVAL);
-- 
2.17.1

[PATCH v2 3/5] drm/xen-front: Add YUYV to supported formats

From: Oleksandr Andrushchenko 

Add YUYV to supported formats, so the frontend can work with the
formats used by cameras and other HW.

Signed-off-by: Oleksandr Andrushchenko 
Acked-by: Noralf Trønnes 
---
 drivers/gpu/drm/xen/xen_drm_front_conn.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/xen/xen_drm_front_conn.c 
b/drivers/gpu/drm/xen/xen_drm_front_conn.c
index 459702fa990e..44f1f70c0aed 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_conn.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_conn.c
@@ -33,6 +33,7 @@ static const u32 plane_formats[] = {
DRM_FORMAT_ARGB,
DRM_FORMAT_XRGB1555,
DRM_FORMAT_ARGB1555,
+   DRM_FORMAT_YUYV,
 };
 
 const u32 *xen_drm_front_conn_get_formats(int *format_count)
-- 
2.17.1

Re: [PATCH 1/6] xen/gntdev: Fix dmabuf import with non-zero sgt offset

2020-08-04 Thread Oleksandr Andrushchenko


On 8/4/20 9:11 AM, Jürgen Groß wrote:
> On 31.07.20 14:51, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko 
>>
>> It is possible that the scatter-gather table during dmabuf import has
>> non-zero offset of the data, but user-space doesn't expect that.
>> Fix this by failing the import, so user-space doesn't access wrong data.
>>
>> Fixes: 37ccb44d0b00 ("xen/gntdev: Implement dma-buf import functionality")
>
> I can't find this commit in the tree. Did you mean bf8dc55b1358?
I'll double-check, thank you
>
> And don't you want to Cc stable for this patch, too?
Hm, yes, sounds reasonable
>
>>
>> Signed-off-by: Oleksandr Andrushchenko 
>
> Acked-by: Juergen Gross 
>
>
> Juergen

Re: [PATCH 2/6] drm/xen-front: Fix misused IS_ERR_OR_NULL checks

2020-08-04 Thread Oleksandr Andrushchenko


On 8/4/20 9:12 AM, Jürgen Groß wrote:
> On 31.07.20 14:51, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko 
>>
>> The patch c575b7eeb89f: "drm/xen-front: Add support for Xen PV
>> display frontend" from Apr 3, 2018, leads to the following static
>> checker warning:
>>
>> drivers/gpu/drm/xen/xen_drm_front_gem.c:140 xen_drm_front_gem_create()
>> warn: passing zero to 'ERR_CAST'
>>
>> drivers/gpu/drm/xen/xen_drm_front_gem.c
>>     133  struct drm_gem_object *xen_drm_front_gem_create(struct drm_device 
>> *dev,
>>     134  size_t size)
>>     135  {
>>     136  struct xen_gem_object *xen_obj;
>>     137
>>     138  xen_obj = gem_create(dev, size);
>>     139  if (IS_ERR_OR_NULL(xen_obj))
>>     140  return ERR_CAST(xen_obj);
>>
>> Fix this and the rest of misused places with IS_ERR_OR_NULL in the
>> driver.
>>
>> Fixes:  c575b7eeb89f: "drm/xen-front: Add support for Xen PV display 
>> frontend"
>
> Again forgot to Cc stable?

I was just not sure if these minor fixes need to go the stable, but I will add

Thank you

>
>
> Juergen

[PATCH 4/6] xen: Sync up with the canonical protocol definition in Xen

From: Oleksandr Andrushchenko 

This is the sync up with the canonical definition of the
display protocol in Xen.

1. Add protocol version as an integer

Version string, which is in fact an integer, is hard to handle in the
code that supports different protocol versions. To simplify that
also add the version as an integer.

2. Pass buffer offset with XENDISPL_OP_DBUF_CREATE

There are cases when display data buffer is created with non-zero
offset to the data start. Handle such cases and provide that offset
while creating a display buffer.

3. Add XENDISPL_OP_GET_EDID command

Add an optional request for reading Extended Display Identification
Data (EDID) structure which allows better configuration of the
display connectors over the configuration set in XenStore.
With this change connectors may have multiple resolutions defined
with respect to detailed timing definitions and additional properties
normally provided by displays.

If this request is not supported by the backend then visible area
is defined by the relevant XenStore's "resolution" property.

If backend provides extended display identification data (EDID) with
XENDISPL_OP_GET_EDID request then EDID values must take precedence
over the resolutions defined in XenStore.

4. Bump protocol version to 2.

Signed-off-by: Oleksandr Andrushchenko 
---
 include/xen/interface/io/displif.h | 91 +-
 1 file changed, 88 insertions(+), 3 deletions(-)

diff --git a/include/xen/interface/io/displif.h 
b/include/xen/interface/io/displif.h
index fdc279dc4a88..c2d900186883 100644
--- a/include/xen/interface/io/displif.h
+++ b/include/xen/interface/io/displif.h
@@ -38,7 +38,8 @@
  *   Protocol version
  **
  */
-#define XENDISPL_PROTOCOL_VERSION  "1"
+#define XENDISPL_PROTOCOL_VERSION  "2"
+#define XENDISPL_PROTOCOL_VERSION_INT   2
 
 /*
  **
@@ -202,6 +203,9 @@
  *  Width and height of the connector in pixels separated by
  *  XENDISPL_RESOLUTION_SEPARATOR. This defines visible area of the
  *  display.
+ *  If backend provides extended display identification data (EDID) with
+ *  XENDISPL_OP_GET_EDID request then EDID values must take precedence
+ *  over the resolutions defined here.
  *
  *-- Connector Request Transport Parameters ---
  *
@@ -349,6 +353,8 @@
 #define XENDISPL_OP_FB_DETACH  0x13
 #define XENDISPL_OP_SET_CONFIG 0x14
 #define XENDISPL_OP_PG_FLIP0x15
+/* The below command is available in protocol version 2 and above. */
+#define XENDISPL_OP_GET_EDID   0x16
 
 /*
  **
@@ -377,6 +383,10 @@
 #define XENDISPL_FIELD_BE_ALLOC"be-alloc"
 #define XENDISPL_FIELD_UNIQUE_ID   "unique-id"
 
+#define XENDISPL_EDID_BLOCK_SIZE   128
+#define XENDISPL_EDID_BLOCK_COUNT  256
+#define XENDISPL_EDID_MAX_SIZE (XENDISPL_EDID_BLOCK_SIZE * 
XENDISPL_EDID_BLOCK_COUNT)
+
 /*
  **
  *  STATUS RETURN CODES
@@ -451,7 +461,9 @@
  * +++++
  * |   gref_directory  | 40
  * +++++
- * | reserved  | 44
+ * | data_ofs  | 44
+ * +++++
+ * | reserved  | 48
  * +++++
  * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
  * +++++
@@ -494,6 +506,7 @@
  *   buffer size (buffer_sz) exceeds what can be addressed by this single page,
  *   then reference to the next page must be supplied (see gref_dir_next_page
  *   below)
+ * data_ofs - uint32_t, offset of the data in the buffer, octets
  */
 
 #define XENDISPL_DBUF_FLG_REQ_ALLOC(1 << 0)
@@ -506,6 +519,7 @@ struct xendispl_dbuf_create_req {
uint32_t buffer_sz;
uint32_t flags;
grant_ref_t gref_directory;
+   uint32_t data_ofs;
 };
 
 /*
@@ -731,6 +745,44 @@ struct xendispl_page_flip_req {
uint64_t fb_cookie;
 };
 
+/*
+ * Request EDID - request EDID describing current connector:
+ * 01 2   3octet
+ * +++++
+ * |

[PATCH 1/6] xen/gntdev: Fix dmabuf import with non-zero sgt offset

From: Oleksandr Andrushchenko 

It is possible that the scatter-gather table during dmabuf import has
non-zero offset of the data, but user-space doesn't expect that.
Fix this by failing the import, so user-space doesn't access wrong data.

Fixes: 37ccb44d0b00 ("xen/gntdev: Implement dma-buf import functionality")

Signed-off-by: Oleksandr Andrushchenko 
---
 drivers/xen/gntdev-dmabuf.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
index 75d3bb948bf3..b1b6eebafd5d 100644
--- a/drivers/xen/gntdev-dmabuf.c
+++ b/drivers/xen/gntdev-dmabuf.c
@@ -613,6 +613,14 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct 
device *dev,
goto fail_detach;
}
 
+   /* Check that we have zero offset. */
+   if (sgt->sgl->offset) {
+   ret = ERR_PTR(-EINVAL);
+   pr_debug("DMA buffer has %d bytes offset, user-space expects 
0\n",
+sgt->sgl->offset);
+   goto fail_unmap;
+   }
+
/* Check number of pages that imported buffer has. */
if (attach->dmabuf->size != gntdev_dmabuf->nr_pages << PAGE_SHIFT) {
ret = ERR_PTR(-EINVAL);
-- 
2.17.1

[PATCH 5/6] drm/xen-front: Pass dumb buffer data offset to the backend

From: Oleksandr Andrushchenko 

While importing a dmabuf it is possible that the data of the buffer
is put with offset which is indicated by the SGT offset.
Respect the offset value and forward it to the backend.

Signed-off-by: Oleksandr Andrushchenko 
---
 drivers/gpu/drm/xen/xen_drm_front.c | 6 --
 drivers/gpu/drm/xen/xen_drm_front.h | 2 +-
 drivers/gpu/drm/xen/xen_drm_front_gem.c | 3 ++-
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front.c 
b/drivers/gpu/drm/xen/xen_drm_front.c
index 88db2726e8ce..013c9e0e412c 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.c
+++ b/drivers/gpu/drm/xen/xen_drm_front.c
@@ -157,7 +157,8 @@ int xen_drm_front_mode_set(struct 
xen_drm_front_drm_pipeline *pipeline,
 
 int xen_drm_front_dbuf_create(struct xen_drm_front_info *front_info,
  u64 dbuf_cookie, u32 width, u32 height,
- u32 bpp, u64 size, struct page **pages)
+ u32 bpp, u64 size, u32 offset,
+ struct page **pages)
 {
struct xen_drm_front_evtchnl *evtchnl;
struct xen_drm_front_dbuf *dbuf;
@@ -194,6 +195,7 @@ int xen_drm_front_dbuf_create(struct xen_drm_front_info 
*front_info,
req->op.dbuf_create.gref_directory =
xen_front_pgdir_shbuf_get_dir_start(>shbuf);
req->op.dbuf_create.buffer_sz = size;
+   req->op.dbuf_create.data_ofs = offset;
req->op.dbuf_create.dbuf_cookie = dbuf_cookie;
req->op.dbuf_create.width = width;
req->op.dbuf_create.height = height;
@@ -408,7 +410,7 @@ static int xen_drm_drv_dumb_create(struct drm_file *filp,
ret = xen_drm_front_dbuf_create(drm_info->front_info,
xen_drm_front_dbuf_to_cookie(obj),
args->width, args->height, args->bpp,
-   args->size,
+   args->size, 0,
xen_drm_front_gem_get_pages(obj));
if (ret)
goto fail_backend;
diff --git a/drivers/gpu/drm/xen/xen_drm_front.h 
b/drivers/gpu/drm/xen/xen_drm_front.h
index f92c258350ca..54486d89650e 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.h
+++ b/drivers/gpu/drm/xen/xen_drm_front.h
@@ -145,7 +145,7 @@ int xen_drm_front_mode_set(struct 
xen_drm_front_drm_pipeline *pipeline,
 
 int xen_drm_front_dbuf_create(struct xen_drm_front_info *front_info,
  u64 dbuf_cookie, u32 width, u32 height,
- u32 bpp, u64 size, struct page **pages);
+ u32 bpp, u64 size, u32 offset, struct page 
**pages);
 
 int xen_drm_front_fb_attach(struct xen_drm_front_info *front_info,
u64 dbuf_cookie, u64 fb_cookie, u32 width,
diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
b/drivers/gpu/drm/xen/xen_drm_front_gem.c
index 4ec8a49241e1..39ff95b75357 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -210,7 +210,8 @@ xen_drm_front_gem_import_sg_table(struct drm_device *dev,
 
ret = xen_drm_front_dbuf_create(drm_info->front_info,

xen_drm_front_dbuf_to_cookie(_obj->base),
-   0, 0, 0, size, xen_obj->pages);
+   0, 0, 0, size, sgt->sgl->offset,
+   xen_obj->pages);
if (ret < 0)
return ERR_PTR(ret);
 
-- 
2.17.1

[PATCH 0/6] Fixes and improvements for Xen pvdrm

From: Oleksandr Andrushchenko 

Hello,

This series contains an assorted set of fixes and improvements for
the Xen para-virtualized display driver and grant device driver which
I have collected over the last couple of months:

1. Minor fixes to grant device driver and drm/xen-front.

2. New format (YUYV) added to the list of the PV DRM supported formats
which allows the driver to be used in zero-copying use-cases when
a camera device is the source of the dma-bufs.

3. Synchronization with the latest para-virtualized protocol definition
in Xen [1].

4. SGT offset is now propagated to the backend: while importing a dmabuf
it is possible that the data of the buffer is put with offset which is
indicated by the SGT offset. This is needed for some GPUs which have
non-zero offset.

5. Version 2 of the Xen displif protocol adds XENDISPL_OP_GET_EDID
request which allows frontends to request EDID structure per
connector. This request is optional and if not supported by the
backend then visible area is still defined by the relevant
XenStore's "resolution" property.
If backend provides EDID with XENDISPL_OP_GET_EDID request then
its values must take precedence over the resolutions defined in
XenStore.

Thank you,
Oleksandr Andrushchenko

[1] 
https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=c27a184225eab54d20435c8cab5ad0ef384dc2c0

Oleksandr Andrushchenko (6):
  xen/gntdev: Fix dmabuf import with non-zero sgt offset
  drm/xen-front: Fix misused IS_ERR_OR_NULL checks
  drm/xen-front: Add YUYV to supported formats
  xen: Sync up with the canonical protocol definition in Xen
  drm/xen-front: Pass dumb buffer data offset to the backend
  drm/xen-front: Add support for EDID based configuration

 drivers/gpu/drm/xen/xen_drm_front.c | 72 +++-
 drivers/gpu/drm/xen/xen_drm_front.h | 11 ++-
 drivers/gpu/drm/xen/xen_drm_front_cfg.c | 82 +++
 drivers/gpu/drm/xen/xen_drm_front_cfg.h |  7 ++
 drivers/gpu/drm/xen/xen_drm_front_conn.c| 27 +-
 drivers/gpu/drm/xen/xen_drm_front_evtchnl.c |  3 +
 drivers/gpu/drm/xen/xen_drm_front_evtchnl.h |  3 +
 drivers/gpu/drm/xen/xen_drm_front_gem.c | 11 +--
 drivers/gpu/drm/xen/xen_drm_front_kms.c |  7 +-
 drivers/xen/gntdev-dmabuf.c |  8 ++
 include/xen/interface/io/displif.h  | 91 -
 11 files changed, 305 insertions(+), 17 deletions(-)

-- 
2.17.1

[PATCH 3/6] drm/xen-front: Add YUYV to supported formats

From: Oleksandr Andrushchenko 

Add YUYV to supported formats, so the frontend can work with the
formats used by cameras and other HW.

Signed-off-by: Oleksandr Andrushchenko 
---
 drivers/gpu/drm/xen/xen_drm_front_conn.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/xen/xen_drm_front_conn.c 
b/drivers/gpu/drm/xen/xen_drm_front_conn.c
index 459702fa990e..44f1f70c0aed 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_conn.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_conn.c
@@ -33,6 +33,7 @@ static const u32 plane_formats[] = {
DRM_FORMAT_ARGB,
DRM_FORMAT_XRGB1555,
DRM_FORMAT_ARGB1555,
+   DRM_FORMAT_YUYV,
 };
 
 const u32 *xen_drm_front_conn_get_formats(int *format_count)
-- 
2.17.1

[PATCH 6/6] drm/xen-front: Add support for EDID based configuration

From: Oleksandr Andrushchenko 

Version 2 of the Xen displif protocol adds XENDISPL_OP_GET_EDID
request which allows frontends to request EDID structure per
connector. This request is optional and if not supported by the
backend then visible area is still defined by the relevant
XenStore's "resolution" property.
If backend provides EDID with XENDISPL_OP_GET_EDID request then
its values must take precedence over the resolutions defined in
XenStore.

Signed-off-by: Oleksandr Andrushchenko 
---
 drivers/gpu/drm/xen/xen_drm_front.c | 62 
 drivers/gpu/drm/xen/xen_drm_front.h |  9 ++-
 drivers/gpu/drm/xen/xen_drm_front_cfg.c | 82 +
 drivers/gpu/drm/xen/xen_drm_front_cfg.h |  7 ++
 drivers/gpu/drm/xen/xen_drm_front_conn.c| 26 ++-
 drivers/gpu/drm/xen/xen_drm_front_evtchnl.c |  3 +
 drivers/gpu/drm/xen/xen_drm_front_evtchnl.h |  3 +
 drivers/gpu/drm/xen/xen_drm_front_kms.c |  5 ++
 8 files changed, 194 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front.c 
b/drivers/gpu/drm/xen/xen_drm_front.c
index 013c9e0e412c..cc5981bdbfb3 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.c
+++ b/drivers/gpu/drm/xen/xen_drm_front.c
@@ -381,6 +381,59 @@ void xen_drm_front_on_frame_done(struct xen_drm_front_info 
*front_info,
fb_cookie);
 }
 
+int xen_drm_front_get_edid(struct xen_drm_front_info *front_info,
+  int conn_idx, struct page **pages,
+  u32 buffer_sz, u32 *edid_sz)
+{
+   struct xen_drm_front_evtchnl *evtchnl;
+   struct xen_front_pgdir_shbuf_cfg buf_cfg;
+   struct xen_front_pgdir_shbuf shbuf;
+   struct xendispl_req *req;
+   unsigned long flags;
+   int ret;
+
+   if (unlikely(conn_idx >= front_info->num_evt_pairs))
+   return -EINVAL;
+
+   memset(_cfg, 0, sizeof(buf_cfg));
+   buf_cfg.xb_dev = front_info->xb_dev;
+   buf_cfg.num_pages = DIV_ROUND_UP(buffer_sz, PAGE_SIZE);
+   buf_cfg.pages = pages;
+   buf_cfg.pgdir = 
+   buf_cfg.be_alloc = false;
+
+   ret = xen_front_pgdir_shbuf_alloc(_cfg);
+   if (ret < 0)
+   return ret;
+
+   evtchnl = _info->evt_pairs[conn_idx].req;
+
+   mutex_lock(>u.req.req_io_lock);
+
+   spin_lock_irqsave(_info->io_lock, flags);
+   req = be_prepare_req(evtchnl, XENDISPL_OP_GET_EDID);
+   req->op.get_edid.gref_directory =
+   xen_front_pgdir_shbuf_get_dir_start();
+   req->op.get_edid.buffer_sz = buffer_sz;
+
+   ret = be_stream_do_io(evtchnl, req);
+   spin_unlock_irqrestore(_info->io_lock, flags);
+
+   if (ret < 0)
+   goto fail;
+
+   ret = be_stream_wait_io(evtchnl);
+   if (ret < 0)
+   goto fail;
+
+   *edid_sz = evtchnl->u.req.resp.get_edid.edid_sz;
+
+fail:
+   mutex_unlock(>u.req.req_io_lock);
+   xen_front_pgdir_shbuf_free();
+   return ret;
+}
+
 static int xen_drm_drv_dumb_create(struct drm_file *filp,
   struct drm_device *dev,
   struct drm_mode_create_dumb *args)
@@ -466,6 +519,7 @@ static void xen_drm_drv_release(struct drm_device *dev)
xenbus_switch_state(front_info->xb_dev,
XenbusStateInitialising);
 
+   xen_drm_front_cfg_free(front_info, _info->cfg);
kfree(drm_info);
 }
 
@@ -562,6 +616,7 @@ static int xen_drm_drv_init(struct xen_drm_front_info 
*front_info)
drm_mode_config_cleanup(drm_dev);
drm_dev_put(drm_dev);
 fail:
+   xen_drm_front_cfg_free(front_info, _info->cfg);
kfree(drm_info);
return ret;
 }
@@ -622,7 +677,14 @@ static int displback_initwait(struct xen_drm_front_info 
*front_info)
 
 static int displback_connect(struct xen_drm_front_info *front_info)
 {
+   int ret;
+
xen_drm_front_evtchnl_set_state(front_info, EVTCHNL_STATE_CONNECTED);
+
+   /* We are all set to read additional configuration from the backend. */
+   ret = xen_drm_front_cfg_tail(front_info, _info->cfg);
+   if (ret < 0)
+   return ret;
return xen_drm_drv_init(front_info);
 }
 
diff --git a/drivers/gpu/drm/xen/xen_drm_front.h 
b/drivers/gpu/drm/xen/xen_drm_front.h
index 54486d89650e..be0c982f4d82 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.h
+++ b/drivers/gpu/drm/xen/xen_drm_front.h
@@ -112,9 +112,12 @@ struct xen_drm_front_drm_pipeline {
struct drm_simple_display_pipe pipe;
 
struct drm_connector conn;
-   /* These are only for connector mode checking */
+   /* These are only for connector mode checking if no EDID present */
int width, height;
 
+   /* Is not NULL if EDID is used for connector configuration. */
+   struct edid *edid;
+
struct drm_pending_vblank_event *pending_event;

[PATCH 2/6] drm/xen-front: Fix misused IS_ERR_OR_NULL checks

From: Oleksandr Andrushchenko 

The patch c575b7eeb89f: "drm/xen-front: Add support for Xen PV
display frontend" from Apr 3, 2018, leads to the following static
checker warning:

drivers/gpu/drm/xen/xen_drm_front_gem.c:140 xen_drm_front_gem_create()
warn: passing zero to 'ERR_CAST'

drivers/gpu/drm/xen/xen_drm_front_gem.c
   133  struct drm_gem_object *xen_drm_front_gem_create(struct drm_device *dev,
   134  size_t size)
   135  {
   136  struct xen_gem_object *xen_obj;
   137
   138  xen_obj = gem_create(dev, size);
   139  if (IS_ERR_OR_NULL(xen_obj))
   140  return ERR_CAST(xen_obj);

Fix this and the rest of misused places with IS_ERR_OR_NULL in the
driver.

Fixes:  c575b7eeb89f: "drm/xen-front: Add support for Xen PV display frontend"

Signed-off-by: Oleksandr Andrushchenko 
Reported-by: Dan Carpenter 
---
 drivers/gpu/drm/xen/xen_drm_front.c | 4 ++--
 drivers/gpu/drm/xen/xen_drm_front_gem.c | 8 
 drivers/gpu/drm/xen/xen_drm_front_kms.c | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front.c 
b/drivers/gpu/drm/xen/xen_drm_front.c
index 3e660fb111b3..88db2726e8ce 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.c
+++ b/drivers/gpu/drm/xen/xen_drm_front.c
@@ -400,8 +400,8 @@ static int xen_drm_drv_dumb_create(struct drm_file *filp,
args->size = args->pitch * args->height;
 
obj = xen_drm_front_gem_create(dev, args->size);
-   if (IS_ERR_OR_NULL(obj)) {
-   ret = PTR_ERR_OR_ZERO(obj);
+   if (IS_ERR(obj)) {
+   ret = PTR_ERR(obj);
goto fail;
}
 
diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
b/drivers/gpu/drm/xen/xen_drm_front_gem.c
index f0b85e094111..4ec8a49241e1 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -83,7 +83,7 @@ static struct xen_gem_object *gem_create(struct drm_device 
*dev, size_t size)
 
size = round_up(size, PAGE_SIZE);
xen_obj = gem_create_obj(dev, size);
-   if (IS_ERR_OR_NULL(xen_obj))
+   if (IS_ERR(xen_obj))
return xen_obj;
 
if (drm_info->front_info->cfg.be_alloc) {
@@ -117,7 +117,7 @@ static struct xen_gem_object *gem_create(struct drm_device 
*dev, size_t size)
 */
xen_obj->num_pages = DIV_ROUND_UP(size, PAGE_SIZE);
xen_obj->pages = drm_gem_get_pages(_obj->base);
-   if (IS_ERR_OR_NULL(xen_obj->pages)) {
+   if (IS_ERR(xen_obj->pages)) {
ret = PTR_ERR(xen_obj->pages);
xen_obj->pages = NULL;
goto fail;
@@ -136,7 +136,7 @@ struct drm_gem_object *xen_drm_front_gem_create(struct 
drm_device *dev,
struct xen_gem_object *xen_obj;
 
xen_obj = gem_create(dev, size);
-   if (IS_ERR_OR_NULL(xen_obj))
+   if (IS_ERR(xen_obj))
return ERR_CAST(xen_obj);
 
return _obj->base;
@@ -194,7 +194,7 @@ xen_drm_front_gem_import_sg_table(struct drm_device *dev,
 
size = attach->dmabuf->size;
xen_obj = gem_create_obj(dev, size);
-   if (IS_ERR_OR_NULL(xen_obj))
+   if (IS_ERR(xen_obj))
return ERR_CAST(xen_obj);
 
ret = gem_alloc_pages_array(xen_obj, size);
diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c 
b/drivers/gpu/drm/xen/xen_drm_front_kms.c
index 78096bbcd226..ef11b1e4de39 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_kms.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c
@@ -60,7 +60,7 @@ fb_create(struct drm_device *dev, struct drm_file *filp,
int ret;
 
fb = drm_gem_fb_create_with_funcs(dev, filp, mode_cmd, _funcs);
-   if (IS_ERR_OR_NULL(fb))
+   if (IS_ERR(fb))
return fb;
 
gem_obj = fb->obj[0];
-- 
2.17.1

Re: [PATCH v2] xen: Stop abusing DT of_dma_configure API

2019-10-09 Thread Oleksandr Andrushchenko

On 10/8/19 10:41 PM, Rob Herring wrote:
> As the removed comments say, these aren't DT based devices.
> of_dma_configure() is going to stop allowing a NULL DT node and calling
> it will no longer work.
>
> The comment is also now out of date as of commit 9ab91e7c5c51 ("arm64:
> default to the direct mapping in get_arch_dma_ops"). Direct mapping
> is now the default rather than dma_dummy_ops.
>
> According to Stefano and Oleksandr, the only other part needed is
> setting the DMA masks and there's no reason to restrict the masks to
> 32-bits. So set the masks to 64 bits.
>
> Cc: Robin Murphy 
> Cc: Julien Grall 
> Cc: Nicolas Saenz Julienne 
> Cc: Oleksandr Andrushchenko 
> Cc: Boris Ostrovsky 
> Cc: Juergen Gross 
> Cc: Stefano Stabellini 
> Cc: Christoph Hellwig 
> Cc: xen-de...@lists.xenproject.org
> Signed-off-by: Rob Herring 
Acked-by: Oleksandr Andrushchenko 

Unfortunately I cannot test this patch with real HW running Xen:
I am still on 4.14 kernel which is dictated by the board's BSP and
it is not possible to have more recent one at the moment.
So, I hope the patch will work as intended.

Thank you,
Oleksandr
> ---
> v2:
>   - Setup dma masks
>   - Also fix xen_drm_front.c
>   
> This can now be applied to the Xen tree independent of the coming
> of_dma_configure() changes.
>
> Rob
>
>   drivers/gpu/drm/xen/xen_drm_front.c | 12 ++--
>   drivers/xen/gntdev.c| 13 ++---
>   2 files changed, 4 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/xen/xen_drm_front.c 
> b/drivers/gpu/drm/xen/xen_drm_front.c
> index ba1828acd8c9..4be49c1aef51 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front.c
> +++ b/drivers/gpu/drm/xen/xen_drm_front.c
> @@ -718,17 +718,9 @@ static int xen_drv_probe(struct xenbus_device *xb_dev,
>   struct device *dev = _dev->dev;
>   int ret;
>   
> - /*
> -  * The device is not spawn from a device tree, so arch_setup_dma_ops
> -  * is not called, thus leaving the device with dummy DMA ops.
> -  * This makes the device return error on PRIME buffer import, which
> -  * is not correct: to fix this call of_dma_configure() with a NULL
> -  * node to set default DMA ops.
> -  */
> - dev->coherent_dma_mask = DMA_BIT_MASK(32);
> - ret = of_dma_configure(dev, NULL, true);
> + ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64));
>   if (ret < 0) {
> - DRM_ERROR("Cannot setup DMA ops, ret %d", ret);
> + DRM_ERROR("Cannot setup DMA mask, ret %d", ret);
>   return ret;
>   }
>   
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index a446a7221e13..81401f386c9c 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -22,6 +22,7 @@
>   
>   #define pr_fmt(fmt) "xen:" KBUILD_MODNAME ": " fmt
>   
> +#include 
>   #include 
>   #include 
>   #include 
> @@ -34,9 +35,6 @@
>   #include 
>   #include 
>   #include 
> -#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
> -#include 
> -#endif
>   
>   #include 
>   #include 
> @@ -625,14 +623,7 @@ static int gntdev_open(struct inode *inode, struct file 
> *flip)
>   flip->private_data = priv;
>   #ifdef CONFIG_XEN_GRANT_DMA_ALLOC
>   priv->dma_dev = gntdev_miscdev.this_device;
> -
> - /*
> -  * The device is not spawn from a device tree, so arch_setup_dma_ops
> -  * is not called, thus leaving the device with dummy DMA ops.
> -  * Fix this by calling of_dma_configure() with a NULL node to set
> -  * default DMA ops.
> -  */
> - of_dma_configure(priv->dma_dev, NULL, true);
> + dma_coerce_mask_and_coherent(priv->dma_dev, DMA_BIT_MASK(64));
>   #endif
>   pr_debug("priv %p\n", priv);
>

Re: [RFC PATCH] xen/gntdev: Stop abusing DT of_dma_configure API

2019-10-01 Thread Oleksandr Andrushchenko

On 10/1/19 9:23 PM, Stefano Stabellini wrote:
> On Thu, 26 Sep 2019, Rob Herring wrote:
>> On Thu, Sep 26, 2019 at 6:16 AM Oleksandr Andrushchenko
>>  wrote:
>>> On 9/26/19 1:46 PM, Robin Murphy wrote:
>>>> On 2019-09-26 11:17 am, Oleksandr Andrushchenko wrote:
>>>>> On 9/26/19 12:49 PM, Julien Grall wrote:
>>>>>> Hi Rob,
>>>>>>
>>>>>>
>>>>>> On 9/25/19 10:50 PM, Rob Herring wrote:
>>>>>>> As the comment says, this isn't a DT based device. of_dma_configure()
>>>>>>> is going to stop allowing a NULL DT node, so this needs to be fixed.
>>>>>> And this can't work on arch not selecting CONFIG_OF and can select
>>>>>> CONFIG_XEN_GRANT_DMA_ALLOC.
>>>>>>
>>>>>> We are lucky enough on x86 because, AFAICT, arch_setup_dma_ops is just
>>>>>> a nop.
>>>>>>
>>>>> No luck is needed as [1] does nothing for those platforms not using
>>>>> CONFIG_OF
>>>>>>> Not sure exactly what setup besides arch_setup_dma_ops is needed...
>>>>>> We probably want to update dma_mask, coherent_dma_mask and
>>>>>> dma_pfn_offset.
>>>>>>
>>>>>> Also, while look at of_configure_dma, I noticed that we consider the
>>>>>> DMA will not be coherent for the grant-table. Oleksandr, do you know
>>>>>> why they can't be coherent?
>>>>> The main and the only reason to use of_configure_dma is that if we don't
>>>>> then we
>>>>> are about to stay with dma_dummy_ops [2]. It effectively means that
>>>>> operations on dma-bufs
>>>>> will end up returning errors, like [3], [4], thus not making it possible
>>>>> for Xen PV DRM and DMA
>>>>> part of gntdev driver to do what we need (dma-bufs in our use-cases
>>>>> allow zero-copying
>>>>> while using graphics buffers and many more).
>>>>>
>>>>> I didn't find any better way of achieving that, but of_configure_dma...
>>>>> If there is any better solution which will not break the existing
>>>>> functionality then
>>>>> I will definitely change the drivers so we do not abuse DT )
>>>>> Before that, please keep in mind that merging this RFC will break Xen PV
>>>>> DRM +
>>>>> DMA buf support in gntdev...
>>>>> Hope we can work out some acceptable solution, so everyone is happy
>>>> As I mentioned elsewhere, the recent dma-direct rework means that
>>>> dma_dummy_ops are now only explicitly installed for the ACPI error
>>>> case, so - much as I may dislike it - you should get regular
>>>> (direct/SWIOTLB) ops by default again.
>>> Ah, my bad, I missed that change. So, if no dummy dma ops are to be used
>>> then
>>> I believe we can apply both changes, e.g. remove of_dma_configure from
>>> both of the drivers.
>> What about the dma masks? I think there's a default setup, but it is
>> considered a driver bug to not set its mask. xen_drm_front sets the
>> coherent_dma_mask (why only 32-bits though?), but not the dma_mask.
>> gntdev is doing neither. I could copy out what of_dma_configure does
>> but better for the Xen folks to decide what is needed or not and test
>> the change. I'm not setup to test any of this.
> FYI I have seen the issue Oleksandr is talking about too. I confirm that
> the only reason for the of_configure_dma call is to get away from the
> dummy_dma_ops and use the default dma_ops instead. I think this should
> be mentioned in the commit message so that if one day the behavior
> regarding dummy_dma_ops changes one more time, hopefully we'll be able
> to figure out the issue more easily with bisection.
>
> In regards to the coherent_dma_mask and dma_mask, I can't see why gntdev
> would have any dma addressing limitations, so we should be able to set
> both to 64 bits.  I also can't see why xen_drm_front would limit it to
> 32 bits, after all this is just the frontend, if anything it would be
> the backend that has a limitation. So, we should be able to set both
> dma_mask and coherent_dma_mask in xen_drm_front to 64 bits. Oleksandr,
> can you confirm?
I am totally fine with 64-bits in both cases and
agree with what Stefano says.

Re: [RFC PATCH] xen/gntdev: Stop abusing DT of_dma_configure API

2019-09-26 Thread Oleksandr Andrushchenko

On 9/26/19 1:46 PM, Robin Murphy wrote:
> On 2019-09-26 11:17 am, Oleksandr Andrushchenko wrote:
>>
>> On 9/26/19 12:49 PM, Julien Grall wrote:
>>> Hi Rob,
>>>
>>>
>>> On 9/25/19 10:50 PM, Rob Herring wrote:
>>>> As the comment says, this isn't a DT based device. of_dma_configure()
>>>> is going to stop allowing a NULL DT node, so this needs to be fixed.
>>>
>>> And this can't work on arch not selecting CONFIG_OF and can select
>>> CONFIG_XEN_GRANT_DMA_ALLOC.
>>>
>>> We are lucky enough on x86 because, AFAICT, arch_setup_dma_ops is just
>>> a nop.
>>>
>> No luck is needed as [1] does nothing for those platforms not using
>> CONFIG_OF
>>>>
>>>> Not sure exactly what setup besides arch_setup_dma_ops is needed...
>>>
>>> We probably want to update dma_mask, coherent_dma_mask and
>>> dma_pfn_offset.
>>>
>>> Also, while look at of_configure_dma, I noticed that we consider the
>>> DMA will not be coherent for the grant-table. Oleksandr, do you know
>>> why they can't be coherent?
>> The main and the only reason to use of_configure_dma is that if we don't
>> then we
>> are about to stay with dma_dummy_ops [2]. It effectively means that
>> operations on dma-bufs
>> will end up returning errors, like [3], [4], thus not making it possible
>> for Xen PV DRM and DMA
>> part of gntdev driver to do what we need (dma-bufs in our use-cases
>> allow zero-copying
>> while using graphics buffers and many more).
>>
>> I didn't find any better way of achieving that, but of_configure_dma...
>> If there is any better solution which will not break the existing
>> functionality then
>> I will definitely change the drivers so we do not abuse DT )
>> Before that, please keep in mind that merging this RFC will break Xen PV
>> DRM +
>> DMA buf support in gntdev...
>> Hope we can work out some acceptable solution, so everyone is happy
>
> As I mentioned elsewhere, the recent dma-direct rework means that 
> dma_dummy_ops are now only explicitly installed for the ACPI error 
> case, so - much as I may dislike it - you should get regular 
> (direct/SWIOTLB) ops by default again.
Ah, my bad, I missed that change. So, if no dummy dma ops are to be used 
then
I believe we can apply both changes, e.g. remove of_dma_configure from 
both of the drivers.
>
> Coherency is trickier - if the guest is allocating buffers for the PV 
> device, which may be shared directly with hardware by the host driver, 
> then the coherency of the PV device should really reflect that of the 
> underlying hardware to avoid potential problems. There are some cases 
> where the stage 2 attributes alone wouldn't be enough to correct a 
> mismatch.
>
> Robin.
Thank you,
Oleksandr

Re: [RFC PATCH] xen/gntdev: Stop abusing DT of_dma_configure API

2019-09-26 Thread Oleksandr Andrushchenko


On 9/26/19 12:49 PM, Julien Grall wrote:
> Hi Rob,
>
>
> On 9/25/19 10:50 PM, Rob Herring wrote:
>> As the comment says, this isn't a DT based device. of_dma_configure()
>> is going to stop allowing a NULL DT node, so this needs to be fixed.
>
> And this can't work on arch not selecting CONFIG_OF and can select 
> CONFIG_XEN_GRANT_DMA_ALLOC.
>
> We are lucky enough on x86 because, AFAICT, arch_setup_dma_ops is just 
> a nop.
>
No luck is needed as [1] does nothing for those platforms not using 
CONFIG_OF
>>
>> Not sure exactly what setup besides arch_setup_dma_ops is needed...
>
> We probably want to update dma_mask, coherent_dma_mask and 
> dma_pfn_offset.
>
> Also, while look at of_configure_dma, I noticed that we consider the 
> DMA will not be coherent for the grant-table. Oleksandr, do you know 
> why they can't be coherent?
The main and the only reason to use of_configure_dma is that if we don't 
then we
are about to stay with dma_dummy_ops [2]. It effectively means that 
operations on dma-bufs
will end up returning errors, like [3], [4], thus not making it possible 
for Xen PV DRM and DMA
part of gntdev driver to do what we need (dma-bufs in our use-cases 
allow zero-copying
while using graphics buffers and many more).

I didn't find any better way of achieving that, but of_configure_dma...
If there is any better solution which will not break the existing 
functionality then
I will definitely change the drivers so we do not abuse DT )
Before that, please keep in mind that merging this RFC will break Xen PV 
DRM +
DMA buf support in gntdev...
Hope we can work out some acceptable solution, so everyone is happy
>
> Cheers,
>

Thank you,
Oleksandr

[1] 
https://elixir.bootlin.com/linux/v5.3.1/source/include/linux/of_device.h#L109
[2] https://elixir.bootlin.com/linux/v5.3.1/source/kernel/dma/dummy.c#L33
[3] https://elixir.bootlin.com/linux/v5.3.1/source/kernel/dma/dummy.c#L11
[4] https://elixir.bootlin.com/linux/v5.3.1/source/kernel/dma/dummy.c#L18

Re: [Xen-devel] [PATCH 0/2] xen/gntdev: sanitize user interface handling

2019-07-18 Thread Oleksandr Andrushchenko


On 7/18/19 9:52 AM, Juergen Gross wrote:

The Xen gntdev driver's checking of the number of allowed mapped pages
is in need of some sanitizing work.

Juergen Gross (2):
   xen/gntdev: replace global limit of mapped pages by limit per call
   xen/gntdev: switch from kcalloc() to kvcalloc()

  drivers/xen/gntdev-common.h |  2 +-
  drivers/xen/gntdev-dmabuf.c | 11 +++---
  drivers/xen/gntdev.c| 52 ++---
  3 files changed, 25 insertions(+), 40 deletions(-)


For the series:
Reviewed-by: Oleksandr Andrushchenko

Re: [Xen-devel] [PATCH] ALSA: xen-front: fix unintention integer overflow on left shifts

2019-06-30 Thread Oleksandr Andrushchenko


On 6/28/19 11:46 AM, Takashi Iwai wrote:

On Thu, 27 Jun 2019 18:58:53 +0200,
Colin King wrote:

From: Colin Ian King 

Shifting the integer value 1 is evaluated using 32-bit
arithmetic and then used in an expression that expects a 64-bit
value, so there is potentially an integer overflow. Fix this
by using the BIT_ULL macro to perform the shift.

Addresses-Coverity: ("Unintentional integer overflow")
Signed-off-by: Colin Ian King 

Thank you for you patch,
Oleksandr

The fix is correct, but luckily we didn't hit the integer overflow, as
all passed values are less than 32bit.

In anyway, applied now.  Thanks.


Takashi


---
  sound/xen/xen_snd_front_alsa.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/xen/xen_snd_front_alsa.c b/sound/xen/xen_snd_front_alsa.c
index b14ab512c2ce..e01631959ed8 100644
--- a/sound/xen/xen_snd_front_alsa.c
+++ b/sound/xen/xen_snd_front_alsa.c
@@ -196,7 +196,7 @@ static u64 to_sndif_formats_mask(u64 alsa_formats)
mask = 0;
for (i = 0; i < ARRAY_SIZE(ALSA_SNDIF_FORMATS); i++)
if (pcm_format_to_bits(ALSA_SNDIF_FORMATS[i].alsa) & 
alsa_formats)
-   mask |= 1 << ALSA_SNDIF_FORMATS[i].sndif;
+   mask |= BIT_ULL(ALSA_SNDIF_FORMATS[i].sndif);
  
  	return mask;

  }
@@ -208,7 +208,7 @@ static u64 to_alsa_formats_mask(u64 sndif_formats)
  
  	mask = 0;

for (i = 0; i < ARRAY_SIZE(ALSA_SNDIF_FORMATS); i++)
-   if (1 << ALSA_SNDIF_FORMATS[i].sndif & sndif_formats)
+   if (BIT_ULL(ALSA_SNDIF_FORMATS[i].sndif) & sndif_formats)
mask |= pcm_format_to_bits(ALSA_SNDIF_FORMATS[i].alsa);
  
  	return mask;

--
2.20.1



___
Xen-devel mailing list
xen-de...@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen, fbfront: mark expected switch fall-through

2019-02-28 Thread Oleksandr Andrushchenko


+Xen-devel list

On 2/27/19 10:53 PM, Gustavo A. R. Silva wrote:

In preparation to enabling -Wimplicit-fallthrough, mark switch
cases where we are expecting to fall through.

This patch fixes the following warning:

drivers/video/fbdev/xen-fbfront.c: In function ‘xenfb_backend_changed’:
drivers/video/fbdev/xen-fbfront.c:678:6: warning: this statement may fall 
through [-Wimplicit-fallthrough=]
if (dev->state == XenbusStateClosed)
   ^
drivers/video/fbdev/xen-fbfront.c:681:2: note: here
   case XenbusStateClosing:
   ^~~~

Warning level 3 was used: -Wimplicit-fallthrough=3

Notice that, in this particular case, the code comment is modified
in accordance with what GCC is expecting to find.

This patch is part of the ongoing efforts to enable
-Wimplicit-fallthrough.

Signed-off-by: Gustavo A. R. Silva 
---
  drivers/video/fbdev/xen-fbfront.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/xen-fbfront.c 
b/drivers/video/fbdev/xen-fbfront.c
index 6a4bbc9e1fb0..a3d6b6db221b 100644
--- a/drivers/video/fbdev/xen-fbfront.c
+++ b/drivers/video/fbdev/xen-fbfront.c
@@ -677,7 +677,7 @@ static void xenfb_backend_changed(struct xenbus_device *dev,
case XenbusStateClosed:
if (dev->state == XenbusStateClosed)
break;
-   /* Missed the backend's CLOSING state -- fallthrough */
+   /* fall through - Missed the backend's CLOSING state. */
case XenbusStateClosing:
xenbus_frontend_closed(dev);
break;

Re: [Xen-devel] xen/evtchn and forced threaded irq

2019-02-25 Thread Oleksandr Andrushchenko

On 2/25/19 3:55 PM, Julien Grall wrote:

Hi Oleksandr,

On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:

On 2/22/19 3:33 PM, Julien Grall wrote:

Hi,

On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:

On 2/20/19 10:46 PM, Julien Grall wrote:
Discussing with my team, a solution that came up would be to
introduce one atomic field per event to record the number of event
received. I will explore that solution tomorrow.

How will this help if events have some payload?

What payload? The event channel does not carry any payload. It only
notify you that something happen. Then this is up to the user to
decide what to you with it.
Sorry, I was probably not precise enough. I mean that an event might
have
associated payload in the ring buffer, for example [1]. So, counting
events

may help somehow, but the ring's data may still be lost

From my understanding of event channels are edge interrupts. By
definition, they can be merged so you can get a signal notification to
the guest for multiple "events". So if you rely on the event to have
an associated payload, then you probably have done something wrong in
your driver.

I haven't implemented PV drivers myself, but I would expect either
side to block if there were no space in the ring.

What do you do in the displif driver when the ring is full?

It is handled by the originator, the display backend in our case: it
doesn't send

events if it sees that the ring will overflow. But I was worried about
such a generic change with counting number of events received and if this
really helps to recover in general case

Cheers,

[1]
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/io/displif.h;h=cc5de9cb1f35dedc99c866d73d086b19e496852a;hb=HEAD#l756

Re: [Xen-devel] xen/evtchn and forced threaded irq

2019-02-25 Thread Oleksandr Andrushchenko


On 2/22/19 3:33 PM, Julien Grall wrote:

Hi,

On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:

On 2/20/19 10:46 PM, Julien Grall wrote:
Discussing with my team, a solution that came up would be to 
introduce one atomic field per event to record the number of event 
received. I will explore that solution tomorrow.

How will this help if events have some payload?


What payload? The event channel does not carry any payload. It only 
notify you that something happen. Then this is up to the user to 
decide what to you with it.

Sorry, I was probably not precise enough. I mean that an event might have
associated payload in the ring buffer, for example [1]. So, counting events
may help somehow, but the ring's data may still be lost


Cheers,

[1] 
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/io/displif.h;h=cc5de9cb1f35dedc99c866d73d086b19e496852a;hb=HEAD#l756

Re: [Xen-devel] xen/evtchn and forced threaded irq

2019-02-22 Thread Oleksandr Andrushchenko


On 2/20/19 10:46 PM, Julien Grall wrote:

(+ Andrew and Jan for feedback on the event channel interrupt)

Hi Boris,

Thank you for the your feedback.

On 2/20/19 8:04 PM, Boris Ostrovsky wrote:

On 2/20/19 1:05 PM, Julien Grall wrote:

Hi,

On 20/02/2019 17:07, Boris Ostrovsky wrote:

On 2/20/19 9:15 AM, Julien Grall wrote:

Hi Boris,

Thank you for your answer.

On 20/02/2019 00:02, Boris Ostrovsky wrote:

On Tue, Feb 19, 2019 at 05:31:10PM +, Julien Grall wrote:

Hi all,

I have been looking at using Linux RT in Dom0. Once the guest is
started,
the console is ending to have a lot of warning (see trace below).

After some investigation, this is because the irq handler will now
be threaded.
I can reproduce the same error with the vanilla Linux when passing
the option
'threadirqs' on the command line (the trace below is from 5.0.0-rc7
that has
not RT support).

FWIW, the interrupt for port 6 is used to for the guest to
communicate with
xenstore.

   From my understanding, this is happening because the interrupt
handler is now
run in a thread. So we can have the following happening.

  Interrupt context    | Interrupt thread
   |
  receive interrupt port 6 |
  clear the evtchn port    |
  set IRQF_RUNTHREAD    |
  kick interrupt thread    |
   |    clear IRQF_RUNTHREAD
   |    call evtchn_interrupt
  receive interrupt port 6 |
  clear the evtchn port    |
  set IRQF_RUNTHREAD   |
  kick interrupt thread    |
   |    disable interrupt port 6
   | evtchn->enabled = false
   |    []
   |
   |    *** Handling the second
interrupt ***
   |    clear IRQF_RUNTHREAD
   |    call evtchn_interrupt
   |    WARN(...)

I am not entirely sure how to fix this. I have two solutions in 
mind:


1) Prevent the interrupt handler to be threaded. We would also
need to
switch from spin_lock to raw_spin_lock as the former may sleep on
RT-Linux.

2) Remove the warning


I think access to evtchn->enabled is racy so (with or without the
warning) we can't use it reliably.


Thinking about it, it would not be the only issue. The ring is sized
to contain only one instance of the same event. So if you receive
twice the event, you may overflow the ring.


Hm... That's another argument in favor of "unthreading" the handler.


I first thought it would be possible to unthread it. However,
wake_up_interruptible is using a spin_lock. On RT spin_lock can sleep,
so this cannot be used in an interrupt context.

So I think "unthreading" the handler is not an option here.


That sounds like a different problem. I.e. there are two issues:
* threaded interrupts don't work properly (races, ring overflow)
* evtchn_interrupt() (threaded or not) has spin_lock(), which is not
going to work for RT


I am afraid that's not correct, you can use spin_lock() in threaded 
interrupt handler.



The first can be fixed by using non-threaded handlers.


The two are somewhat related, if you use a non-threaded handler then 
you are not going to help the RT case.


In general, the unthreaded solution should be used in the last resort.









Another alternative could be to queue the irq if !evtchn->enabled 
and

handle it in evtchn_write() (which is where irq is supposed to be
re-enabled).

What do you mean by queue? Is it queueing in the ring?



No, I was thinking about having a new structure for deferred 
interrupts.


Hmmm, I am not entirely sure what would be the structure here. Could
you expand your thinking?


Some sort of a FIFO that stores {irq, data} tuple. It could obviously be
implemented as a ring but not necessarily as Xen shared ring (if that's
what you were referring to).


The underlying question is what happen if you miss an interrupt. Is it 
going to be ok? If no, then we have to record everything and can't 
kill/send an error to the user app because it is not its fault.


This means a FIFO would not be a viable. How do you size it? Static 
(i.e pre-defined) size is not going to be possible because you don't 
know how many interrupt you are going to receive before the thread 
handler runs. You can't make it grow dynamically as it make become 
quite big for the same reason.


Discussing with my team, a solution that came up would be to introduce 
one atomic field per event to record the number of event received. I 
will explore that solution tomorrow.

How will this help if events have some payload?


Cheers,

Re: [Xen-devel][PATCH 1/2] xen/gntdev: Do not destroy context while dma-bufs are in use

2019-02-15 Thread Oleksandr Andrushchenko


On 2/15/19 5:28 PM, Boris Ostrovsky wrote:

On 2/15/19 10:07 AM, Oleksandr Andrushchenko wrote:

On 2/15/19 5:03 PM, Boris Ostrovsky wrote:

On 2/14/19 9:23 AM, Oleksandr Andrushchenko wrote:

     /* DMA buffer export support. */
@@ -311,6 +317,7 @@ static void dmabuf_exp_release(struct kref *kref)
     dmabuf_exp_wait_obj_signal(gntdev_dmabuf->priv, gntdev_dmabuf);
   list_del(_dmabuf->next);
+    fput(gntdev_dmabuf->priv->filp);
   kfree(gntdev_dmabuf);
   }
   @@ -423,6 +430,7 @@ static int dmabuf_exp_from_pages(struct
gntdev_dmabuf_export_args *args)
   mutex_lock(>dmabuf_priv->lock);
   list_add(_dmabuf->next, >dmabuf_priv->exp_list);
   mutex_unlock(>dmabuf_priv->lock);
+    get_file(gntdev_dmabuf->priv->filp);

Not fget()?

fget wants file descriptor [1] and returns struct file *,
but we already have struct file*, so I use get_file [2]
which does what I need - increments the reference counter
on the file


Reviewed-by: Boris Ostrovsky 

Thank you,
any chance we can get this for 5.1?

Re: [Xen-devel][PATCH 1/2] xen/gntdev: Do not destroy context while dma-bufs are in use

2019-02-15 Thread Oleksandr Andrushchenko


On 2/15/19 5:03 PM, Boris Ostrovsky wrote:

On 2/14/19 9:23 AM, Oleksandr Andrushchenko wrote:
  
  /* DMA buffer export support. */

@@ -311,6 +317,7 @@ static void dmabuf_exp_release(struct kref *kref)
  
  	dmabuf_exp_wait_obj_signal(gntdev_dmabuf->priv, gntdev_dmabuf);

list_del(_dmabuf->next);
+   fput(gntdev_dmabuf->priv->filp);
kfree(gntdev_dmabuf);
  }
  
@@ -423,6 +430,7 @@ static int dmabuf_exp_from_pages(struct gntdev_dmabuf_export_args *args)

mutex_lock(>dmabuf_priv->lock);
list_add(_dmabuf->next, >dmabuf_priv->exp_list);
mutex_unlock(>dmabuf_priv->lock);
+   get_file(gntdev_dmabuf->priv->filp);

Not fget()?

fget wants file descriptor [1] and returns struct file *,
but we already have struct file*, so I use get_file [2]
which does what I need - increments the reference counter
on the file


-boris


[1] 
https://elixir.bootlin.com/linux/v5.0-rc6/source/include/linux/file.h#L46

[2] https://elixir.bootlin.com/linux/v5.0-rc6/source/include/linux/fs.h#L949

[Xen-devel][PATCH 2/2] xen/gntdev: Check and release imported dma-bufs on close

2019-02-14 Thread Oleksandr Andrushchenko

From: Oleksandr Andrushchenko 

Check if there are any imported dma-bufs left not released by
user-space when grant device's release callback is called and
free those if this is the case. This can happen if user-space
leaks the buffers because of a bug or application has been
terminated for any reason.

Signed-off-by: Oleksandr Andrushchenko 
---
 drivers/xen/gntdev-dmabuf.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
index d97fcfc5e558..2c4f324f8626 100644
--- a/drivers/xen/gntdev-dmabuf.c
+++ b/drivers/xen/gntdev-dmabuf.c
@@ -745,6 +745,14 @@ static int dmabuf_imp_release(struct gntdev_dmabuf_priv 
*priv, u32 fd)
return 0;
 }
 
+static void dmabuf_imp_release_all(struct gntdev_dmabuf_priv *priv)
+{
+   struct gntdev_dmabuf *q, *gntdev_dmabuf;
+
+   list_for_each_entry_safe(gntdev_dmabuf, q, >imp_list, next)
+   dmabuf_imp_release(priv, gntdev_dmabuf->fd);
+}
+
 /* DMA buffer IOCTL support. */
 
 long gntdev_ioctl_dmabuf_exp_from_refs(struct gntdev_priv *priv, int 
use_ptemod,
@@ -862,5 +870,6 @@ struct gntdev_dmabuf_priv *gntdev_dmabuf_init(struct file 
*filp)
 
 void gntdev_dmabuf_fini(struct gntdev_dmabuf_priv *priv)
 {
+   dmabuf_imp_release_all(priv);
kfree(priv);
 }
-- 
2.20.1

[Xen-devel][PATCH 1/2] xen/gntdev: Do not destroy context while dma-bufs are in use

2019-02-14 Thread Oleksandr Andrushchenko

From: Oleksandr Andrushchenko 

If there are exported DMA buffers which are still in use and
grant device is closed by either normal user-space close or by
a signal this leads to the grant device context to be destroyed,
thus making it not possible to correctly destroy those exported
buffers when they are returned back to gntdev and makes the module
crash:

[  339.617540] [] dmabuf_exp_ops_release+0x40/0xa8
[  339.617560] [] dma_buf_release+0x60/0x190
[  339.617577] [] __fput+0x88/0x1d0
[  339.617589] [] fput+0xc/0x18
[  339.617607] [] task_work_run+0x9c/0xc0
[  339.617622] [] do_notify_resume+0xfc/0x108

Fix this by referencing gntdev on each DMA buffer export and
unreferencing on buffer release.

Signed-off-by: Oleksandr Andrushchenko 
---
 drivers/xen/gntdev-dmabuf.c | 12 +++-
 drivers/xen/gntdev-dmabuf.h |  2 +-
 drivers/xen/gntdev.c|  2 +-
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
index cba6b586bfbd..d97fcfc5e558 100644
--- a/drivers/xen/gntdev-dmabuf.c
+++ b/drivers/xen/gntdev-dmabuf.c
@@ -80,6 +80,12 @@ struct gntdev_dmabuf_priv {
struct list_head imp_list;
/* This is the lock which protects dma_buf_xxx lists. */
struct mutex lock;
+   /*
+* We reference this file while exporting dma-bufs, so
+* the grant device context is not destroyed while there are
+* external users alive.
+*/
+   struct file *filp;
 };
 
 /* DMA buffer export support. */
@@ -311,6 +317,7 @@ static void dmabuf_exp_release(struct kref *kref)
 
dmabuf_exp_wait_obj_signal(gntdev_dmabuf->priv, gntdev_dmabuf);
list_del(_dmabuf->next);
+   fput(gntdev_dmabuf->priv->filp);
kfree(gntdev_dmabuf);
 }
 
@@ -423,6 +430,7 @@ static int dmabuf_exp_from_pages(struct 
gntdev_dmabuf_export_args *args)
mutex_lock(>dmabuf_priv->lock);
list_add(_dmabuf->next, >dmabuf_priv->exp_list);
mutex_unlock(>dmabuf_priv->lock);
+   get_file(gntdev_dmabuf->priv->filp);
return 0;
 
 fail:
@@ -834,7 +842,7 @@ long gntdev_ioctl_dmabuf_imp_release(struct gntdev_priv 
*priv,
return dmabuf_imp_release(priv->dmabuf_priv, op.fd);
 }
 
-struct gntdev_dmabuf_priv *gntdev_dmabuf_init(void)
+struct gntdev_dmabuf_priv *gntdev_dmabuf_init(struct file *filp)
 {
struct gntdev_dmabuf_priv *priv;
 
@@ -847,6 +855,8 @@ struct gntdev_dmabuf_priv *gntdev_dmabuf_init(void)
INIT_LIST_HEAD(>exp_wait_list);
INIT_LIST_HEAD(>imp_list);
 
+   priv->filp = filp;
+
return priv;
 }
 
diff --git a/drivers/xen/gntdev-dmabuf.h b/drivers/xen/gntdev-dmabuf.h
index 7220a53d0fc5..3d9b9cf9d5a1 100644
--- a/drivers/xen/gntdev-dmabuf.h
+++ b/drivers/xen/gntdev-dmabuf.h
@@ -14,7 +14,7 @@
 struct gntdev_dmabuf_priv;
 struct gntdev_priv;
 
-struct gntdev_dmabuf_priv *gntdev_dmabuf_init(void);
+struct gntdev_dmabuf_priv *gntdev_dmabuf_init(struct file *filp);
 
 void gntdev_dmabuf_fini(struct gntdev_dmabuf_priv *priv);
 
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index b0b02a501167..9d8e02cfd480 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -600,7 +600,7 @@ static int gntdev_open(struct inode *inode, struct file 
*flip)
mutex_init(>lock);
 
 #ifdef CONFIG_XEN_GNTDEV_DMABUF
-   priv->dmabuf_priv = gntdev_dmabuf_init();
+   priv->dmabuf_priv = gntdev_dmabuf_init(flip);
if (IS_ERR(priv->dmabuf_priv)) {
ret = PTR_ERR(priv->dmabuf_priv);
kfree(priv);
-- 
2.20.1

Re: [Xen-devel][PATCH] drm/xen-front: Fix mmap attributes for display buffers

2019-02-03 Thread Oleksandr Andrushchenko

On 1/29/19 9:07 PM, Julien Grall wrote:
> Hi Oleksandr,
>
> On 1/29/19 3:04 PM, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko 
>>
>> When GEM backing storage is allocated those are normal pages,
>> so there is no point using pgprot_writecombine while mmaping.
>> This fixes mismatch of buffer pages' memory attributes between
>> the frontend and backend which may cause screen artifacts.
>>
>> Fixes: c575b7eeb89f ("drm/xen-front: Add support for Xen PV display 
>> frontend")
>>
>> Signed-off-by: Oleksandr Andrushchenko 
>> 
>> Suggested-by: Julien Grall 
>> ---
>>   drivers/gpu/drm/xen/xen_drm_front_gem.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
>> b/drivers/gpu/drm/xen/xen_drm_front_gem.c
>> index d303a2e17f5e..9d5c03d7668d 100644
>> --- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
>> +++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
>> @@ -235,8 +235,7 @@ static int gem_mmap_obj(struct xen_gem_object 
>> *xen_obj,
>>   vma->vm_flags &= ~VM_PFNMAP;
>>   vma->vm_flags |= VM_MIXEDMAP;
>>   vma->vm_pgoff = 0;
>> -    vma->vm_page_prot =
>> - pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
>> +    vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>
> The patch looks good to me. It would be worth expanding the comment a 
> bit before to explain that we overwrite vm_page_prot to use cacheable 
> attribute as required by the Xen ABI.
>
> With the comment updated:
>
> Acked-by: Julien Grall 
>
> Cheers,
>
Applied to drm-misc-next

Re: [Xen-devel] [PATCH -next] drm/xen-front: Drop pointless static qualifier in fb_destroy()

2019-02-03 Thread Oleksandr Andrushchenko

On 1/26/19 2:05 PM, YueHaibing wrote:
> There is no need to have the 'struct drm_framebuffer *fb' variable
> static since new value always be assigned before use it.
>
> Signed-off-by: YueHaibing 
> ---
>   drivers/gpu/drm/xen/xen_drm_front_kms.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c 
> b/drivers/gpu/drm/xen/xen_drm_front_kms.c
> index 860da05..c2955d3 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front_kms.c
> +++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c
> @@ -54,7 +54,7 @@ static void fb_destroy(struct drm_framebuffer *fb)
> const struct drm_mode_fb_cmd2 *mode_cmd)
>   {
>   struct xen_drm_front_drm_info *drm_info = dev->dev_private;
> - static struct drm_framebuffer *fb;
> + struct drm_framebuffer *fb;
>   struct drm_gem_object *gem_obj;
>   int ret;
>   
Applied to drm-misc-next

Re: [Xen-devel] xen/mem-reservation API and out-of-tree kernel modules

On 2/1/19 11:14 AM, Juergen Gross wrote:
> On 01/02/2019 09:39, Oleksandr Andrushchenko wrote:
>> On 1/31/19 11:44 PM, Stefano Stabellini wrote:
>>> On Thu, 31 Jan 2019, Oleksandr Andrushchenko wrote:
>>>> Hello,
>>>>
>>>> I am working on porting an out-of-tree kernel driver to the kernel
>>>> 5.0 and that driver uses functionality provided by
>>>> drivers/xen/mem-reservation.c
>>>> module.  Since commit [1] it is not possible to build a kernel module
>>>> which uses mem-reservation API as xen_scrub_pages variable, which is
>>>> checked in
>>>> xenmem_reservation_scrub_page, became a kernel module parameter and is
>>>> now only
>>>> accessible for built-in modules:
>>>>
>>>> static inline void xenmem_reservation_scrub_page(struct page *page)
>>>> ^
>>>> {
>>>>    if (xen_scrub_pages)
>>>>    ^^^
>>>>        clear_highpage(page);
>>>> }
>>>>
>>>> This results in link-time warning:
>>>>
>>>>    WARNING: "xen_scrub_pages" [yourmodule.ko] undefined!
>>>>
>>>> and thus not allowing the module to run. At the moment I can only see a
>>>> possible fix
>>>> for this by making the following change:
>>>>
>>>> diff --git a/drivers/xen/mem-reservation.c b/drivers/xen/mem-reservation.c
>>>> index 3782cf070338..85fecfec50e1 100644
>>>> --- a/drivers/xen/mem-reservation.c
>>>> +++ b/drivers/xen/mem-reservation.c
>>>> @@ -18,6 +18,7 @@
>>>>
>>>> bool __read_mostly xen_scrub_pages =
>>>> IS_ENABLED(CONFIG_XEN_SCRUB_PAGES_DEFAULT);
>>>> core_param(xen_scrub_pages, xen_scrub_pages, bool, 0);
>>>> +EXPORT_SYMBOL(xen_scrub_pages);
>>>>
>>>> but this looks a bit unusual for the kernel?
>>>>
>>>> I am looking for community advice here and help
>>>>
>>>> Thank you,
>>>> Oleksandr
>>>>
>>>> [1]
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=197ecb3802c04499d8ff4f8cb28f6efa008067db
>>> The alternative would be to turn xenmem_reservation_scrub_page into a
>>> regular function (not a static inline)?
>> Yes, it seems there is no other reasonable solution to this, but
>> a regular function. I'll send a patch for that
> What would you gain? This function would need to be exported.
Yes, this is true, the function should be exported then
> So its either the variable or the function.
I am a bit confused with this because I'll have to export
module parameter in this case, e.g.

core_param(xen_scrub_pages, xen_scrub_pages, bool, 0);
EXPORT_SYMBOL(xen_scrub_pages);

which looks a bit unusual to me
>
> Juergen

Re: [Xen-devel][PATCH] drm/xen-front: Fix mmap attributes for display buffers

On 1/30/19 11:09 AM, Oleksandr Andrushchenko wrote:
> On 1/29/19 9:07 PM, Julien Grall wrote:
>> Hi Oleksandr,
>>
>> On 1/29/19 3:04 PM, Oleksandr Andrushchenko wrote:
>>> From: Oleksandr Andrushchenko 
>>>
>>> When GEM backing storage is allocated those are normal pages,
>>> so there is no point using pgprot_writecombine while mmaping.
>>> This fixes mismatch of buffer pages' memory attributes between
>>> the frontend and backend which may cause screen artifacts.
>>>
>>> Fixes: c575b7eeb89f ("drm/xen-front: Add support for Xen PV display 
>>> frontend")
>>>
>>> Signed-off-by: Oleksandr Andrushchenko 
>>> 
>>> Suggested-by: Julien Grall 
>>> ---
>>>   drivers/gpu/drm/xen/xen_drm_front_gem.c | 5 ++---
>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
>>> b/drivers/gpu/drm/xen/xen_drm_front_gem.c
>>> index d303a2e17f5e..9d5c03d7668d 100644
>>> --- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
>>> +++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
>>> @@ -235,8 +235,7 @@ static int gem_mmap_obj(struct xen_gem_object 
>>> *xen_obj,
>>>   vma->vm_flags &= ~VM_PFNMAP;
>>>   vma->vm_flags |= VM_MIXEDMAP;
>>>   vma->vm_pgoff = 0;
>>> -    vma->vm_page_prot =
>>> - pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
>>> +    vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>>
>> The patch looks good to me. It would be worth expanding the comment a 
>> bit before to explain that we overwrite vm_page_prot to use cacheable 
>> attribute as required by the Xen ABI.
>>
> Ok, then I'll put:
>
> +   /*
> +    * According to Xen on ARM ABI (xen/include/public/arch-arm.h):
> +    * all memory which is shared with other entities in the system
> +    * (including the hypervisor and other guests) must reside in 
> memory
> +    * which is mapped as Normal Inner Write-Back Outer Write-Back
> +    * Inner-Shareable.
> +    */
>     vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> Please let me know if this is not what you want
>> With the comment updated:
>>
>> Acked-by: Julien Grall 
>>
If nobody objects I'll apply this to drm-misc-fixes next Monday
>> Cheers,
>>
> Thank you,
> Oleksandr

Re: [PATCH -next] drm/xen-front: Drop pointless static qualifier in fb_destroy()

On 1/28/19 8:36 AM, Oleksandr Andrushchenko wrote:
> On 1/26/19 2:05 PM, YueHaibing wrote:
>> There is no need to have the 'struct drm_framebuffer *fb' variable
>> static since new value always be assigned before use it.
>>
>> Signed-off-by: YueHaibing 
> Good catch, thank you!
> Reviewed-by: Oleksandr Andrushchenko 
If nobody objects I'll apply this to drm-misc-fixes next Monday
>> ---
>>   drivers/gpu/drm/xen/xen_drm_front_kms.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c 
>> b/drivers/gpu/drm/xen/xen_drm_front_kms.c
>> index 860da05..c2955d3 100644
>> --- a/drivers/gpu/drm/xen/xen_drm_front_kms.c
>> +++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c
>> @@ -54,7 +54,7 @@ static void fb_destroy(struct drm_framebuffer *fb)
>>     const struct drm_mode_fb_cmd2 *mode_cmd)
>>   {
>>   struct xen_drm_front_drm_info *drm_info = dev->dev_private;
>> -    static struct drm_framebuffer *fb;
>> +    struct drm_framebuffer *fb;
>>   struct drm_gem_object *gem_obj;
>>   int ret;
>>
>>
>>
>>
>>

Re: [Xen-devel] xen/mem-reservation API and out-of-tree kernel modules

On 1/31/19 11:44 PM, Stefano Stabellini wrote:
> On Thu, 31 Jan 2019, Oleksandr Andrushchenko wrote:
>> Hello,
>>
>> I am working on porting an out-of-tree kernel driver to the kernel
>> 5.0 and that driver uses functionality provided by
>> drivers/xen/mem-reservation.c
>> module.  Since commit [1] it is not possible to build a kernel module
>> which uses mem-reservation API as xen_scrub_pages variable, which is
>> checked in
>> xenmem_reservation_scrub_page, became a kernel module parameter and is
>> now only
>> accessible for built-in modules:
>>
>> static inline void xenmem_reservation_scrub_page(struct page *page)
>> ^
>> {
>>       if (xen_scrub_pages)
>>       ^^^
>>           clear_highpage(page);
>> }
>>
>> This results in link-time warning:
>>
>>       WARNING: "xen_scrub_pages" [yourmodule.ko] undefined!
>>
>> and thus not allowing the module to run. At the moment I can only see a
>> possible fix
>> for this by making the following change:
>>
>> diff --git a/drivers/xen/mem-reservation.c b/drivers/xen/mem-reservation.c
>> index 3782cf070338..85fecfec50e1 100644
>> --- a/drivers/xen/mem-reservation.c
>> +++ b/drivers/xen/mem-reservation.c
>> @@ -18,6 +18,7 @@
>>
>>    bool __read_mostly xen_scrub_pages =
>> IS_ENABLED(CONFIG_XEN_SCRUB_PAGES_DEFAULT);
>>    core_param(xen_scrub_pages, xen_scrub_pages, bool, 0);
>> +EXPORT_SYMBOL(xen_scrub_pages);
>>
>> but this looks a bit unusual for the kernel?
>>
>> I am looking for community advice here and help
>>
>> Thank you,
>> Oleksandr
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=197ecb3802c04499d8ff4f8cb28f6efa008067db
> The alternative would be to turn xenmem_reservation_scrub_page into a
> regular function (not a static inline)?
Yes, it seems there is no other reasonable solution to this, but
a regular function. I'll send a patch for that

Thank you,
Oleksandr

Re: [Xen-devel] xen/mem-reservation API and out-of-tree kernel modules

On 2/1/19 10:27 AM, Christoph Hellwig wrote:
> On Thu, Jan 31, 2019 at 01:44:15PM -0800, Stefano Stabellini wrote:
>> The alternative would be to turn xenmem_reservation_scrub_page into a
>> regular function (not a static inline)?
> All that is a moot point until said currently out of tree module gets
> submitted for inclusion anyway.
Indeed this is a moot point, so I can't argue here.
But this is how it is and unfortunately we have to live
with those modules and depend on 3rd parties willing or not
to disclose their sources to public...

[Xen-devel] xen/mem-reservation API and out-of-tree kernel modules

2019-01-31 Thread Oleksandr Andrushchenko

Hello,

I am working on porting an out-of-tree kernel driver to the kernel
5.0 and that driver uses functionality provided by 
drivers/xen/mem-reservation.c
module.  Since commit [1] it is not possible to build a kernel module
which uses mem-reservation API as xen_scrub_pages variable, which is 
checked in
xenmem_reservation_scrub_page, became a kernel module parameter and is 
now only
accessible for built-in modules:

static inline void xenmem_reservation_scrub_page(struct page *page)
^
{
     if (xen_scrub_pages)
     ^^^
         clear_highpage(page);
}

This results in link-time warning:

     WARNING: "xen_scrub_pages" [yourmodule.ko] undefined!

and thus not allowing the module to run. At the moment I can only see a 
possible fix
for this by making the following change:

diff --git a/drivers/xen/mem-reservation.c b/drivers/xen/mem-reservation.c
index 3782cf070338..85fecfec50e1 100644
--- a/drivers/xen/mem-reservation.c
+++ b/drivers/xen/mem-reservation.c
@@ -18,6 +18,7 @@

  bool __read_mostly xen_scrub_pages = 
IS_ENABLED(CONFIG_XEN_SCRUB_PAGES_DEFAULT);
  core_param(xen_scrub_pages, xen_scrub_pages, bool, 0);
+EXPORT_SYMBOL(xen_scrub_pages);

but this looks a bit unusual for the kernel?

I am looking for community advice here and help

Thank you,
Oleksandr

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=197ecb3802c04499d8ff4f8cb28f6efa008067db

Re: [Xen-devel][PATCH] drm/xen-front: Fix mmap attributes for display buffers

2019-01-30 Thread Oleksandr Andrushchenko

On 1/29/19 9:07 PM, Julien Grall wrote:
> Hi Oleksandr,
>
> On 1/29/19 3:04 PM, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko 
>>
>> When GEM backing storage is allocated those are normal pages,
>> so there is no point using pgprot_writecombine while mmaping.
>> This fixes mismatch of buffer pages' memory attributes between
>> the frontend and backend which may cause screen artifacts.
>>
>> Fixes: c575b7eeb89f ("drm/xen-front: Add support for Xen PV display 
>> frontend")
>>
>> Signed-off-by: Oleksandr Andrushchenko 
>> 
>> Suggested-by: Julien Grall 
>> ---
>>   drivers/gpu/drm/xen/xen_drm_front_gem.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
>> b/drivers/gpu/drm/xen/xen_drm_front_gem.c
>> index d303a2e17f5e..9d5c03d7668d 100644
>> --- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
>> +++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
>> @@ -235,8 +235,7 @@ static int gem_mmap_obj(struct xen_gem_object 
>> *xen_obj,
>>   vma->vm_flags &= ~VM_PFNMAP;
>>   vma->vm_flags |= VM_MIXEDMAP;
>>   vma->vm_pgoff = 0;
>> -    vma->vm_page_prot =
>> - pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
>> +    vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>
> The patch looks good to me. It would be worth expanding the comment a 
> bit before to explain that we overwrite vm_page_prot to use cacheable 
> attribute as required by the Xen ABI.
>
Ok, then I'll put:

+   /*
+    * According to Xen on ARM ABI (xen/include/public/arch-arm.h):
+    * all memory which is shared with other entities in the system
+    * (including the hypervisor and other guests) must reside in memory
+    * which is mapped as Normal Inner Write-Back Outer Write-Back
+    * Inner-Shareable.
+    */
     vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
Please let me know if this is not what you want
> With the comment updated:
>
> Acked-by: Julien Grall 
>
> Cheers,
>
Thank you,
Oleksandr

Re: [Xen-devel] [PATCH v2] drm/xen-front: Make shmem backed display buffer coherent

2019-01-30 Thread Oleksandr Andrushchenko

Dropped in favor of https://lkml.org/lkml/2019/1/29/910

On 1/24/19 5:02 PM, Julien Grall wrote:
>
>
> On 24/01/2019 14:34, Oleksandr Andrushchenko wrote:
>> Hello, Julien!
>
> Hi,
>
>> On 1/22/19 1:44 PM, Julien Grall wrote:
>>>
>>>
>>> On 1/22/19 10:28 AM, Oleksandr Andrushchenko wrote:
>>>> Hello, Julien!
>>>
>>> Hi,
>>>
>>>> On 1/21/19 7:09 PM, Julien Grall wrote:
>>>> Well, I didn't get the attributes of pages at the backend side, but 
>>>> IMO
>>>> those
>>>> do not matter in my use-case (for simplicity I am not using
>>>> zero-copying at
>>>> backend side):
>>>
>>> They are actually important no matter what is your use case. If you
>>> access the same physical page with different attributes, then you are
>>> asking for trouble.
>> So, we have:
>>
>> DomU: frontend side
>> 
>> !PTE_RDONLY + PTE_PXN + PTE_SHARED + PTE_AF + PTE_UXN +
>> PTE_ATTRINDX(MT_NORMAL)
>
> I still don't understand how you came up with MT_NORMAL when you seem 
> to confirm...
>
>>
>> DomD: backend side
>> 
>> PTE_USER + !PTE_RDONLY + PTE_PXN + PTE_NG + PTE_CONT + PTE_TABLE_BIT +
>> PTE_UXN + PTE_ATTRINDX(MT_NORMAL)
>>
>>   From the above it seems that I don't violate cached/non-cached
>> agreement here
>>>
>>> This is why Xen imposes all the pages shared to have their memory
>>> attributes following some rules. Actually, speaking with Mark R., we
>>> may want to tight a bit more the attributes.
>>>
>>>>
>>>> 1. Frontend device allocates display buffer pages which come from 
>>>> shmem
>>>> and have these attributes:
>>>> !PTE_RDONLY + PTE_PXN + PTE_SHARED + PTE_AF + PTE_UXN +
>>>> PTE_ATTRINDX(MT_NORMAL)
>>>
>>> My knowledge of Xen DRM is inexistent. However, looking at the code in
>>> 5.0-rc2, I don't seem to find the same attributes. For instance
>>> xen_drm_front_gem_prime_vmap and gem_mmap_obj are using
>>> pgprot_writecombine. So it looks like, the mapping will be
>>> non-cacheable on Arm64.
>>>
>>> Can you explain how you came up to these attributes?
>> pgprot_writecombine is PTE_ATTRINDX(MT_NORMAL_NC), so it seems to be
>> applicable here? [1]
>
> ... that MT_NORMAL_NC is used for the frontend pages.
>
> MT_NORMAL_NC is different from MT_NORMAL. The use of the former will 
> result to non-cacheable memory while the latter will result to 
> cacheable memory.
>
> To me, this looks like the exact reason why you see artifact on the 
> display buffer. As the author of this code, can you explain why you 
> decided to use pgprot_writecombine here instead of relying on the 
> default VMA prot?
>
> [...]
>
>>> We actually never required to use cache flush in other PV protocol, so
>>> I still don't understand why the PV DRM should be different here.
>> Well, you are right. But at the same time not flushing the buffer makes
>> troubles,
>> so this is why I am trying to figure out what is wrong here.
>
> The cache flush is likely hiding the real problem rather than solving it.
>
>>>
>>> To me, it looks like that you are either missing some barriers
>> Barriers for the buffer? Not sure what you mean here.
>
> If you share information between two entities, you may need some 
> ordering so the information are seen consistently by the consumer 
> side. This can be achieved by using barriers.
>
>> Even more, we have
>> a use case
>> when the buffer is not touched by CPU in DomD and is solely owned by 
>> the HW.
>
> Memory ordering issues are subtle. The fact that one of your use-case 
> works does not imply that barriers are not necessary. I am not saying 
> there are a missing barriers, I am only pointed out potential reasons.
>
> Anyway, I don't think your problem is a missing barriers here. It is 
> more likely because of mismatch memory attributes (see above).
>
> Cheers,
>

[Xen-devel][PATCH] drm/xen-front: Fix mmap attributes for display buffers

2019-01-29 Thread Oleksandr Andrushchenko

From: Oleksandr Andrushchenko 

When GEM backing storage is allocated those are normal pages,
so there is no point using pgprot_writecombine while mmaping.
This fixes mismatch of buffer pages' memory attributes between
the frontend and backend which may cause screen artifacts.

Fixes: c575b7eeb89f ("drm/xen-front: Add support for Xen PV display frontend")

Signed-off-by: Oleksandr Andrushchenko 
Suggested-by: Julien Grall 
---
 drivers/gpu/drm/xen/xen_drm_front_gem.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
b/drivers/gpu/drm/xen/xen_drm_front_gem.c
index d303a2e17f5e..9d5c03d7668d 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -235,8 +235,7 @@ static int gem_mmap_obj(struct xen_gem_object *xen_obj,
vma->vm_flags &= ~VM_PFNMAP;
vma->vm_flags |= VM_MIXEDMAP;
vma->vm_pgoff = 0;
-   vma->vm_page_prot =
-   pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
+   vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
 
/*
 * vm_operations_struct.fault handler will be called if CPU access
@@ -283,7 +282,7 @@ void *xen_drm_front_gem_prime_vmap(struct drm_gem_object 
*gem_obj)
return NULL;
 
return vmap(xen_obj->pages, xen_obj->num_pages,
-   VM_MAP, pgprot_writecombine(PAGE_KERNEL));
+   VM_MAP, PAGE_KERNEL);
 }
 
 void xen_drm_front_gem_prime_vunmap(struct drm_gem_object *gem_obj,
-- 
2.20.1

Re: [Xen-devel] [PATCH v2] drm/xen-front: Make shmem backed display buffer coherent

2019-01-29 Thread Oleksandr Andrushchenko

On 1/24/19 5:02 PM, Julien Grall wrote:
>
>
> On 24/01/2019 14:34, Oleksandr Andrushchenko wrote:
>> Hello, Julien!
>
> Hi,
>
>> On 1/22/19 1:44 PM, Julien Grall wrote:
>>>
>>>
>>> On 1/22/19 10:28 AM, Oleksandr Andrushchenko wrote:
>>>> Hello, Julien!
>>>
>>> Hi,
>>>
>>>> On 1/21/19 7:09 PM, Julien Grall wrote:
>>>> Well, I didn't get the attributes of pages at the backend side, but 
>>>> IMO
>>>> those
>>>> do not matter in my use-case (for simplicity I am not using
>>>> zero-copying at
>>>> backend side):
>>>
>>> They are actually important no matter what is your use case. If you
>>> access the same physical page with different attributes, then you are
>>> asking for trouble.
>> So, we have:
>>
>> DomU: frontend side
>> 
>> !PTE_RDONLY + PTE_PXN + PTE_SHARED + PTE_AF + PTE_UXN +
>> PTE_ATTRINDX(MT_NORMAL)
>
> I still don't understand how you came up with MT_NORMAL when you seem 
> to confirm...
>
>>
>> DomD: backend side
>> 
>> PTE_USER + !PTE_RDONLY + PTE_PXN + PTE_NG + PTE_CONT + PTE_TABLE_BIT +
>> PTE_UXN + PTE_ATTRINDX(MT_NORMAL)
>>
>>   From the above it seems that I don't violate cached/non-cached
>> agreement here
>>>
>>> This is why Xen imposes all the pages shared to have their memory
>>> attributes following some rules. Actually, speaking with Mark R., we
>>> may want to tight a bit more the attributes.
>>>
>>>>
>>>> 1. Frontend device allocates display buffer pages which come from 
>>>> shmem
>>>> and have these attributes:
>>>> !PTE_RDONLY + PTE_PXN + PTE_SHARED + PTE_AF + PTE_UXN +
>>>> PTE_ATTRINDX(MT_NORMAL)
>>>
>>> My knowledge of Xen DRM is inexistent. However, looking at the code in
>>> 5.0-rc2, I don't seem to find the same attributes. For instance
>>> xen_drm_front_gem_prime_vmap and gem_mmap_obj are using
>>> pgprot_writecombine. So it looks like, the mapping will be
>>> non-cacheable on Arm64.
>>>
>>> Can you explain how you came up to these attributes?
>> pgprot_writecombine is PTE_ATTRINDX(MT_NORMAL_NC), so it seems to be
>> applicable here? [1]
>
> ... that MT_NORMAL_NC is used for the frontend pages.
>
> MT_NORMAL_NC is different from MT_NORMAL. The use of the former will 
> result to non-cacheable memory while the latter will result to 
> cacheable memory.
>
> To me, this looks like the exact reason why you see artifact on the 
> display buffer. As the author of this code, can you explain why you 
> decided to use pgprot_writecombine here instead of relying on the 
> default VMA prot?
>
> [...]
Sorry for late reply, it took me quite some time to re-check all the 
use-cases
that we have with PV DRM.
First of all I found a bug in my debug code which reported page 
attributes and
now I can confirm that all the use-cases sue MT_NORMAL, both backend and 
frontend.
You are right: there is no reason to have pgprot_writecombine and indeed 
I have
to rely on almost default VMA prot. I came up with the following (used 
by omap drm,
for example):

diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
b/drivers/gpu/drm/xen/xen_drm_front_gem.c
index ae28ad4b4254..867800a2ed42 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -238,8 +238,7 @@ static int gem_mmap_obj(struct xen_gem_object *xen_obj,
     vma->vm_flags &= ~VM_PFNMAP;
     vma->vm_flags |= VM_MIXEDMAP;
     vma->vm_pgoff = 0;
-   vma->vm_page_prot =
- pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
+   vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);

     {
     int cnt = xen_obj->num_pages > 5 ? 5 : xen_obj->num_pages;
@@ -295,7 +294,7 @@ void *xen_drm_front_gem_prime_vmap(struct 
drm_gem_object *gem_obj)
     return NULL;

     return vmap(xen_obj->pages, xen_obj->num_pages,
-   VM_MAP, pgprot_writecombine(PAGE_KERNEL));
+   VM_MAP, PAGE_KERNEL);
  }

With the above all the artifacts are gone now as page attributes are the 
same across
all domains. So, I consider this as a fix and will send it as v3 or better
drop this patch and have a new one.

THANK YOU for helping figuring this out!
>
>>> We actually never required to use cache flush in other PV protocol, so
>>> I still don't understand why the PV DRM should be different here.
>> Well, you are right. But at the same time not flushing the buffer makes
>> troubles,
>>

Re: [Xen-devel] [PATCH] arch/arm/xen: Remove duplicate header

2019-01-28 Thread Oleksandr Andrushchenko


+Boris and Juergen who can also help getting it in

On 1/28/19 8:34 AM, Souptick Joarder wrote:

On Mon, Jan 14, 2019 at 4:08 PM Oleksandr Andrushchenko
 wrote:

On 1/7/19 7:37 PM, Souptick Joarder wrote:

Remove duplicate header which is included twice.

Signed-off-by: Souptick Joarder 

Reviewed-by: Oleksandr Andrushchenko 

Can we get this patch in queue for 5.1 ?

---
   arch/arm/xen/mm.c | 1 -
   1 file changed, 1 deletion(-)

diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index cb44aa2..e1d44b9 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -7,7 +7,6 @@
   #include 
   #include 
   #include 
-#include 
   #include 
   #include

Re: [PATCH -next] drm/xen-front: Drop pointless static qualifier in fb_destroy()

2019-01-27 Thread Oleksandr Andrushchenko

On 1/26/19 2:05 PM, YueHaibing wrote:
> There is no need to have the 'struct drm_framebuffer *fb' variable
> static since new value always be assigned before use it.
>
> Signed-off-by: YueHaibing 
Good catch, thank you!
Reviewed-by: Oleksandr Andrushchenko 
> ---
>   drivers/gpu/drm/xen/xen_drm_front_kms.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c 
> b/drivers/gpu/drm/xen/xen_drm_front_kms.c
> index 860da05..c2955d3 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front_kms.c
> +++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c
> @@ -54,7 +54,7 @@ static void fb_destroy(struct drm_framebuffer *fb)
> const struct drm_mode_fb_cmd2 *mode_cmd)
>   {
>   struct xen_drm_front_drm_info *drm_info = dev->dev_private;
> - static struct drm_framebuffer *fb;
> + struct drm_framebuffer *fb;
>   struct drm_gem_object *gem_obj;
>   int ret;
>   
>
>
>
>
>

Re: [Xen-devel] [PATCH v2] drm/xen-front: Make shmem backed display buffer coherent

2019-01-24 Thread Oleksandr Andrushchenko

Hello, Julien!
Sorry for the late reply - it took quite some time to collect the data 
requested.

On 1/22/19 1:44 PM, Julien Grall wrote:
>
>
> On 1/22/19 10:28 AM, Oleksandr Andrushchenko wrote:
>> Hello, Julien!
>
> Hi,
>
>> On 1/21/19 7:09 PM, Julien Grall wrote:
>> Well, I didn't get the attributes of pages at the backend side, but IMO
>> those
>> do not matter in my use-case (for simplicity I am not using 
>> zero-copying at
>> backend side):
>
> They are actually important no matter what is your use case. If you 
> access the same physical page with different attributes, then you are 
> asking for trouble.
So, we have:

DomU: frontend side

!PTE_RDONLY + PTE_PXN + PTE_SHARED + PTE_AF + PTE_UXN + 
PTE_ATTRINDX(MT_NORMAL)

DomD: backend side

PTE_USER + !PTE_RDONLY + PTE_PXN + PTE_NG + PTE_CONT + PTE_TABLE_BIT + 
PTE_UXN + PTE_ATTRINDX(MT_NORMAL)

 From the above it seems that I don't violate cached/non-cached 
agreement here
>
> This is why Xen imposes all the pages shared to have their memory 
> attributes following some rules. Actually, speaking with Mark R., we 
> may want to tight a bit more the attributes.
>
>>
>> 1. Frontend device allocates display buffer pages which come from shmem
>> and have these attributes:
>> !PTE_RDONLY + PTE_PXN + PTE_SHARED + PTE_AF + PTE_UXN +
>> PTE_ATTRINDX(MT_NORMAL)
>
> My knowledge of Xen DRM is inexistent. However, looking at the code in 
> 5.0-rc2, I don't seem to find the same attributes. For instance 
> xen_drm_front_gem_prime_vmap and gem_mmap_obj are using 
> pgprot_writecombine. So it looks like, the mapping will be 
> non-cacheable on Arm64.
>
> Can you explain how you came up to these attributes?
pgprot_writecombine is PTE_ATTRINDX(MT_NORMAL_NC), so it seems to be 
applicable here? [1]
>
>>
>> 2. Frontend grants references to these pages and shares those with the
>> backend
>>
>> 3. Backend is a user-space application (Weston client), so it uses
>> gntdev kernel
>> driver to mmap the pages. The pages, which are used by gntdev, are those
>> coming
>> from the Xen balloon driver and I believe they are all normal memory and
>> shouldn't be non-cached.
>>
>> 4. Once the frontend starts displaying it flips the buffers and backend
>> does *memcpy*
>> from the frontend-backend shared buffer into Weston's buffer. This means
>> no HW at the backend side touches the shared buffer.
>>
>> 5. I can see distorted picture.
>>
>> Previously I used setup with zero-copying, so then the picture becomes
>> more complicated
>> in terms of buffers and how those used by the backed, but anyways it
>> seems that the
>> very basic scenario with memory copying doesn't work for me.
>>
>> Using DMA API on frontend's side does help - no artifacts are seen.
>> This is why I'm thinking that this is related to frontend/kernel side
>> rather then to
>> the backend side. This is why I'm thinking this is related to caches and
>> trying to figure
>> out what can be done here instead of using DMA API.
>
> We actually never required to use cache flush in other PV protocol, so 
> I still don't understand why the PV DRM should be different here.
Well, you are right. But at the same time not flushing the buffer makes 
troubles,
so this is why I am trying to figure out what is wrong here.
>
> To me, it looks like that you are either missing some barriers
Barriers for the buffer? Not sure what you mean here. Even more, we have 
a use case
when the buffer is not touched by CPU in DomD and is solely owned by the HW.
> or the memory attributes are not correct.
Please see above - I do need your advice here
>
> Cheers,
>
Thank you very much for your time,
Oleksandr

[1] 
https://elixir.bootlin.com/linux/v5.0-rc2/source/arch/arm64/include/asm/pgtable.h#L414

Re: [Xen-devel] [PATCH v2] drm/xen-front: Make shmem backed display buffer coherent

2019-01-22 Thread Oleksandr Andrushchenko

Hello, Julien!

On 1/21/19 7:09 PM, Julien Grall wrote:
> Hello,
>
> On 21/01/2019 12:43, Oleksandr Andrushchenko wrote:
>> On 1/18/19 1:43 PM, Julien Grall wrote:
>>> On 18/01/2019 09:40, Oleksandr Andrushchenko wrote:
>>>> On 1/17/19 11:18 AM, Christoph Hellwig wrote:
>>>>> On Wed, Jan 16, 2019 at 06:43:29AM +, Oleksandr Andrushchenko
>>>>> wrote:
>>>>>>> This whole issue keeps getting more and more confusing.
>>>>>> Well, I don't really do DMA here, but instead the buffers in
>>>>>> question are shared with other Xen domain, so effectively it
>>>>>> could be thought of some sort of DMA here, where the "device" is
>>>>>> that remote domain. If the buffers are not flushed then the
>>>>>> remote part sees some inconsistency which in my case results
>>>>>> in artifacts on screen while displaying the buffers.
>>>>>> When buffers are allocated via DMA API then there are no artifacts;
>>>>>> if buffers are allocated with shmem + DMA mapping then there are no
>>>>>> artifacts as well.
>>>>>> The only offending use-case is when I use shmem backed buffers,
>>>>>> but do not flush them
>>>>> The right answer would be to implement cache maintainance hooks for
>>>>> this case in the Xen arch code.  These would basically look the same
>>>>> as the low-level cache maintainance used by the DMA ops, but without
>>>>> going through the DMA mapping layer, in fact they should probably
>>>>> reuse the same low-level assembly routines.
>>>>>
>>>>> I don't think this is the first usage of such Xen buffer sharing, so
>>>>> what do the other users do?
>>>> I'll have to get even deeper into it. Initially I
>>>> looked at the code, but didn't find anything useful.
>>>> Or maybe I have just overlooked obvious things there
>>>  From Xen on Arm ABI:
>>>
>>> "All memory which is shared with other entities in the system
>>> (including the hypervisor and other guests) must reside in memory
>>> which is mapped as Normal Inner Write-Back Outer Write-Back
>>> Inner-Shareable.
>>> This applies to:
>>>    - hypercall arguments passed via a pointer to guest memory.
>>>    - memory shared via the grant table mechanism (including PV I/O
>>>  rings etc).
>>>    - memory shared with the hypervisor (struct shared_info, struct
>>>  vcpu_info, the grant table, etc).
>>> "
>>>
>>> So you should not need any cache maintenance here. Can you provide
>>> more details on the memory attribute you use for memory shared in both
>>> the backend and frontend?
>>>
>> It takes quite some time to collect this (because many components are
>> involved in the
>> use-case), but for now the pages in the guest domain are:
>> !PTE_RDONLY + PTE_PXN + PTE_SHARED + PTE_AF + PTE_UXN +
>> PTE_ATTRINDX(MT_NORMAL)
>
> So that's the attribute for the page mapped in the frontend, right? 
> How about the backend side?
Please see below
>
> Also, could that page be handed to the graphic card correctly?
Yes, if we use zero-copying. But please see below
> If so, is your graphic card coherent?
Yes, it is
>
> If one of your components is mapping with non-cacheable attributes 
> then you are already not following the Xen Arm ABI. In that case, we 
> would need to discuss how to extend the ABI.
>
> Cheers,
>
Well, I didn't get the attributes of pages at the backend side, but IMO 
those
do not matter in my use-case (for simplicity I am not using zero-copying at
backend side):

1. Frontend device allocates display buffer pages which come from shmem
and have these attributes:
!PTE_RDONLY + PTE_PXN + PTE_SHARED + PTE_AF + PTE_UXN + 
PTE_ATTRINDX(MT_NORMAL)

2. Frontend grants references to these pages and shares those with the 
backend

3. Backend is a user-space application (Weston client), so it uses 
gntdev kernel
driver to mmap the pages. The pages, which are used by gntdev, are those 
coming
from the Xen balloon driver and I believe they are all normal memory and
shouldn't be non-cached.

4. Once the frontend starts displaying it flips the buffers and backend 
does *memcpy*
from the frontend-backend shared buffer into Weston's buffer. This means
no HW at the backend side touches the shared buffer.

5. I can see distorted picture.

Previously I used setup with zero-copying, so then the picture becomes 
more complicated
in terms of buffers and how those used by the backed, but anyways it 
seems that the
very basic scenario with memory copying doesn't work for me.

Using DMA API on frontend's side does help - no artifacts are seen.
This is why I'm thinking that this is related to frontend/kernel side 
rather then to
the backend side. This is why I'm thinking this is related to caches and 
trying to figure
out what can be done here instead of using DMA API.

Thank you,
Olkesandr

Re: [Xen-devel] [PATCH v2] drm/xen-front: Make shmem backed display buffer coherent

2019-01-21 Thread Oleksandr Andrushchenko

On 1/18/19 1:43 PM, Julien Grall wrote:
> (+ Stefano)
>
> Hi,
>
> Sorry for jumping late in the conversation.
>
> On 18/01/2019 09:40, Oleksandr Andrushchenko wrote:
>> On 1/17/19 11:18 AM, Christoph Hellwig wrote:
>>> On Wed, Jan 16, 2019 at 06:43:29AM +, Oleksandr Andrushchenko 
>>> wrote:
>>>>> This whole issue keeps getting more and more confusing.
>>>> Well, I don't really do DMA here, but instead the buffers in
>>>> question are shared with other Xen domain, so effectively it
>>>> could be thought of some sort of DMA here, where the "device" is
>>>> that remote domain. If the buffers are not flushed then the
>>>> remote part sees some inconsistency which in my case results
>>>> in artifacts on screen while displaying the buffers.
>>>> When buffers are allocated via DMA API then there are no artifacts;
>>>> if buffers are allocated with shmem + DMA mapping then there are no
>>>> artifacts as well.
>>>> The only offending use-case is when I use shmem backed buffers,
>>>> but do not flush them
>>> The right answer would be to implement cache maintainance hooks for
>>> this case in the Xen arch code.  These would basically look the same
>>> as the low-level cache maintainance used by the DMA ops, but without
>>> going through the DMA mapping layer, in fact they should probably
>>> reuse the same low-level assembly routines.
>>>
>>> I don't think this is the first usage of such Xen buffer sharing, so
>>> what do the other users do?
>> I'll have to get even deeper into it. Initially I
>> looked at the code, but didn't find anything useful.
>> Or maybe I have just overlooked obvious things there
> From Xen on Arm ABI:
>
> "All memory which is shared with other entities in the system
> (including the hypervisor and other guests) must reside in memory
> which is mapped as Normal Inner Write-Back Outer Write-Back 
> Inner-Shareable.
> This applies to:
>   - hypercall arguments passed via a pointer to guest memory.
>   - memory shared via the grant table mechanism (including PV I/O
>     rings etc).
>   - memory shared with the hypervisor (struct shared_info, struct
>     vcpu_info, the grant table, etc).
> "
>
> So you should not need any cache maintenance here. Can you provide 
> more details on the memory attribute you use for memory shared in both 
> the backend and frontend?
>
It takes quite some time to collect this (because many components are 
involved in the
use-case), but for now the pages in the guest domain are:
!PTE_RDONLY + PTE_PXN + PTE_SHARED + PTE_AF + PTE_UXN + 
PTE_ATTRINDX(MT_NORMAL)

> Cheers,
>
>>
>> Thank you,
>> Oleksandr
>> ___
>> Xen-devel mailing list
>> xen-de...@lists.xenproject.org
>> https://lists.xenproject.org/mailman/listinfo/xen-devel
>>
>

Re: [PATCH v2] drm/xen-front: Make shmem backed display buffer coherent

2019-01-18 Thread Oleksandr Andrushchenko

On 1/17/19 11:18 AM, Christoph Hellwig wrote:
> On Wed, Jan 16, 2019 at 06:43:29AM +0000, Oleksandr Andrushchenko wrote:
>>> This whole issue keeps getting more and more confusing.
>> Well, I don't really do DMA here, but instead the buffers in
>> question are shared with other Xen domain, so effectively it
>> could be thought of some sort of DMA here, where the "device" is
>> that remote domain. If the buffers are not flushed then the
>> remote part sees some inconsistency which in my case results
>> in artifacts on screen while displaying the buffers.
>> When buffers are allocated via DMA API then there are no artifacts;
>> if buffers are allocated with shmem + DMA mapping then there are no
>> artifacts as well.
>> The only offending use-case is when I use shmem backed buffers,
>> but do not flush them
> The right answer would be to implement cache maintainance hooks for
> this case in the Xen arch code.  These would basically look the same
> as the low-level cache maintainance used by the DMA ops, but without
> going through the DMA mapping layer, in fact they should probably
> reuse the same low-level assembly routines.
>
> I don't think this is the first usage of such Xen buffer sharing, so
> what do the other users do?
I'll have to get even deeper into it. Initially I
looked at the code, but didn't find anything useful.
Or maybe I have just overlooked obvious things there

Thank you,
Oleksandr

Re: [PATCH v2] drm/xen-front: Make shmem backed display buffer coherent

2019-01-15 Thread Oleksandr Andrushchenko

On 1/16/19 8:36 AM, Christoph Hellwig wrote:
> On Wed, Jan 16, 2019 at 07:30:02AM +0100, Gerd Hoffmann wrote:
>>Hi,
>>
>>> +   if (!dma_map_sg(dev->dev, xen_obj->sgt->sgl, xen_obj->sgt->nents,
>>> +   DMA_BIDIRECTIONAL)) {
>>> +   ret = -EFAULT;
>>> +   goto fail_free_sgt;
>>> +   }
>> Hmm, so it seems the arm guys could not come up with a suggestion how to
>> solve that one in a better way.  Ok, lets go with this then.
>>
>> But didn't we agree that this deserves a comment exmplaining the purpose
>> of the dma_map_sg() call is to flush caches and that there is no actual
>> DMA happening here?
> Using a dma mapping call to flush caches is complete no-go.  But the
> real question is why you'd even want to flush cashes if you do not
> want a dma mapping?
>
> This whole issue keeps getting more and more confusing.
Well, I don't really do DMA here, but instead the buffers in
question are shared with other Xen domain, so effectively it
could be thought of some sort of DMA here, where the "device" is
that remote domain. If the buffers are not flushed then the
remote part sees some inconsistency which in my case results
in artifacts on screen while displaying the buffers.
When buffers are allocated via DMA API then there are no artifacts;
if buffers are allocated with shmem + DMA mapping then there are no
artifacts as well.
The only offending use-case is when I use shmem backed buffers,
but do not flush them

Re: [PATCH v2] drm/xen-front: Make shmem backed display buffer coherent

2019-01-15 Thread Oleksandr Andrushchenko

On 1/16/19 8:30 AM, Gerd Hoffmann wrote:
>Hi,
>
>> +if (!dma_map_sg(dev->dev, xen_obj->sgt->sgl, xen_obj->sgt->nents,
>> +DMA_BIDIRECTIONAL)) {
>> +ret = -EFAULT;
>> +goto fail_free_sgt;
>> +}
> Hmm, so it seems the arm guys could not come up with a suggestion how to
> solve that one in a better way.  Ok, lets go with this then.
>
> But didn't we agree that this deserves a comment exmplaining the purpose
> of the dma_map_sg() call is to flush caches and that there is no actual
> DMA happening here?
My bad, will add the comment
> cheers,
>Gerd
>
Thank you,
Oleksandr

[PATCH v2] drm/xen-front: Make shmem backed display buffer coherent

2019-01-15 Thread Oleksandr Andrushchenko

From: Oleksandr Andrushchenko 

When GEM backing storage is allocated with drm_gem_get_pages
the backing pages may be cached, thus making it possible that
the backend sees only partial content of the buffer which may
lead to screen artifacts. Make sure that the frontend's
memory is coherent and the backend always sees correct display
buffer content.

Fixes: c575b7eeb89f ("drm/xen-front: Add support for Xen PV display frontend")

Signed-off-by: Oleksandr Andrushchenko 

---
Changes since v1:
 - Remove GFP_USER|__GFP_DMA32 mapping flags (Gerd)
 - Use drm_prime_pages_to_sg directly (Noralf)

 drivers/gpu/drm/xen/xen_drm_front_gem.c | 38 ++---
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
b/drivers/gpu/drm/xen/xen_drm_front_gem.c
index 28bc501af450..0b0d9b4f97dc 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -32,8 +32,11 @@ struct xen_gem_object {
/* set for buffers allocated by the backend */
bool be_alloc;
 
-   /* this is for imported PRIME buffer */
-   struct sg_table *sgt_imported;
+   /*
+* this is for imported PRIME buffer or the one allocated via
+* drm_gem_get_pages.
+*/
+   struct sg_table *sgt;
 };
 
 static inline struct xen_gem_object *
@@ -124,8 +127,28 @@ static struct xen_gem_object *gem_create(struct drm_device 
*dev, size_t size)
goto fail;
}
 
+   xen_obj->sgt = drm_prime_pages_to_sg(xen_obj->pages,
+xen_obj->num_pages);
+   if (IS_ERR_OR_NULL(xen_obj->sgt)) {
+   ret = PTR_ERR(xen_obj->sgt);
+   xen_obj->sgt = NULL;
+   goto fail_put_pages;
+   }
+
+   if (!dma_map_sg(dev->dev, xen_obj->sgt->sgl, xen_obj->sgt->nents,
+   DMA_BIDIRECTIONAL)) {
+   ret = -EFAULT;
+   goto fail_free_sgt;
+   }
+
return xen_obj;
 
+fail_free_sgt:
+   sg_free_table(xen_obj->sgt);
+   xen_obj->sgt = NULL;
+fail_put_pages:
+   drm_gem_put_pages(_obj->base, xen_obj->pages, true, false);
+   xen_obj->pages = NULL;
 fail:
DRM_ERROR("Failed to allocate buffer with size %zu\n", size);
return ERR_PTR(ret);
@@ -148,7 +171,7 @@ void xen_drm_front_gem_free_object_unlocked(struct 
drm_gem_object *gem_obj)
struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj);
 
if (xen_obj->base.import_attach) {
-   drm_prime_gem_destroy(_obj->base, xen_obj->sgt_imported);
+   drm_prime_gem_destroy(_obj->base, xen_obj->sgt);
gem_free_pages_array(xen_obj);
} else {
if (xen_obj->pages) {
@@ -157,6 +180,13 @@ void xen_drm_front_gem_free_object_unlocked(struct 
drm_gem_object *gem_obj)
xen_obj->pages);
gem_free_pages_array(xen_obj);
} else {
+   if (xen_obj->sgt) {
+   dma_unmap_sg(xen_obj->base.dev->dev,
+xen_obj->sgt->sgl,
+xen_obj->sgt->nents,
+DMA_BIDIRECTIONAL);
+   sg_free_table(xen_obj->sgt);
+   }
drm_gem_put_pages(_obj->base,
  xen_obj->pages, true, false);
}
@@ -202,7 +232,7 @@ xen_drm_front_gem_import_sg_table(struct drm_device *dev,
if (ret < 0)
return ERR_PTR(ret);
 
-   xen_obj->sgt_imported = sgt;
+   xen_obj->sgt = sgt;
 
ret = drm_prime_sg_to_page_addr_arrays(sgt, xen_obj->pages,
   NULL, xen_obj->num_pages);
-- 
2.20.1

Re: [Xen-devel] [PATCH] arch/arm/xen: Remove duplicate header

2019-01-14 Thread Oleksandr Andrushchenko


On 1/7/19 7:37 PM, Souptick Joarder wrote:

Remove duplicate header which is included twice.

Signed-off-by: Souptick Joarder 

Reviewed-by: Oleksandr Andrushchenko 

---
  arch/arm/xen/mm.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index cb44aa2..e1d44b9 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -7,7 +7,6 @@
  #include 
  #include 
  #include 
-#include 
  #include 
  #include

Re: [PATCH 5/9] drm/xen/xen_drm_front_gem.c: Convert to use vm_insert_range

2019-01-11 Thread Oleksandr Andrushchenko


On 1/11/19 5:10 PM, Souptick Joarder wrote:

Convert to use vm_insert_range() to map range of kernel
memory to user vma.

Signed-off-by: Souptick Joarder 

Reviewed-by: Oleksandr Andrushchenko 

---
  drivers/gpu/drm/xen/xen_drm_front_gem.c | 18 +-
  1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
b/drivers/gpu/drm/xen/xen_drm_front_gem.c
index 47ff019..9990c2f 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -225,8 +225,7 @@ struct drm_gem_object *
  static int gem_mmap_obj(struct xen_gem_object *xen_obj,
struct vm_area_struct *vma)
  {
-   unsigned long addr = vma->vm_start;
-   int i;
+   int ret;
  
  	/*

 * clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
@@ -247,18 +246,11 @@ static int gem_mmap_obj(struct xen_gem_object *xen_obj,
 * FIXME: as we insert all the pages now then no .fault handler must
 * be called, so don't provide one
 */
-   for (i = 0; i < xen_obj->num_pages; i++) {
-   int ret;
-
-   ret = vm_insert_page(vma, addr, xen_obj->pages[i]);
-   if (ret < 0) {
-   DRM_ERROR("Failed to insert pages into vma: %d\n", ret);
-   return ret;
-   }
+   ret = vm_insert_range(vma, xen_obj->pages, xen_obj->num_pages);
+   if (ret < 0)
+   DRM_ERROR("Failed to insert pages into vma: %d\n", ret);
  
-		addr += PAGE_SIZE;

-   }
-   return 0;
+   return ret;
  }
  
  int xen_drm_front_gem_mmap(struct file *filp, struct vm_area_struct *vma)

Re: [PATCH v4 08/16] drm/bochs: atomic: use suspend/resume helpers

2019-01-11 Thread Oleksandr Andrushchenko


On 1/11/19 7:37 AM, Gerd Hoffmann wrote:

Switch to atomic helpers: drm_mode_config_helper_suspend/resume().

Signed-off-by: Gerd Hoffmann 

Reviewed-by: Oleksandr Andrushchenko 

---
  drivers/gpu/drm/bochs/bochs_drv.c | 11 ++-
  1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs_drv.c 
b/drivers/gpu/drm/bochs/bochs_drv.c
index f3dd66ae99..08ba6029d2 100644
--- a/drivers/gpu/drm/bochs/bochs_drv.c
+++ b/drivers/gpu/drm/bochs/bochs_drv.c
@@ -103,11 +103,8 @@ static int bochs_pm_suspend(struct device *dev)
struct drm_device *drm_dev = pci_get_drvdata(pdev);
struct bochs_device *bochs = drm_dev->dev_private;
  
-	drm_kms_helper_poll_disable(drm_dev);

-
drm_fb_helper_set_suspend_unlocked(>fb.helper, 1);
-
-   return 0;
+   return drm_mode_config_helper_suspend(drm_dev);
  }
  
  static int bochs_pm_resume(struct device *dev)

@@ -116,12 +113,8 @@ static int bochs_pm_resume(struct device *dev)
struct drm_device *drm_dev = pci_get_drvdata(pdev);
struct bochs_device *bochs = drm_dev->dev_private;
  
-	drm_helper_resume_force_mode(drm_dev);

-
drm_fb_helper_set_suspend_unlocked(>fb.helper, 0);
-
-   drm_kms_helper_poll_enable(drm_dev);
-   return 0;
+   return drm_mode_config_helper_resume(drm_dev);
  }
  #endif

Re: [PATCH] drm/virtio: drop prime import/export callbacks

2019-01-11 Thread Oleksandr Andrushchenko


On 1/10/19 1:15 PM, Gerd Hoffmann wrote:

Also set prime_handle_to_fd and prime_fd_to_handle to NULL,
so drm will not advertive DRM_PRIME_CAP_{IMPORT,EXPORT} to
userspace.

Signed-off-by: Gerd Hoffmann 

Reviewed-by: Oleksandr Andrushchenko 

---
  drivers/gpu/drm/virtio/virtgpu_drv.h   |  4 
  drivers/gpu/drm/virtio/virtgpu_drv.c   |  4 
  drivers/gpu/drm/virtio/virtgpu_prime.c | 14 --
  3 files changed, 22 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h 
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index cf896d8793..4f2f3c43a4 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -354,10 +354,6 @@ int virtio_gpu_object_wait(struct virtio_gpu_object *bo, 
bool no_wait);
  /* virtgpu_prime.c */
  int virtgpu_gem_prime_pin(struct drm_gem_object *obj);
  void virtgpu_gem_prime_unpin(struct drm_gem_object *obj);
-struct sg_table *virtgpu_gem_prime_get_sg_table(struct drm_gem_object *obj);
-struct drm_gem_object *virtgpu_gem_prime_import_sg_table(
-   struct drm_device *dev, struct dma_buf_attachment *attach,
-   struct sg_table *sgt);
  void *virtgpu_gem_prime_vmap(struct drm_gem_object *obj);
  void virtgpu_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr);
  int virtgpu_gem_prime_mmap(struct drm_gem_object *obj,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c 
b/drivers/gpu/drm/virtio/virtgpu_drv.c
index af92964b68..b996ac1d4f 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -205,14 +205,10 @@ static struct drm_driver driver = {
  #if defined(CONFIG_DEBUG_FS)
.debugfs_init = virtio_gpu_debugfs_init,
  #endif
-   .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
-   .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
.gem_prime_export = drm_gem_prime_export,
.gem_prime_import = drm_gem_prime_import,
.gem_prime_pin = virtgpu_gem_prime_pin,
.gem_prime_unpin = virtgpu_gem_prime_unpin,
-   .gem_prime_get_sg_table = virtgpu_gem_prime_get_sg_table,
-   .gem_prime_import_sg_table = virtgpu_gem_prime_import_sg_table,
.gem_prime_vmap = virtgpu_gem_prime_vmap,
.gem_prime_vunmap = virtgpu_gem_prime_vunmap,
.gem_prime_mmap = virtgpu_gem_prime_mmap,
diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c 
b/drivers/gpu/drm/virtio/virtgpu_prime.c
index 86ce0ae93f..c59ec34c80 100644
--- a/drivers/gpu/drm/virtio/virtgpu_prime.c
+++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
@@ -39,20 +39,6 @@ void virtgpu_gem_prime_unpin(struct drm_gem_object *obj)
WARN_ONCE(1, "not implemented");
  }
  
-struct sg_table *virtgpu_gem_prime_get_sg_table(struct drm_gem_object *obj)

-{
-   WARN_ONCE(1, "not implemented");
-   return ERR_PTR(-ENODEV);
-}
-
-struct drm_gem_object *virtgpu_gem_prime_import_sg_table(
-   struct drm_device *dev, struct dma_buf_attachment *attach,
-   struct sg_table *table)
-{
-   WARN_ONCE(1, "not implemented");
-   return ERR_PTR(-ENODEV);
-}
-
  void *virtgpu_gem_prime_vmap(struct drm_gem_object *obj)
  {
struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);

Re: [PATCH 01/10] drm/virtio: log error responses

2018-12-21 Thread Oleksandr Andrushchenko


On 12/19/18 2:26 PM, Gerd Hoffmann wrote:

If we got an error response code from the host, print it to the log.

Signed-off-by: Gerd Hoffmann 

Reviewed-by: Oleksandr Andrushchenko 

---
  drivers/gpu/drm/virtio/virtgpu_vq.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c 
b/drivers/gpu/drm/virtio/virtgpu_vq.c
index e27c4aedb8..6bc2008b0d 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -192,8 +192,16 @@ void virtio_gpu_dequeue_ctrl_func(struct work_struct *work)
  
  	list_for_each_entry_safe(entry, tmp, _list, list) {

resp = (struct virtio_gpu_ctrl_hdr *)entry->resp_buf;
-   if (resp->type != cpu_to_le32(VIRTIO_GPU_RESP_OK_NODATA))
-   DRM_DEBUG("response 0x%x\n", le32_to_cpu(resp->type));
+   if (resp->type != cpu_to_le32(VIRTIO_GPU_RESP_OK_NODATA)) {
+   if (resp->type >= 
cpu_to_le32(VIRTIO_GPU_RESP_ERR_UNSPEC)) {
+   struct virtio_gpu_ctrl_hdr *cmd;
+   cmd = (struct virtio_gpu_ctrl_hdr *)entry->buf;
+   DRM_ERROR("response 0x%x (command 0x%x)\n",
+ le32_to_cpu(resp->type),
+ le32_to_cpu(cmd->type));
+   } else
+   DRM_DEBUG("response 0x%x\n", 
le32_to_cpu(resp->type));
+   }
if (resp->flags & cpu_to_le32(VIRTIO_GPU_FLAG_FENCE)) {
u64 f = le64_to_cpu(resp->fence_id);

Re: [PATCH] drm/xen-front: Make shmem backed display buffer coherent

2018-12-21 Thread Oleksandr Andrushchenko


On 12/20/18 8:38 PM, Christoph Hellwig wrote:

On Thu, Dec 20, 2018 at 07:35:15PM +0100, Daniel Vetter wrote:

Err, with streaming DMA buffer sharing is trivial.  The coherent DMA
allocator is what causes all kinds of horrible hacks that can't actually
work on various platforms.

Hm, I thought the streaming dma api is the one that causes bounce
buffers and all that fun. If you're unlucky at least.

Yes it may.  But even if that happens everything will actually work,
just slower.  While the dma coherent API is simply broken.

But if you don't want bounce buffering you need to use the dma
noncoherent allocator as proposed here:


https://lists.linuxfoundation.org/pipermail/iommu/2018-December/031982.html

which combines allocating memory that doesn't need to be bounce
buffered with a sharing scheme that can actually work.

So, the bottom line will be: I can use DMA API for what I need, but:
1. I need to remove GFP_USER
2. No need for DMA32 (so no chance for bouncing to step in)
3. I may need to check if mapping and unmapping of the buffer
at once will also help, e.g. no need to have the buffer mapped until
it is destroyed
Did I get it all right?

Thank you,
Oleksandr

Re: [PATCH] drm/xen-front: Make shmem backed display buffer coherent


On 12/20/18 5:36 PM, Christoph Hellwig wrote:

On Tue, Dec 18, 2018 at 08:20:22PM +0100, Noralf Trønnes wrote:

+   if (!dma_map_sg(dev->dev, xen_obj->sgt->sgl, xen_obj->sgt->nents,
+   DMA_BIDIRECTIONAL)) {


Are you using the DMA streaming API as a way to flush the caches?

This looks rather broken.  Please send the whole patch series to
the iommu list for proper review.

This is the only patch [1], no series. And at the moment I think
there is nothing to review as I am not sure how to deal with those
shmem pages: this patch is rather to start a discussion on how shmem
pages can be flushed on ARM (the only workaround I have so far is
in this patch which uses DMA API). This is where I am looking for
some advice, so I can implement the patch the right way.

Does this mean that GFP_USER isn't making the buffer coherent?

How could GFP_USER make memory coherent in any way?

I am no way an expert here, but other DRM drivers allocate buffers
from shmem and then use DMA API [2], for example [3]

[1] https://patchwork.kernel.org/patch/10700089/
[2] https://elixir.bootlin.com/linux/v4.20-rc7/ident/drm_gem_get_pages
[3] 
https://elixir.bootlin.com/linux/v4.20-rc7/source/drivers/gpu/drm/omapdrm/omap_gem.c#L248

Re: [PATCH 14/14] drm/bochs: move remaining fb bits to kms


On 12/19/18 1:51 PM, Gerd Hoffmann wrote:

bochs_fbdev.c is almost empty now.  Move the remaining framebuffer bits
over to bochs_kms.c.  Pure code motion. No functional change.

Signed-off-by: Gerd Hoffmann 

Reviewed-by: Oleksandr Andrushchenko 

---
  drivers/gpu/drm/bochs/bochs_fbdev.c | 29 -
  drivers/gpu/drm/bochs/bochs_kms.c   | 17 +
  drivers/gpu/drm/bochs/Makefile  |  2 +-
  3 files changed, 18 insertions(+), 30 deletions(-)
  delete mode 100644 drivers/gpu/drm/bochs/bochs_fbdev.c

diff --git a/drivers/gpu/drm/bochs/bochs_fbdev.c 
b/drivers/gpu/drm/bochs/bochs_fbdev.c
deleted file mode 100644
index 7cac3f5253..00
--- a/drivers/gpu/drm/bochs/bochs_fbdev.c
+++ /dev/null
@@ -1,29 +0,0 @@
-/*
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- */
-
-#include "bochs.h"
-#include 
-#include 
-
-/* -- */
-
-static struct drm_framebuffer *
-bochs_gem_fb_create(struct drm_device *dev, struct drm_file *file,
-   const struct drm_mode_fb_cmd2 *mode_cmd)
-{
-   if (mode_cmd->pixel_format != DRM_FORMAT_XRGB &&
-   mode_cmd->pixel_format != DRM_FORMAT_BGRX)
-   return ERR_PTR(-EINVAL);
-
-   return drm_gem_fb_create(dev, file, mode_cmd);
-}
-
-const struct drm_mode_config_funcs bochs_mode_funcs = {
-   .fb_create = bochs_gem_fb_create,
-   .atomic_check = drm_atomic_helper_check,
-   .atomic_commit = drm_atomic_helper_commit,
-};
diff --git a/drivers/gpu/drm/bochs/bochs_kms.c 
b/drivers/gpu/drm/bochs/bochs_kms.c
index a1a0129f3e..3688d0b616 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -9,6 +9,7 @@
  #include 
  #include 
  #include 
+#include 
  
  static int defx = 1024;

  static int defy = 768;
@@ -256,6 +257,22 @@ static void bochs_connector_init(struct drm_device *dev)
}
  }
  
+static struct drm_framebuffer *

+bochs_gem_fb_create(struct drm_device *dev, struct drm_file *file,
+   const struct drm_mode_fb_cmd2 *mode_cmd)
+{
+   if (mode_cmd->pixel_format != DRM_FORMAT_XRGB &&
+   mode_cmd->pixel_format != DRM_FORMAT_BGRX)
+   return ERR_PTR(-EINVAL);
+
+   return drm_gem_fb_create(dev, file, mode_cmd);
+}
+
+const struct drm_mode_config_funcs bochs_mode_funcs = {
+   .fb_create = bochs_gem_fb_create,
+   .atomic_check = drm_atomic_helper_check,
+   .atomic_commit = drm_atomic_helper_commit,
+};
  
  int bochs_kms_init(struct bochs_device *bochs)

  {
diff --git a/drivers/gpu/drm/bochs/Makefile b/drivers/gpu/drm/bochs/Makefile
index 98ef60a19e..e9e0f8f5eb 100644
--- a/drivers/gpu/drm/bochs/Makefile
+++ b/drivers/gpu/drm/bochs/Makefile
@@ -1,3 +1,3 @@
-bochs-drm-y := bochs_drv.o bochs_mm.o bochs_kms.o bochs_fbdev.o bochs_hw.o
+bochs-drm-y := bochs_drv.o bochs_mm.o bochs_kms.o bochs_hw.o
  
  obj-$(CONFIG_DRM_BOCHS)	+= bochs-drm.o

Re: [PATCH 13/14] drm/bochs: drop old fbdev emulation code


On 12/19/18 1:51 PM, Gerd Hoffmann wrote:

Not needed any more, bochs uses the generic emulation now.

Signed-off-by: Gerd Hoffmann 

Reviewed-by: Oleksandr Andrushchenko 

---
  drivers/gpu/drm/bochs/bochs.h   |   9 ---
  drivers/gpu/drm/bochs/bochs_drv.c   |   6 --
  drivers/gpu/drm/bochs/bochs_fbdev.c | 137 
  3 files changed, 152 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
index 4236d5d811..42a587e71e 100644
--- a/drivers/gpu/drm/bochs/bochs.h
+++ b/drivers/gpu/drm/bochs/bochs.h
@@ -80,12 +80,6 @@ struct bochs_device {
struct ttm_bo_device bdev;
bool initialized;
} ttm;
-
-   /* fbdev */
-   struct {
-   struct drm_framebuffer *fb;
-   struct drm_fb_helper helper;
-   } fb;
  };
  
  struct bochs_bo {

@@ -161,7 +155,4 @@ int bochs_kms_init(struct bochs_device *bochs);
  void bochs_kms_fini(struct bochs_device *bochs);
  
  /* bochs_fbdev.c */

-int bochs_fbdev_init(struct bochs_device *bochs);
-void bochs_fbdev_fini(struct bochs_device *bochs);
-
  extern const struct drm_mode_config_funcs bochs_mode_funcs;
diff --git a/drivers/gpu/drm/bochs/bochs_drv.c 
b/drivers/gpu/drm/bochs/bochs_drv.c
index f1f65324bb..ad1290ca7b 100644
--- a/drivers/gpu/drm/bochs/bochs_drv.c
+++ b/drivers/gpu/drm/bochs/bochs_drv.c
@@ -110,12 +110,9 @@ static int bochs_pm_suspend(struct device *dev)
  {
struct pci_dev *pdev = to_pci_dev(dev);
struct drm_device *drm_dev = pci_get_drvdata(pdev);
-   struct bochs_device *bochs = drm_dev->dev_private;
  
  	drm_kms_helper_poll_disable(drm_dev);
  
-	drm_fb_helper_set_suspend_unlocked(>fb.helper, 1);

-
return 0;
  }
  
@@ -123,12 +120,9 @@ static int bochs_pm_resume(struct device *dev)

  {
struct pci_dev *pdev = to_pci_dev(dev);
struct drm_device *drm_dev = pci_get_drvdata(pdev);
-   struct bochs_device *bochs = drm_dev->dev_private;
  
  	drm_helper_resume_force_mode(drm_dev);
  
-	drm_fb_helper_set_suspend_unlocked(>fb.helper, 0);

-
drm_kms_helper_poll_enable(drm_dev);
return 0;
  }
diff --git a/drivers/gpu/drm/bochs/bochs_fbdev.c 
b/drivers/gpu/drm/bochs/bochs_fbdev.c
index 92feb817ff..7cac3f5253 100644
--- a/drivers/gpu/drm/bochs/bochs_fbdev.c
+++ b/drivers/gpu/drm/bochs/bochs_fbdev.c
@@ -11,132 +11,6 @@
  
  /* -- */
  
-static int bochsfb_mmap(struct fb_info *info,

-   struct vm_area_struct *vma)
-{
-   struct drm_fb_helper *fb_helper = info->par;
-   struct bochs_bo *bo = gem_to_bochs_bo(fb_helper->fb->obj[0]);
-
-   return ttm_fbdev_mmap(vma, >bo);
-}
-
-static struct fb_ops bochsfb_ops = {
-   .owner = THIS_MODULE,
-   DRM_FB_HELPER_DEFAULT_OPS,
-   .fb_fillrect = drm_fb_helper_cfb_fillrect,
-   .fb_copyarea = drm_fb_helper_cfb_copyarea,
-   .fb_imageblit = drm_fb_helper_cfb_imageblit,
-   .fb_mmap = bochsfb_mmap,
-};
-
-static int bochsfb_create_object(struct bochs_device *bochs,
-const struct drm_mode_fb_cmd2 *mode_cmd,
-struct drm_gem_object **gobj_p)
-{
-   struct drm_device *dev = bochs->dev;
-   struct drm_gem_object *gobj;
-   u32 size;
-   int ret = 0;
-
-   size = mode_cmd->pitches[0] * mode_cmd->height;
-   ret = bochs_gem_create(dev, size, true, );
-   if (ret)
-   return ret;
-
-   *gobj_p = gobj;
-   return ret;
-}
-
-static int bochsfb_create(struct drm_fb_helper *helper,
- struct drm_fb_helper_surface_size *sizes)
-{
-   struct bochs_device *bochs =
-   container_of(helper, struct bochs_device, fb.helper);
-   struct fb_info *info;
-   struct drm_framebuffer *fb;
-   struct drm_mode_fb_cmd2 mode_cmd;
-   struct drm_gem_object *gobj = NULL;
-   struct bochs_bo *bo = NULL;
-   int size, ret;
-
-   if (sizes->surface_bpp != 32)
-   return -EINVAL;
-
-   mode_cmd.width = sizes->surface_width;
-   mode_cmd.height = sizes->surface_height;
-   mode_cmd.pitches[0] = sizes->surface_width * 4;
-   mode_cmd.pixel_format = DRM_FORMAT_HOST_XRGB;
-   size = mode_cmd.pitches[0] * mode_cmd.height;
-
-   /* alloc, pin & map bo */
-   ret = bochsfb_create_object(bochs, _cmd, );
-   if (ret) {
-   DRM_ERROR("failed to create fbcon backing object %d\n", ret);
-   return ret;
-   }
-
-   bo = gem_to_bochs_bo(gobj);
-
-   ret = ttm_bo_reserve(>bo, true, false, NULL);
-   if (ret)
-   return ret;
-
-   ret = bochs_bo_pin(bo, TTM_PL_FLAG_VRAM);
-   if (ret) {
-   DRM_ERROR("failed to pin fbcon\n");
-   ttm_bo_unreserve(>bo);
-

Re: [PATCH 12/14] drm/bochs: switch to generic drm fbdev emulation


On 12/19/18 1:51 PM, Gerd Hoffmann wrote:

Signed-off-by: Gerd Hoffmann 
---
  drivers/gpu/drm/bochs/bochs_drv.c | 5 +
  1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs_drv.c 
b/drivers/gpu/drm/bochs/bochs_drv.c
index a9c7140e3b..f1f65324bb 100644
--- a/drivers/gpu/drm/bochs/bochs_drv.c
+++ b/drivers/gpu/drm/bochs/bochs_drv.c
@@ -27,7 +27,6 @@ static void bochs_unload(struct drm_device *dev)
  {
struct bochs_device *bochs = dev->dev_private;
  
-	bochs_fbdev_fini(bochs);

bochs_kms_fini(bochs);
bochs_mm_fini(bochs);
bochs_hw_fini(dev);
@@ -58,9 +57,6 @@ static int bochs_load(struct drm_device *dev)
if (ret)
goto err;
  
-	if (enable_fbdev)

-   bochs_fbdev_init(bochs);
-

I think that after this change you don't need
"module_param_named(fbdev, enable_fbdev, bool, 0444);"

return 0;
  
  err:

@@ -178,6 +174,7 @@ static int bochs_pci_probe(struct pci_dev *pdev,
if (ret)
goto err_unload;
  
+	drm_fbdev_generic_setup(dev, 32);

return ret;
  
  err_unload:

Re: [PATCH 11/14] drm/bochs: add basic prime support

bj,
+struct vm_area_struct *vma)
+{
+   struct bochs_bo *bo = gem_to_bochs_bo(obj);
+
+   return ttm_fbdev_mmap(vma, >bo);
+}

With the above fixed:
Reviewed-by: Oleksandr Andrushchenko

Re: [PATCH 10/14] drm/bochs: drop unused gpu_addr arg from bochs_bo_pin()


On 12/19/18 1:51 PM, Gerd Hoffmann wrote:

It's always NULL, so just remove it.

Signed-off-by: Gerd Hoffmann 

Reviewed-by: Oleksandr Andrushchenko 

---
  drivers/gpu/drm/bochs/bochs.h   |  2 +-
  drivers/gpu/drm/bochs/bochs_fbdev.c |  2 +-
  drivers/gpu/drm/bochs/bochs_kms.c   |  2 +-
  drivers/gpu/drm/bochs/bochs_mm.c| 11 +--
  4 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
index 4dc1b6384e..d0d474e06f 100644
--- a/drivers/gpu/drm/bochs/bochs.h
+++ b/drivers/gpu/drm/bochs/bochs.h
@@ -142,7 +142,7 @@ int bochs_dumb_create(struct drm_file *file, struct 
drm_device *dev,
  int bochs_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev,
   uint32_t handle, uint64_t *offset);
  
-int bochs_bo_pin(struct bochs_bo *bo, u32 pl_flag, u64 *gpu_addr);

+int bochs_bo_pin(struct bochs_bo *bo, u32 pl_flag);
  int bochs_bo_unpin(struct bochs_bo *bo);
  
  /* bochs_kms.c */

diff --git a/drivers/gpu/drm/bochs/bochs_fbdev.c 
b/drivers/gpu/drm/bochs/bochs_fbdev.c
index d9f3d42999..92feb817ff 100644
--- a/drivers/gpu/drm/bochs/bochs_fbdev.c
+++ b/drivers/gpu/drm/bochs/bochs_fbdev.c
@@ -81,7 +81,7 @@ static int bochsfb_create(struct drm_fb_helper *helper,
if (ret)
return ret;
  
-	ret = bochs_bo_pin(bo, TTM_PL_FLAG_VRAM, NULL);

+   ret = bochs_bo_pin(bo, TTM_PL_FLAG_VRAM);
if (ret) {
DRM_ERROR("failed to pin fbcon\n");
ttm_bo_unreserve(>bo);
diff --git a/drivers/gpu/drm/bochs/bochs_kms.c 
b/drivers/gpu/drm/bochs/bochs_kms.c
index 85dd268fa1..a1a0129f3e 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -96,7 +96,7 @@ static int bochs_plane_prepare_fb(struct drm_plane *plane,
if (!new_state->fb)
return 0;
bo = gem_to_bochs_bo(new_state->fb->obj[0]);
-   return bochs_bo_pin(bo, TTM_PL_FLAG_VRAM, NULL);
+   return bochs_bo_pin(bo, TTM_PL_FLAG_VRAM);
  }
  
  static void bochs_plane_cleanup_fb(struct drm_plane *plane,

diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c
index 0980411e41..5a0e092847 100644
--- a/drivers/gpu/drm/bochs/bochs_mm.c
+++ b/drivers/gpu/drm/bochs/bochs_mm.c
@@ -210,20 +210,13 @@ static void bochs_ttm_placement(struct bochs_bo *bo, int 
domain)
bo->placement.num_busy_placement = c;
  }
  
-static inline u64 bochs_bo_gpu_offset(struct bochs_bo *bo)

-{
-   return bo->bo.offset;
-}
-
-int bochs_bo_pin(struct bochs_bo *bo, u32 pl_flag, u64 *gpu_addr)
+int bochs_bo_pin(struct bochs_bo *bo, u32 pl_flag)
  {
struct ttm_operation_ctx ctx = { false, false };
int i, ret;
  
  	if (bo->pin_count) {

bo->pin_count++;
-   if (gpu_addr)
-   *gpu_addr = bochs_bo_gpu_offset(bo);
return 0;
}
  
@@ -235,8 +228,6 @@ int bochs_bo_pin(struct bochs_bo *bo, u32 pl_flag, u64 *gpu_addr)

return ret;
  
  	bo->pin_count = 1;

-   if (gpu_addr)
-   *gpu_addr = bochs_bo_gpu_offset(bo);
return 0;
  }

Re: [PATCH 09/14] drm/bochs: remove old bochs_crtc_* functions


On 12/19/18 1:51 PM, Gerd Hoffmann wrote:

Remove the old, now unused crtc callbacks.

Signed-off-by: Gerd Hoffmann 

Reviewed-by: Oleksandr Andrushchenko 

---
  drivers/gpu/drm/bochs/bochs_kms.c | 81 ---
  1 file changed, 81 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs_kms.c 
b/drivers/gpu/drm/bochs/bochs_kms.c
index 9e836386e9..85dd268fa1 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -20,74 +20,6 @@ MODULE_PARM_DESC(defy, "default y resolution");
  
  /* -- */
  
-static void bochs_crtc_dpms(struct drm_crtc *crtc, int mode)

-{
-   switch (mode) {
-   case DRM_MODE_DPMS_ON:
-   case DRM_MODE_DPMS_STANDBY:
-   case DRM_MODE_DPMS_SUSPEND:
-   case DRM_MODE_DPMS_OFF:
-   default:
-   return;
-   }
-}
-
-static int bochs_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
-   struct drm_framebuffer *old_fb)
-{
-   struct bochs_device *bochs =
-   container_of(crtc, struct bochs_device, crtc);
-   struct bochs_bo *bo;
-   u64 gpu_addr = 0;
-   int ret;
-
-   if (old_fb) {
-   bo = gem_to_bochs_bo(old_fb->obj[0]);
-   ret = ttm_bo_reserve(>bo, true, false, NULL);
-   if (ret) {
-   DRM_ERROR("failed to reserve old_fb bo\n");
-   } else {
-   bochs_bo_unpin(bo);
-   ttm_bo_unreserve(>bo);
-   }
-   }
-
-   if (WARN_ON(crtc->primary->fb == NULL))
-   return -EINVAL;
-
-   bo = gem_to_bochs_bo(crtc->primary->fb->obj[0]);
-   ret = ttm_bo_reserve(>bo, true, false, NULL);
-   if (ret)
-   return ret;
-
-   ret = bochs_bo_pin(bo, TTM_PL_FLAG_VRAM, _addr);
-   if (ret) {
-   ttm_bo_unreserve(>bo);
-   return ret;
-   }
-
-   ttm_bo_unreserve(>bo);
-   bochs_hw_setbase(bochs, x, y, gpu_addr);
-   return 0;
-}
-
-static int bochs_crtc_mode_set(struct drm_crtc *crtc,
-  struct drm_display_mode *mode,
-  struct drm_display_mode *adjusted_mode,
-  int x, int y, struct drm_framebuffer *old_fb)
-{
-   struct bochs_device *bochs =
-   container_of(crtc, struct bochs_device, crtc);
-
-   if (WARN_ON(crtc->primary->fb == NULL))
-   return -EINVAL;
-
-   bochs_hw_setmode(bochs, mode);
-   bochs_hw_setformat(bochs, crtc->primary->fb->format);
-   bochs_crtc_mode_set_base(crtc, x, y, old_fb);
-   return 0;
-}
-
  static void bochs_crtc_mode_set_nofb(struct drm_crtc *crtc)
  {
struct bochs_device *bochs =
@@ -96,14 +28,6 @@ static void bochs_crtc_mode_set_nofb(struct drm_crtc *crtc)
bochs_hw_setmode(bochs, >mode);
  }
  
-static void bochs_crtc_prepare(struct drm_crtc *crtc)

-{
-}
-
-static void bochs_crtc_commit(struct drm_crtc *crtc)
-{
-}
-
  static void bochs_crtc_atomic_enable(struct drm_crtc *crtc,
 struct drm_crtc_state *old_crtc_state)
  {
@@ -138,12 +62,7 @@ static const struct drm_crtc_funcs bochs_crtc_funcs = {
  };
  
  static const struct drm_crtc_helper_funcs bochs_helper_funcs = {

-   .dpms = bochs_crtc_dpms,
-   .mode_set = bochs_crtc_mode_set,
-   .mode_set_base = bochs_crtc_mode_set_base,
.mode_set_nofb = bochs_crtc_mode_set_nofb,
-   .prepare = bochs_crtc_prepare,
-   .commit = bochs_crtc_commit,
.atomic_enable = bochs_crtc_atomic_enable,
.atomic_flush = bochs_crtc_atomic_flush,
  };

Re: [PATCH 08/14] drm/bochs: atomic: set DRIVER_ATOMIC


On 12/19/18 1:51 PM, Gerd Hoffmann wrote:

Conversion to atomic modesetting, final step.
Set the DRIVER_ATOMIC flag.

Signed-off-by: Gerd Hoffmann 

Reviewed-by: Oleksandr Andrushchenko 

---
  drivers/gpu/drm/bochs/bochs_drv.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bochs/bochs_drv.c 
b/drivers/gpu/drm/bochs/bochs_drv.c
index f3dd66ae99..278f9d2e7f 100644
--- a/drivers/gpu/drm/bochs/bochs_drv.c
+++ b/drivers/gpu/drm/bochs/bochs_drv.c
@@ -81,7 +81,7 @@ static const struct file_operations bochs_fops = {
  };
  
  static struct drm_driver bochs_driver = {

-   .driver_features= DRIVER_GEM | DRIVER_MODESET,
+   .driver_features= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
.fops   = _fops,
.name   = "bochs-drm",
.desc   = "bochs dispi vga interface (qemu stdvga)",

Re: [PATCH 07/14] drm/bochs: atomic: use atomic page_flip helper


On 12/19/18 1:51 PM, Gerd Hoffmann wrote:

Conversion to atomic modesetting, step five.
Use atomic page_flip helper for crtc.

Signed-off-by: Gerd Hoffmann 

Reviewed-by: Oleksandr Andrushchenko 

---
  drivers/gpu/drm/bochs/bochs_kms.c | 23 +--
  1 file changed, 1 insertion(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs_kms.c 
b/drivers/gpu/drm/bochs/bochs_kms.c
index dcc8b864fc..9e836386e9 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -104,27 +104,6 @@ static void bochs_crtc_commit(struct drm_crtc *crtc)
  {
  }
  
-static int bochs_crtc_page_flip(struct drm_crtc *crtc,

-   struct drm_framebuffer *fb,
-   struct drm_pending_vblank_event *event,
-   uint32_t page_flip_flags,
-   struct drm_modeset_acquire_ctx *ctx)
-{
-   struct bochs_device *bochs =
-   container_of(crtc, struct bochs_device, crtc);
-   struct drm_framebuffer *old_fb = crtc->primary->fb;
-   unsigned long irqflags;
-
-   drm_atomic_set_fb_for_plane(crtc->primary->state, fb);
-   bochs_crtc_mode_set_base(crtc, 0, 0, old_fb);
-   if (event) {
-   spin_lock_irqsave(>dev->event_lock, irqflags);
-   drm_crtc_send_vblank_event(crtc, event);
-   spin_unlock_irqrestore(>dev->event_lock, irqflags);
-   }
-   return 0;
-}
-
  static void bochs_crtc_atomic_enable(struct drm_crtc *crtc,
 struct drm_crtc_state *old_crtc_state)
  {
@@ -152,7 +131,7 @@ static void bochs_crtc_atomic_flush(struct drm_crtc *crtc,
  static const struct drm_crtc_funcs bochs_crtc_funcs = {
.set_config = drm_atomic_helper_set_config,
.destroy = drm_crtc_cleanup,
-   .page_flip = bochs_crtc_page_flip,
+   .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,

Re: [PATCH 05/14] drm/bochs: atomic: switch planes to atomic, wire up helpers.


On 12/19/18 1:51 PM, Gerd Hoffmann wrote:

Conversion to atomic modesetting, step three.
Wire up atomic helpers.  Switch planes to atomic.

We are late to the party, the transitional helpers are gone,
so this can't be splitted into smaller steps any more.

Signed-off-by: Gerd Hoffmann 
---
  drivers/gpu/drm/bochs/bochs_fbdev.c |  3 ++
  drivers/gpu/drm/bochs/bochs_kms.c   | 70 +++--
  2 files changed, 70 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs_fbdev.c 
b/drivers/gpu/drm/bochs/bochs_fbdev.c
index dd3c7df267..d9f3d42999 100644
--- a/drivers/gpu/drm/bochs/bochs_fbdev.c
+++ b/drivers/gpu/drm/bochs/bochs_fbdev.c
@@ -6,6 +6,7 @@
   */
  
  #include "bochs.h"

+#include 
  #include 
  
  /* -- */

@@ -149,6 +150,8 @@ bochs_gem_fb_create(struct drm_device *dev, struct drm_file 
*file,
  
  const struct drm_mode_config_funcs bochs_mode_funcs = {

.fb_create = bochs_gem_fb_create,
+   .atomic_check = drm_atomic_helper_check,
+   .atomic_commit = drm_atomic_helper_commit,
  };
  
  int bochs_fbdev_init(struct bochs_device *bochs)

diff --git a/drivers/gpu/drm/bochs/bochs_kms.c 
b/drivers/gpu/drm/bochs/bochs_kms.c
index 18b705fb0b..aa3ba0377a 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -6,7 +6,9 @@
   */
  
  #include "bochs.h"

+#include 
  #include 
+#include 
  
  static int defx = 1024;

  static int defy = 768;
@@ -113,7 +115,7 @@ static int bochs_crtc_page_flip(struct drm_crtc *crtc,
struct drm_framebuffer *old_fb = crtc->primary->fb;
unsigned long irqflags;
  
-	crtc->primary->fb = fb;

+   drm_atomic_set_fb_for_plane(crtc->primary->state, fb);

Taking into account that crtc->primary access goes away in the series:
Reviewed-by: Oleksandr Andrushchenko 

bochs_crtc_mode_set_base(crtc, 0, 0, old_fb);
if (event) {
spin_lock_irqsave(>dev->event_lock, irqflags);
@@ -151,6 +153,9 @@ static const struct drm_crtc_funcs bochs_crtc_funcs = {
.set_config = drm_crtc_helper_set_config,
.destroy = drm_crtc_cleanup,
.page_flip = bochs_crtc_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,
  };
  
  static const struct drm_crtc_helper_funcs bochs_helper_funcs = {

@@ -169,6 +174,59 @@ static const uint32_t bochs_formats[] = {
DRM_FORMAT_BGRX,
  };
  
+static void bochs_plane_atomic_update(struct drm_plane *plane,

+ struct drm_plane_state *old_state)
+{
+   struct bochs_device *bochs = plane->dev->dev_private;
+   struct bochs_bo *bo;
+
+   if (!plane->state->fb)
+   return;
+   bo = gem_to_bochs_bo(plane->state->fb->obj[0]);
+   bochs_hw_setbase(bochs,
+plane->state->crtc_x,
+plane->state->crtc_y,
+bo->bo.offset);
+   bochs_hw_setformat(bochs, plane->state->fb->format);
+}
+
+static int bochs_plane_prepare_fb(struct drm_plane *plane,
+   struct drm_plane_state *new_state)
+{
+   struct bochs_bo *bo;
+
+   if (!new_state->fb)
+   return 0;
+   bo = gem_to_bochs_bo(new_state->fb->obj[0]);
+   return bochs_bo_pin(bo, TTM_PL_FLAG_VRAM, NULL);
+}
+
+static void bochs_plane_cleanup_fb(struct drm_plane *plane,
+  struct drm_plane_state *old_state)
+{
+   struct bochs_bo *bo;
+
+   if (!old_state->fb)
+   return;
+   bo = gem_to_bochs_bo(old_state->fb->obj[0]);
+   bochs_bo_unpin(bo);
+}
+
+static const struct drm_plane_helper_funcs bochs_plane_helper_funcs = {
+   .atomic_update = bochs_plane_atomic_update,
+   .prepare_fb = bochs_plane_prepare_fb,
+   .cleanup_fb = bochs_plane_cleanup_fb,
+};
+
+static const struct drm_plane_funcs bochs_plane_funcs = {
+   .update_plane   = drm_atomic_helper_update_plane,
+   .disable_plane  = drm_atomic_helper_disable_plane,
+   .destroy= drm_primary_helper_destroy,
+   .reset  = drm_atomic_helper_plane_reset,
+   .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+   .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+};
+
  static struct drm_plane *bochs_primary_plane(struct drm_device *dev)
  {
struct drm_plane *primary;
@@ -181,16 +239,17 @@ static struct drm_plane *bochs_primary_plane(struct 
drm_device *dev)
}
  
  	ret = drm_universal_plane_init(dev, primary, 0,

-  _primary_helper_funcs,
+  _plane_funcs,

Re: [PATCH 06/14] drm/bochs: atomic: use atomic set_config helper


On 12/19/18 1:51 PM, Gerd Hoffmann wrote:

Conversion to atomic modesetting, step four.
Use atomic set_config helper for crtc.

Signed-off-by: Gerd Hoffmann 

Reviewed-by: Oleksandr Andrushchenko 

---
  drivers/gpu/drm/bochs/bochs_kms.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bochs/bochs_kms.c 
b/drivers/gpu/drm/bochs/bochs_kms.c
index aa3ba0377a..dcc8b864fc 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -150,7 +150,7 @@ static void bochs_crtc_atomic_flush(struct drm_crtc *crtc,
  
  /* These provide the minimum set of functions required to handle a CRTC */

  static const struct drm_crtc_funcs bochs_crtc_funcs = {
-   .set_config = drm_crtc_helper_set_config,
+   .set_config = drm_atomic_helper_set_config,
.destroy = drm_crtc_cleanup,
.page_flip = bochs_crtc_page_flip,
.reset = drm_atomic_helper_crtc_reset,

Re: [PATCH 04/14] drm/bochs: atomic: add mode_set_nofb callback.




On 12/19/18 1:51 PM, Gerd Hoffmann wrote:

Conversion to atomic modesetting, step two.
Add mode_set_nofb crtc helper callback.

Signed-off-by: Gerd Hoffmann 

Reviewed-by: Oleksandr Andrushchenko 

---
  drivers/gpu/drm/bochs/bochs_kms.c | 9 +
  1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/bochs/bochs_kms.c 
b/drivers/gpu/drm/bochs/bochs_kms.c
index 59d469f343..18b705fb0b 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -86,6 +86,14 @@ static int bochs_crtc_mode_set(struct drm_crtc *crtc,
return 0;
  }
  
+static void bochs_crtc_mode_set_nofb(struct drm_crtc *crtc)

+{
+   struct bochs_device *bochs =
+   container_of(crtc, struct bochs_device, crtc);
+
+   bochs_hw_setmode(bochs, >mode);
+}
+
  static void bochs_crtc_prepare(struct drm_crtc *crtc)
  {
  }
@@ -149,6 +157,7 @@ static const struct drm_crtc_helper_funcs 
bochs_helper_funcs = {
.dpms = bochs_crtc_dpms,
.mode_set = bochs_crtc_mode_set,
.mode_set_base = bochs_crtc_mode_set_base,
+   .mode_set_nofb = bochs_crtc_mode_set_nofb,
.prepare = bochs_crtc_prepare,
.commit = bochs_crtc_commit,
.atomic_enable = bochs_crtc_atomic_enable,

Re: [PATCH v3] drm/bochs: add edid present check


On 12/20/18 12:51 PM, Daniel Vetter wrote:

On Thu, Dec 20, 2018 at 11:11:21AM +0100, Gerd Hoffmann wrote:

Check header before trying to read the complete edid blob, to avoid the
log being spammed in case qemu has no edid support (old qemu or edid
support turned off).

Fixes: 01f23459cf drm/bochs: add edid support.
Signed-off-by: Gerd Hoffmann 

Reviewed-by: Daniel Vetter 

Reviewed-by: Oleksandr Andrushchenko 



---
  drivers/gpu/drm/bochs/bochs_hw.c | 7 +++
  1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/bochs/bochs_hw.c b/drivers/gpu/drm/bochs/bochs_hw.c
index c90a0d492f..d0b4e1cee8 100644
--- a/drivers/gpu/drm/bochs/bochs_hw.c
+++ b/drivers/gpu/drm/bochs/bochs_hw.c
@@ -86,9 +86,16 @@ static int bochs_get_edid_block(void *data, u8 *buf,
  
  int bochs_hw_load_edid(struct bochs_device *bochs)

  {
+   u8 header[8];
+
if (!bochs->mmio)
return -1;
  
+	/* check header to detect whenever edid support is enabled in qemu */

+   bochs_get_edid_block(bochs, header, 0, ARRAY_SIZE(header));
+   if (drm_edid_header_is_valid(header) != 8)
+   return -1;
+
kfree(bochs->edid);
bochs->edid = drm_do_get_edid(>connector,
  bochs_get_edid_block, bochs);
--
2.9.3

Re: [PATCH] drm/xen-front: Make shmem backed display buffer coherent


On 12/19/18 6:14 PM, Noralf Trønnes wrote:


Den 19.12.2018 09.18, skrev Oleksandr Andrushchenko:

On 12/18/18 9:20 PM, Noralf Trønnes wrote:


Den 27.11.2018 11.32, skrev Oleksandr Andrushchenko:

From: Oleksandr Andrushchenko 

When GEM backing storage is allocated with drm_gem_get_pages
the backing pages may be cached, thus making it possible that
the backend sees only partial content of the buffer which may
lead to screen artifacts. Make sure that the frontend's
memory is coherent and the backend always sees correct display
buffer content.

Fixes: c575b7eeb89f ("drm/xen-front: Add support for Xen PV display 
frontend")


Signed-off-by: Oleksandr Andrushchenko 


---
  drivers/gpu/drm/xen/xen_drm_front_gem.c | 62 
+++--

  1 file changed, 48 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
b/drivers/gpu/drm/xen/xen_drm_front_gem.c

index 47ff019d3aef..c592735e49d2 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -33,8 +33,11 @@ struct xen_gem_object {
  /* set for buffers allocated by the backend */
  bool be_alloc;
  -    /* this is for imported PRIME buffer */
-    struct sg_table *sgt_imported;
+    /*
+ * this is for imported PRIME buffer or the one allocated via
+ * drm_gem_get_pages.
+ */
+    struct sg_table *sgt;
  };
    static inline struct xen_gem_object *
@@ -77,10 +80,21 @@ static struct xen_gem_object 
*gem_create_obj(struct drm_device *dev,

  return xen_obj;
  }
  +struct sg_table *xen_drm_front_gem_get_sg_table(struct 
drm_gem_object *gem_obj)

+{
+    struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj);
+
+    if (!xen_obj->pages)
+    return ERR_PTR(-ENOMEM);
+
+    return drm_prime_pages_to_sg(xen_obj->pages, xen_obj->num_pages);
+}
+
  static struct xen_gem_object *gem_create(struct drm_device *dev, 
size_t size)

  {
  struct xen_drm_front_drm_info *drm_info = dev->dev_private;
  struct xen_gem_object *xen_obj;
+    struct address_space *mapping;
  int ret;
    size = round_up(size, PAGE_SIZE);
@@ -113,10 +127,14 @@ static struct xen_gem_object 
*gem_create(struct drm_device *dev, size_t size)

  xen_obj->be_alloc = true;
  return xen_obj;
  }
+
  /*
   * need to allocate backing pages now, so we can share those
   * with the backend
   */



Let's see if I understand what you're doing:

Here you say that the pages should be DMA accessible for devices 
that can

only see 4GB.


Yes, your understanding is correct. As we are a para-virtualized 
device we


do not have strict requirements for 32-bit DMA. But, via dma-buf export,

the buffer we create can be used by real HW, e.g. one can pass-through

real HW devices into a guest domain and they can import our buffer (yes,

they can be IOMMU backed and other conditions may apply).

So, this is why we are limiting to DMA32 here, just to allow more 
possible


use-cases




+    mapping = xen_obj->base.filp->f_mapping;
+    mapping_set_gfp_mask(mapping, GFP_USER | __GFP_DMA32);
+
  xen_obj->num_pages = DIV_ROUND_UP(size, PAGE_SIZE);
  xen_obj->pages = drm_gem_get_pages(_obj->base);
  if (IS_ERR_OR_NULL(xen_obj->pages)) {
@@ -125,8 +143,27 @@ static struct xen_gem_object 
*gem_create(struct drm_device *dev, size_t size)

  goto fail;
  }
  +    xen_obj->sgt = xen_drm_front_gem_get_sg_table(_obj->base);
+    if (IS_ERR_OR_NULL(xen_obj->sgt)){
+    ret = PTR_ERR(xen_obj->sgt);
+    xen_obj->sgt = NULL;
+    goto fail_put_pages;
+    }
+
+    if (!dma_map_sg(dev->dev, xen_obj->sgt->sgl, xen_obj->sgt->nents,
+    DMA_BIDIRECTIONAL)) {



Are you using the DMA streaming API as a way to flush the caches?

Yes

Does this mean that GFP_USER isn't making the buffer coherent?


No, it didn't help. I had a question [1] if there are any other 
better way


to achieve the same, but didn't have any response yet. So, I implemented

it via DMA API which helped.


As Gerd says asking on the arm list is probably the best way of finding a
future proof solution and understanding what's going on.

Yes, it seems so


But if you don't get any help there and you end up with the present
solution I suggest you add a comment that this is for flushing the caches
on arm. With the current code one can be led to believe that the driver
uses the dma address somewhere.

Makes sense


What about x86, does the problem exist there?


Yes, but there I could do drm_clflush_pages which is not implemented for ARM

I wonder if you can call dma_unmap_sg() right away since the flushing has
already happened. That would contain this flushing "hack" inside the
gem_create function.

Yes, I was thinking about this "solution" as well


I also suggest calling drm_prime_pages_to_sg() directly to increase
readability, since the check in xen_drm_front_ge

Re: [PATCH] drm/xen-front: Make shmem backed display buffer coherent


On 12/19/18 4:10 PM, Gerd Hoffmann wrote:

   Hi,


Sure this actually helps?  It's below 4G in guest physical address
space, so it can be backed by pages which are actually above 4G in host
physical address space ...

Yes, you are right here. This is why I wrote about the IOMMU
and other conditions. E.g. you can have a device which only
expects 32-bit, but thanks to IOMMU it can access pages above
4GiB seamlessly. So, this is why I *hope* that this code *may* help
such devices. Do you think I don't need that and have to remove?

I would try without that, and maybe add a runtime option (module
parameter) later if it turns out some hardware actually needs that.
Devices which can do 32bit DMA only become less and less common these
days.

Good point, will remove then

+    if (!dma_map_sg(dev->dev, xen_obj->sgt->sgl, xen_obj->sgt->nents,
+    DMA_BIDIRECTIONAL)) {

Are you using the DMA streaming API as a way to flush the caches?

Yes

Does this mean that GFP_USER isn't making the buffer coherent?

No, it didn't help. I had a question [1] if there are any other better way
to achieve the same, but didn't have any response yet. So, I implemented
it via DMA API which helped.

set_pages_array_*() ?

See arch/x86/include/asm/set_memory.h

Well, x86... I am on arm which doesn't define that...

Oh, arm.  Maybe ask on a arm list then.  I know on arm you have to care
about caching a lot more, but that also is where my knowledge ends ...

Using dma_map_sg for cache flushing looks like a sledge hammer approach
to me.

It is. This is why I am so unsure this is way to go

   But maybe it is needed to make xen flush the caches (xen guests
have their own dma mapping implementation, right?  Or is this different
on arm than on x86?).

I'll try to figure out

cheers,
   Gerd


Thank you,
Oleksandr

Re: [PATCH] drm/bochs: add edid present check


On 12/19/18 5:21 PM, Gerd Hoffmann wrote:

   Hi,


You could probably have a comment here explaining the magic below
(just like in the commit message to ease the task of understanding
while reading the code why 2 of 8 bytes of the EDID header is checked
and why it is all needed). Of course one can use git blame... Up to you

Makes sense.


+   if (readb(bochs->mmio + 0) != 0x00 ||
+   readb(bochs->mmio + 1) != 0xff)

bochs->mmio is defined as "void __iomem   *mmio;". Can we please avoid
void pointer arithmetic here?

Why is that a problem?  gcc uses bytes when doing pointer arithmetic
with void pointers (even though it is undefined in the C standard),
and as far I know the linux kernel depends on that behavior anyway.

Also the driver already does it everywhere.

Ok then, just to be consistent with the rest of the driver.

cheers,
   Gerd

Re: [PATCH 03/14] drm/bochs: atomic: add atomic_flush+atomic_enable callbacks.


Hi, Gerd!

On 12/19/18 1:51 PM, Gerd Hoffmann wrote:

Conversion to atomic modesetting, step one.
Add atomic crtc helper callbacks.

Signed-off-by: Gerd Hoffmann 
---
  drivers/gpu/drm/bochs/bochs_kms.c | 25 +
  1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/bochs/bochs_kms.c 
b/drivers/gpu/drm/bochs/bochs_kms.c
index f7e6d1a9b3..59d469f343 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -115,6 +115,29 @@ static int bochs_crtc_page_flip(struct drm_crtc *crtc,
return 0;
  }
  
+static void bochs_crtc_atomic_enable(struct drm_crtc *crtc,

+struct drm_crtc_state *old_crtc_state)
+{
+}
+
+static void bochs_crtc_atomic_flush(struct drm_crtc *crtc,
+   struct drm_crtc_state *old_crtc_state)
+{
+   struct drm_device *dev = crtc->dev;
+   struct drm_pending_vblank_event *event;
+   unsigned long irqflags;

You could probably move these under the actual if where these are needed,
but it's up to you

+
+   if (crtc->state && crtc->state->event) {
+   event = crtc->state->event;
+   crtc->state->event = NULL;
Not quite sure if touching the event above is allowed w/o holding the 
event lock

+
+   spin_lock_irqsave(>event_lock, irqflags);
+   drm_crtc_send_vblank_event(crtc, event);
+   spin_unlock_irqrestore(>event_lock, irqflags);
+   }
+}
+
+
  /* These provide the minimum set of functions required to handle a CRTC */
  static const struct drm_crtc_funcs bochs_crtc_funcs = {
.set_config = drm_crtc_helper_set_config,
@@ -128,6 +151,8 @@ static const struct drm_crtc_helper_funcs 
bochs_helper_funcs = {
.mode_set_base = bochs_crtc_mode_set_base,
.prepare = bochs_crtc_prepare,
.commit = bochs_crtc_commit,
+   .atomic_enable = bochs_crtc_atomic_enable,
+   .atomic_flush = bochs_crtc_atomic_flush,
  };
  
  static const uint32_t bochs_formats[] = {

Re: [PATCH 02/14] drm/bochs: split bochs_hw_setmode


Hi, Gerd!

On 12/19/18 1:51 PM, Gerd Hoffmann wrote:

Create a separate bochs_hw_setformat function to configure
the framebuffer format (actually just the byteorder).

Signed-off-by: Gerd Hoffmann 
---
  drivers/gpu/drm/bochs/bochs.h |  5 +++--
  drivers/gpu/drm/bochs/bochs_hw.c  | 19 ---
  drivers/gpu/drm/bochs/bochs_kms.c |  3 ++-
  3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
index fb38c8b857..4dc1b6384e 100644
--- a/drivers/gpu/drm/bochs/bochs.h
+++ b/drivers/gpu/drm/bochs/bochs.h
@@ -121,8 +121,9 @@ int bochs_hw_init(struct drm_device *dev);
  void bochs_hw_fini(struct drm_device *dev);
  
  void bochs_hw_setmode(struct bochs_device *bochs,

- struct drm_display_mode *mode,
- const struct drm_format_info *format);
+ struct drm_display_mode *mode);
+void bochs_hw_setformat(struct bochs_device *bochs,
+   const struct drm_format_info *format);
  void bochs_hw_setbase(struct bochs_device *bochs,
  int x, int y, u64 addr);
  int bochs_hw_load_edid(struct bochs_device *bochs);
diff --git a/drivers/gpu/drm/bochs/bochs_hw.c b/drivers/gpu/drm/bochs/bochs_hw.c
index c90a0d492f..bbb251fc78 100644
--- a/drivers/gpu/drm/bochs/bochs_hw.c
+++ b/drivers/gpu/drm/bochs/bochs_hw.c
@@ -197,8 +197,7 @@ void bochs_hw_fini(struct drm_device *dev)
  }
  
  void bochs_hw_setmode(struct bochs_device *bochs,

- struct drm_display_mode *mode,
- const struct drm_format_info *format)
+ struct drm_display_mode *mode)
  {
bochs->xres = mode->hdisplay;
bochs->yres = mode->vdisplay;
@@ -206,12 +205,8 @@ void bochs_hw_setmode(struct bochs_device *bochs,
bochs->stride = mode->hdisplay * (bochs->bpp / 8);
bochs->yres_virtual = bochs->fb_size / bochs->stride;
  
-	DRM_DEBUG_DRIVER("%dx%d @ %d bpp, format %c%c%c%c, vy %d\n",

+   DRM_DEBUG_DRIVER("%dx%d @ %d bpp, vy %d\n",
 bochs->xres, bochs->yres, bochs->bpp,
-(format->format >>  0) & 0xff,
-(format->format >>  8) & 0xff,
-(format->format >> 16) & 0xff,
-(format->format >> 24) & 0xff,
 bochs->yres_virtual);
  
  	bochs_vga_writeb(bochs, 0x3c0, 0x20); /* unblank */

@@ -229,6 +224,16 @@ void bochs_hw_setmode(struct bochs_device *bochs,
  
  	bochs_dispi_write(bochs, VBE_DISPI_INDEX_ENABLE,

  VBE_DISPI_ENABLED | VBE_DISPI_LFB_ENABLED);
+}
+
+void bochs_hw_setformat(struct bochs_device *bochs,
+   const struct drm_format_info *format)
+{
+   DRM_DEBUG_DRIVER("format %c%c%c%c\n",
+(format->format >>  0) & 0xff,
+(format->format >>  8) & 0xff,
+(format->format >> 16) & 0xff,
+(format->format >> 24) & 0xff);
  
  	switch (format->format) {

case DRM_FORMAT_XRGB:
diff --git a/drivers/gpu/drm/bochs/bochs_kms.c 
b/drivers/gpu/drm/bochs/bochs_kms.c
index c8ce54498d..f7e6d1a9b3 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -80,7 +80,8 @@ static int bochs_crtc_mode_set(struct drm_crtc *crtc,
if (WARN_ON(crtc->primary->fb == NULL))
return -EINVAL;
  
-	bochs_hw_setmode(bochs, mode, crtc->primary->fb->format);

+   bochs_hw_setmode(bochs, mode);
+   bochs_hw_setformat(bochs, crtc->primary->fb->format);


I was about to suggest that you go away from direct crtc->primary

use, but it seems this code goes away in this series


bochs_crtc_mode_set_base(crtc, x, y, old_fb);
return 0;
  }

Reviewed-by: Oleksandr Andrushchenko

Re: [PATCH 01/14] drm/bochs: encoder cleanup


Hi, Gerd!

On 12/19/18 1:51 PM, Gerd Hoffmann wrote:

Most unused callbacks can be NULL pointers these days.
Drop a bunch of empty encoder callbacks.

Signed-off-by: Gerd Hoffmann 
---
  drivers/gpu/drm/bochs/bochs_kms.c | 26 --
  1 file changed, 26 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs_kms.c 
b/drivers/gpu/drm/bochs/bochs_kms.c
index f87c284dd9..c8ce54498d 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -170,31 +170,6 @@ static void bochs_crtc_init(struct drm_device *dev)
drm_crtc_helper_add(crtc, _helper_funcs);
  }
  
-static void bochs_encoder_mode_set(struct drm_encoder *encoder,

-  struct drm_display_mode *mode,
-  struct drm_display_mode *adjusted_mode)
-{
-}
-
-static void bochs_encoder_dpms(struct drm_encoder *encoder, int state)
-{
-}
-
-static void bochs_encoder_prepare(struct drm_encoder *encoder)
-{
-}
-
-static void bochs_encoder_commit(struct drm_encoder *encoder)
-{
-}
-
-static const struct drm_encoder_helper_funcs bochs_encoder_helper_funcs = {
-   .dpms = bochs_encoder_dpms,
-   .mode_set = bochs_encoder_mode_set,
-   .prepare = bochs_encoder_prepare,
-   .commit = bochs_encoder_commit,
-};
-
  static const struct drm_encoder_funcs bochs_encoder_encoder_funcs = {
.destroy = drm_encoder_cleanup,
  };
@@ -207,7 +182,6 @@ static void bochs_encoder_init(struct drm_device *dev)
encoder->possible_crtcs = 0x1;
drm_encoder_init(dev, encoder, _encoder_encoder_funcs,
 DRM_MODE_ENCODER_DAC, NULL);
-   drm_encoder_helper_add(encoder, _encoder_helper_funcs);
  }
  
  

Reviewed-by: Oleksandr Andrushchenko

Re: [PATCH] drm/bochs: add edid present check


Hello, Gerd!

On 12/18/18 9:33 AM, Gerd Hoffmann wrote:

Check first two header bytes before trying to read the edid blob,
to avoid the log being spammed in case qemu has no edid support (old
qemu or edid turned off).

Fixes: 01f23459cf drm/bochs: add edid support.
Signed-off-by: Gerd Hoffmann 
---
  drivers/gpu/drm/bochs/bochs_hw.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/bochs/bochs_hw.c b/drivers/gpu/drm/bochs/bochs_hw.c
index c90a0d492f..f91e049625 100644
--- a/drivers/gpu/drm/bochs/bochs_hw.c
+++ b/drivers/gpu/drm/bochs/bochs_hw.c
@@ -89,6 +89,10 @@ int bochs_hw_load_edid(struct bochs_device *bochs)
if (!bochs->mmio)
return -1;
  


You could probably have a comment here explaining the magic below

(just like in the commit message to ease the task of understanding

while reading the code why 2 of 8 bytes of the EDID header is checked

and why it is all needed). Of course one can use git blame... Up to you


+   if (readb(bochs->mmio + 0) != 0x00 ||
+   readb(bochs->mmio + 1) != 0xff)


bochs->mmio is defined as "void __iomem   *mmio;". Can we please avoid

void pointer arithmetic here?


+   return -1;
+
kfree(bochs->edid);
bochs->edid = drm_do_get_edid(>connector,
  bochs_get_edid_block, bochs);


With the above fixed:

Reviewed-by: Oleksandr Andrushchenko

Re: [PATCH] drm/xen-front: Make shmem backed display buffer coherent


On 12/19/18 3:14 PM, Gerd Hoffmann wrote:

   Hi,


+    mapping = xen_obj->base.filp->f_mapping;
+    mapping_set_gfp_mask(mapping, GFP_USER | __GFP_DMA32);

Let's see if I understand what you're doing:

Here you say that the pages should be DMA accessible for devices that can
only see 4GB.

Yes, your understanding is correct. As we are a para-virtualized device we
do not have strict requirements for 32-bit DMA. But, via dma-buf export,
the buffer we create can be used by real HW, e.g. one can pass-through
real HW devices into a guest domain and they can import our buffer (yes,
they can be IOMMU backed and other conditions may apply).
So, this is why we are limiting to DMA32 here, just to allow more possible
use-cases

Sure this actually helps?  It's below 4G in guest physical address
space, so it can be backed by pages which are actually above 4G in host
physical address space ...


Yes, you are right here. This is why I wrote about the IOMMU

and other conditions. E.g. you can have a device which only

expects 32-bit, but thanks to IOMMU it can access pages above

4GiB seamlessly. So, this is why I *hope* that this code *may* help

such devices. Do you think I don't need that and have to remove?


+    if (!dma_map_sg(dev->dev, xen_obj->sgt->sgl, xen_obj->sgt->nents,
+    DMA_BIDIRECTIONAL)) {


Are you using the DMA streaming API as a way to flush the caches?

Yes

Does this mean that GFP_USER isn't making the buffer coherent?

No, it didn't help. I had a question [1] if there are any other better way
to achieve the same, but didn't have any response yet. So, I implemented
it via DMA API which helped.

set_pages_array_*() ?

See arch/x86/include/asm/set_memory.h

Well, x86... I am on arm which doesn't define that...

HTH,
   Gerd


Thank you,

Oleksandr

Re: [PATCH] drm/xen-front: Make shmem backed display buffer coherent


On 12/18/18 9:20 PM, Noralf Trønnes wrote:


Den 27.11.2018 11.32, skrev Oleksandr Andrushchenko:

From: Oleksandr Andrushchenko 

When GEM backing storage is allocated with drm_gem_get_pages
the backing pages may be cached, thus making it possible that
the backend sees only partial content of the buffer which may
lead to screen artifacts. Make sure that the frontend's
memory is coherent and the backend always sees correct display
buffer content.

Fixes: c575b7eeb89f ("drm/xen-front: Add support for Xen PV display 
frontend")


Signed-off-by: Oleksandr Andrushchenko 


---
  drivers/gpu/drm/xen/xen_drm_front_gem.c | 62 +++--
  1 file changed, 48 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
b/drivers/gpu/drm/xen/xen_drm_front_gem.c

index 47ff019d3aef..c592735e49d2 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -33,8 +33,11 @@ struct xen_gem_object {
  /* set for buffers allocated by the backend */
  bool be_alloc;
  -    /* this is for imported PRIME buffer */
-    struct sg_table *sgt_imported;
+    /*
+ * this is for imported PRIME buffer or the one allocated via
+ * drm_gem_get_pages.
+ */
+    struct sg_table *sgt;
  };
    static inline struct xen_gem_object *
@@ -77,10 +80,21 @@ static struct xen_gem_object 
*gem_create_obj(struct drm_device *dev,

  return xen_obj;
  }
  +struct sg_table *xen_drm_front_gem_get_sg_table(struct 
drm_gem_object *gem_obj)

+{
+    struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj);
+
+    if (!xen_obj->pages)
+    return ERR_PTR(-ENOMEM);
+
+    return drm_prime_pages_to_sg(xen_obj->pages, xen_obj->num_pages);
+}
+
  static struct xen_gem_object *gem_create(struct drm_device *dev, 
size_t size)

  {
  struct xen_drm_front_drm_info *drm_info = dev->dev_private;
  struct xen_gem_object *xen_obj;
+    struct address_space *mapping;
  int ret;
    size = round_up(size, PAGE_SIZE);
@@ -113,10 +127,14 @@ static struct xen_gem_object *gem_create(struct 
drm_device *dev, size_t size)

  xen_obj->be_alloc = true;
  return xen_obj;
  }
+
  /*
   * need to allocate backing pages now, so we can share those
   * with the backend
   */



Let's see if I understand what you're doing:

Here you say that the pages should be DMA accessible for devices that can
only see 4GB.


Yes, your understanding is correct. As we are a para-virtualized device we

do not have strict requirements for 32-bit DMA. But, via dma-buf export,

the buffer we create can be used by real HW, e.g. one can pass-through

real HW devices into a guest domain and they can import our buffer (yes,

they can be IOMMU backed and other conditions may apply).

So, this is why we are limiting to DMA32 here, just to allow more possible

use-cases




+    mapping = xen_obj->base.filp->f_mapping;
+    mapping_set_gfp_mask(mapping, GFP_USER | __GFP_DMA32);
+
  xen_obj->num_pages = DIV_ROUND_UP(size, PAGE_SIZE);
  xen_obj->pages = drm_gem_get_pages(_obj->base);
  if (IS_ERR_OR_NULL(xen_obj->pages)) {
@@ -125,8 +143,27 @@ static struct xen_gem_object *gem_create(struct 
drm_device *dev, size_t size)

  goto fail;
  }
  +    xen_obj->sgt = xen_drm_front_gem_get_sg_table(_obj->base);
+    if (IS_ERR_OR_NULL(xen_obj->sgt)){
+    ret = PTR_ERR(xen_obj->sgt);
+    xen_obj->sgt = NULL;
+    goto fail_put_pages;
+    }
+
+    if (!dma_map_sg(dev->dev, xen_obj->sgt->sgl, xen_obj->sgt->nents,
+    DMA_BIDIRECTIONAL)) {



Are you using the DMA streaming API as a way to flush the caches?

Yes

Does this mean that GFP_USER isn't making the buffer coherent?


No, it didn't help. I had a question [1] if there are any other better way

to achieve the same, but didn't have any response yet. So, I implemented

it via DMA API which helped.



Noralf.


+    ret = -EFAULT;
+    goto fail_free_sgt;
+    }
+
  return xen_obj;
  +fail_free_sgt:
+    sg_free_table(xen_obj->sgt);
+    xen_obj->sgt = NULL;
+fail_put_pages:
+    drm_gem_put_pages(_obj->base, xen_obj->pages, true, false);
+    xen_obj->pages = NULL;
  fail:
  DRM_ERROR("Failed to allocate buffer with size %zu\n", size);
  return ERR_PTR(ret);
@@ -149,7 +186,7 @@ void 
xen_drm_front_gem_free_object_unlocked(struct drm_gem_object *gem_obj)

  struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj);
    if (xen_obj->base.import_attach) {
-    drm_prime_gem_destroy(_obj->base, xen_obj->sgt_imported);
+    drm_prime_gem_destroy(_obj->base, xen_obj->sgt);
  gem_free_pages_array(xen_obj);
  } else {
  if (xen_obj->pages) {
@@ -158,6 +195,13 @@ void 
xen_drm_front_gem_free_object_unlocked(struct drm_gem_object *gem_obj)

  xen_ob

Re: [Xen-devel][PATCH v2 2/3] drm/xen-front: Use Xen common shared buffer implementation

2018-12-18 Thread Oleksandr Andrushchenko




On 12/18/18 20:31, Boris Ostrovsky wrote:

On 12/18/18 11:15 AM, Noralf Trønnes wrote:

Den 30.11.2018 08.42, skrev Oleksandr Andrushchenko:

From: Oleksandr Andrushchenko 

Use page directory based shared buffer implementation
now available as common code for Xen frontend drivers.

Remove flushing of shared buffer on page flip as this
workaround needs a proper fix.

Signed-off-by: Oleksandr Andrushchenko

---

Reviewed-by: Noralf Trønnes 

Thank you, Noralf!


Now that all 3 have been acked/reviewed

Applied to for-linus-4.21

Thank you, Boris, Juergen!

Re: [Xen-devel] [PATCH v2 2/3] drm/xen-front: Use Xen common shared buffer implementation

2018-12-18 Thread Oleksandr Andrushchenko


On 12/17/18 5:26 PM, Boris Ostrovsky wrote:

On 12/17/18 10:03 AM, Oleksandr Andrushchenko wrote:

On 12/17/18 4:52 PM, Boris Ostrovsky wrote:

On 12/17/18 5:19 AM, Oleksandr Andrushchenko wrote:

Hello, Juergen, Boris!

As this DRM part of the series is the only one which needs ack/nack

(and it might take quite some time to complete) could we please

merge the patches 1 and 3 now that already have ack/r-b?



TBH I am not sure it makes sense to do this without the second patch.
Refactoring (and IIUIC this series is purely refactoring --- is it not?)
is done to reduce amount of code, and with only first and third patch we
end up with quite a significant increase in the number of LoC. (I am
going purely by diffstat)

Of course, the other reason for refactoring is to eliminate code
duplication, but without second patch that will not happen.

Agree, but this is the basis for the new pv camera frontend

I am working on now [1], so even if we do not remove the code from DRM

then we at least do not add it to the camera driver


Since 1 and 3 are already ACKed you should be able to start the camera
series with these two patches as pre-requisites even if patch 2 is still
stalled by the time your camera code is posted (which I assume will be
4.22 or later).

Agreed, maybe by that time DRM part will also get its r-b/ack



-boris



-boris

Thank you,

Oleksandr

[1]
https://github.com/andr2000/linux/blob/camera_front_v1/drivers/media/xen/Kconfig#L6

Re: [Xen-devel] [PATCH v2 2/3] drm/xen-front: Use Xen common shared buffer implementation

2018-12-17 Thread Oleksandr Andrushchenko


On 12/17/18 4:52 PM, Boris Ostrovsky wrote:

On 12/17/18 5:19 AM, Oleksandr Andrushchenko wrote:

Hello, Juergen, Boris!

As this DRM part of the series is the only one which needs ack/nack

(and it might take quite some time to complete) could we please

merge the patches 1 and 3 now that already have ack/r-b?




TBH I am not sure it makes sense to do this without the second patch.
Refactoring (and IIUIC this series is purely refactoring --- is it not?)
is done to reduce amount of code, and with only first and third patch we
end up with quite a significant increase in the number of LoC. (I am
going purely by diffstat)

Of course, the other reason for refactoring is to eliminate code
duplication, but without second patch that will not happen.


Agree, but this is the basis for the new pv camera frontend

I am working on now [1], so even if we do not remove the code from DRM

then we at least do not add it to the camera driver


-boris


Thank you,

Oleksandr

[1] 
https://github.com/andr2000/linux/blob/camera_front_v1/drivers/media/xen/Kconfig#L6

Re: [Xen-devel][PATCH v2 2/3] drm/xen-front: Use Xen common shared buffer implementation

2018-12-17 Thread Oleksandr Andrushchenko


Hello, Juergen, Boris!

As this DRM part of the series is the only one which needs ack/nack

(and it might take quite some time to complete) could we please

merge the patches 1 and 3 now that already have ack/r-b?

Thank you,

Oleksandr

On 12/13/18 12:16 PM, Oleksandr Andrushchenko wrote:

bump

On 12/5/18 10:20 AM, Oleksandr Andrushchenko wrote:

Hello, Daniel!

Could you please ack/nack the patch, so either we can merge the

series or I can address your comments if any

Thank you,

Oleksandr

On 11/30/18 9:42 AM, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 

Use page directory based shared buffer implementation
now available as common code for Xen frontend drivers.

Remove flushing of shared buffer on page flip as this
workaround needs a proper fix.

Signed-off-by: Oleksandr Andrushchenko 


---
  drivers/gpu/drm/xen/Kconfig   |   1 +
  drivers/gpu/drm/xen/Makefile  |   1 -
  drivers/gpu/drm/xen/xen_drm_front.c   |  65 ++--
  drivers/gpu/drm/xen/xen_drm_front_gem.c   |   1 -
  drivers/gpu/drm/xen/xen_drm_front_shbuf.c | 414 
--

  drivers/gpu/drm/xen/xen_drm_front_shbuf.h |  64 
  6 files changed, 26 insertions(+), 520 deletions(-)
  delete mode 100644 drivers/gpu/drm/xen/xen_drm_front_shbuf.c
  delete mode 100644 drivers/gpu/drm/xen/xen_drm_front_shbuf.h

diff --git a/drivers/gpu/drm/xen/Kconfig b/drivers/gpu/drm/xen/Kconfig
index 4cca160782ab..f969d486855d 100644
--- a/drivers/gpu/drm/xen/Kconfig
+++ b/drivers/gpu/drm/xen/Kconfig
@@ -12,6 +12,7 @@ config DRM_XEN_FRONTEND
  select DRM_KMS_HELPER
  select VIDEOMODE_HELPERS
  select XEN_XENBUS_FRONTEND
+    select XEN_FRONT_PGDIR_SHBUF
  help
    Choose this option if you want to enable a para-virtualized
    frontend DRM/KMS driver for Xen guest OSes.
diff --git a/drivers/gpu/drm/xen/Makefile 
b/drivers/gpu/drm/xen/Makefile

index 712afff5ffc3..825905f67faa 100644
--- a/drivers/gpu/drm/xen/Makefile
+++ b/drivers/gpu/drm/xen/Makefile
@@ -4,7 +4,6 @@ drm_xen_front-objs := xen_drm_front.o \
    xen_drm_front_kms.o \
    xen_drm_front_conn.o \
    xen_drm_front_evtchnl.o \
-  xen_drm_front_shbuf.o \
    xen_drm_front_cfg.o \
    xen_drm_front_gem.o
  diff --git a/drivers/gpu/drm/xen/xen_drm_front.c 
b/drivers/gpu/drm/xen/xen_drm_front.c

index 6b6d5ab82ec3..4d3d36fc3a5d 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.c
+++ b/drivers/gpu/drm/xen/xen_drm_front.c
@@ -19,6 +19,7 @@
  #include 
  #include 
  +#include 
  #include 
    #include "xen_drm_front.h"
@@ -26,28 +27,20 @@
  #include "xen_drm_front_evtchnl.h"
  #include "xen_drm_front_gem.h"
  #include "xen_drm_front_kms.h"
-#include "xen_drm_front_shbuf.h"
    struct xen_drm_front_dbuf {
  struct list_head list;
  u64 dbuf_cookie;
  u64 fb_cookie;
-    struct xen_drm_front_shbuf *shbuf;
+
+    struct xen_front_pgdir_shbuf shbuf;
  };
  -static int dbuf_add_to_list(struct xen_drm_front_info *front_info,
-    struct xen_drm_front_shbuf *shbuf, u64 dbuf_cookie)
+static void dbuf_add_to_list(struct xen_drm_front_info *front_info,
+ struct xen_drm_front_dbuf *dbuf, u64 dbuf_cookie)
  {
-    struct xen_drm_front_dbuf *dbuf;
-
-    dbuf = kzalloc(sizeof(*dbuf), GFP_KERNEL);
-    if (!dbuf)
-    return -ENOMEM;
-
  dbuf->dbuf_cookie = dbuf_cookie;
-    dbuf->shbuf = shbuf;
  list_add(>list, _info->dbuf_list);
-    return 0;
  }
    static struct xen_drm_front_dbuf *dbuf_get(struct list_head 
*dbuf_list,
@@ -62,15 +55,6 @@ static struct xen_drm_front_dbuf *dbuf_get(struct 
list_head *dbuf_list,

  return NULL;
  }
  -static void dbuf_flush_fb(struct list_head *dbuf_list, u64 
fb_cookie)

-{
-    struct xen_drm_front_dbuf *buf, *q;
-
-    list_for_each_entry_safe(buf, q, dbuf_list, list)
-    if (buf->fb_cookie == fb_cookie)
-    xen_drm_front_shbuf_flush(buf->shbuf);
-}
-
  static void dbuf_free(struct list_head *dbuf_list, u64 dbuf_cookie)
  {
  struct xen_drm_front_dbuf *buf, *q;
@@ -78,8 +62,8 @@ static void dbuf_free(struct list_head *dbuf_list, 
u64 dbuf_cookie)

  list_for_each_entry_safe(buf, q, dbuf_list, list)
  if (buf->dbuf_cookie == dbuf_cookie) {
  list_del(>list);
-    xen_drm_front_shbuf_unmap(buf->shbuf);
-    xen_drm_front_shbuf_free(buf->shbuf);
+    xen_front_pgdir_shbuf_unmap(>shbuf);
+    xen_front_pgdir_shbuf_free(>shbuf);
  kfree(buf);
  break;
  }
@@ -91,8 +75,8 @@ static void dbuf_free_all(struct list_head 
*dbuf_list)

    list_for_each_entry_safe(buf, q, dbuf_list, list) {
  list_del(>list);
-    xen_drm_front_shbuf_unmap(buf->shbuf);
-    xen_drm_front_shbuf_free(buf->shbuf);
+    xen_front_pgdir_shbuf_unmap(>shbuf);
+

Re: [PATCH] drm/xen-front: Make shmem backed display buffer coherent

2018-12-17 Thread Oleksandr Andrushchenko


On 12/14/18 10:35 AM, Daniel Vetter wrote:

On Fri, Dec 14, 2018 at 09:09:45AM +0200, Oleksandr Andrushchenko wrote:

On 12/13/18 5:48 PM, Daniel Vetter wrote:

On Thu, Dec 13, 2018 at 12:17:54PM +0200, Oleksandr Andrushchenko wrote:

Daniel, could you please comment?

Cross-revieweing someone else's stuff would scale better,

fair enough

   I don't think
I'll get around to anything before next year.

I put you on CC explicitly because you had comments on other patch [1]

and this one tries to solve the issue raised (I tried to figure out

at [2] if this is the way to go, but it seems I have no alternative here).

While at it [3] (I hope) addresses your comments and the series just

needs your single ack/nack to get in: all the rest ack/r-b are already

there. Do you mind looking at it?

As mentioned, much better if you aim for more per review with others, not
just me. And all that dma coherency stuff isn't something a really
understand all that well (I just know we have lots of pain). For options
maybe work together with Gerd Hoffman or Noralf Tronnes, I think either
has some patches pending that also need some review.


Fair enough,

thank you


-Daniel


-Daniel

Thank you very much for your time,

Oleksandr


Thank you

On 11/27/18 12:32 PM, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 

When GEM backing storage is allocated with drm_gem_get_pages
the backing pages may be cached, thus making it possible that
the backend sees only partial content of the buffer which may
lead to screen artifacts. Make sure that the frontend's
memory is coherent and the backend always sees correct display
buffer content.

Fixes: c575b7eeb89f ("drm/xen-front: Add support for Xen PV display frontend")

Signed-off-by: Oleksandr Andrushchenko 
---
drivers/gpu/drm/xen/xen_drm_front_gem.c | 62 +++--
1 file changed, 48 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
b/drivers/gpu/drm/xen/xen_drm_front_gem.c
index 47ff019d3aef..c592735e49d2 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -33,8 +33,11 @@ struct xen_gem_object {
/* set for buffers allocated by the backend */
bool be_alloc;
-   /* this is for imported PRIME buffer */
-   struct sg_table *sgt_imported;
+   /*
+* this is for imported PRIME buffer or the one allocated via
+* drm_gem_get_pages.
+*/
+   struct sg_table *sgt;
};
static inline struct xen_gem_object *
@@ -77,10 +80,21 @@ static struct xen_gem_object *gem_create_obj(struct 
drm_device *dev,
return xen_obj;
}
+struct sg_table *xen_drm_front_gem_get_sg_table(struct drm_gem_object *gem_obj)
+{
+   struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj);
+
+   if (!xen_obj->pages)
+   return ERR_PTR(-ENOMEM);
+
+   return drm_prime_pages_to_sg(xen_obj->pages, xen_obj->num_pages);
+}
+
static struct xen_gem_object *gem_create(struct drm_device *dev, size_t 
size)
{
struct xen_drm_front_drm_info *drm_info = dev->dev_private;
struct xen_gem_object *xen_obj;
+   struct address_space *mapping;
int ret;
size = round_up(size, PAGE_SIZE);
@@ -113,10 +127,14 @@ static struct xen_gem_object *gem_create(struct 
drm_device *dev, size_t size)
xen_obj->be_alloc = true;
return xen_obj;
}
+
/*
 * need to allocate backing pages now, so we can share those
 * with the backend
 */
+   mapping = xen_obj->base.filp->f_mapping;
+   mapping_set_gfp_mask(mapping, GFP_USER | __GFP_DMA32);
+
xen_obj->num_pages = DIV_ROUND_UP(size, PAGE_SIZE);
xen_obj->pages = drm_gem_get_pages(_obj->base);
if (IS_ERR_OR_NULL(xen_obj->pages)) {
@@ -125,8 +143,27 @@ static struct xen_gem_object *gem_create(struct drm_device 
*dev, size_t size)
goto fail;
}
+   xen_obj->sgt = xen_drm_front_gem_get_sg_table(_obj->base);
+   if (IS_ERR_OR_NULL(xen_obj->sgt)){
+   ret = PTR_ERR(xen_obj->sgt);
+   xen_obj->sgt = NULL;
+   goto fail_put_pages;
+   }
+
+   if (!dma_map_sg(dev->dev, xen_obj->sgt->sgl, xen_obj->sgt->nents,
+   DMA_BIDIRECTIONAL)) {
+   ret = -EFAULT;
+   goto fail_free_sgt;
+   }
+
return xen_obj;
+fail_free_sgt:
+   sg_free_table(xen_obj->sgt);
+   xen_obj->sgt = NULL;
+fail_put_pages:
+   drm_gem_put_pages(_obj->base, xen_obj->pages, true, false);
+   xen_obj->pages = NULL;
fail:
DRM_ERROR("Failed to allocate buffer with size %zu\n", size);
return ERR_PTR(ret);
@@ -149,7 +186,7 @@ void xen_drm_front_gem_free_object_unlocked(struct 
drm_gem_object *gem_obj)
struct xen_gem_object *xen_obj = to_xen_

Re: [PATCH] drm/xen-front: Make shmem backed display buffer coherent

2018-12-13 Thread Oleksandr Andrushchenko


On 12/13/18 5:48 PM, Daniel Vetter wrote:

On Thu, Dec 13, 2018 at 12:17:54PM +0200, Oleksandr Andrushchenko wrote:

Daniel, could you please comment?

Cross-revieweing someone else's stuff would scale better,

fair enough

  I don't think
I'll get around to anything before next year.


I put you on CC explicitly because you had comments on other patch [1]

and this one tries to solve the issue raised (I tried to figure out

at [2] if this is the way to go, but it seems I have no alternative here).

While at it [3] (I hope) addresses your comments and the series just

needs your single ack/nack to get in: all the rest ack/r-b are already

there. Do you mind looking at it?


-Daniel


Thank you very much for your time,

Oleksandr


Thank you

On 11/27/18 12:32 PM, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 

When GEM backing storage is allocated with drm_gem_get_pages
the backing pages may be cached, thus making it possible that
the backend sees only partial content of the buffer which may
lead to screen artifacts. Make sure that the frontend's
memory is coherent and the backend always sees correct display
buffer content.

Fixes: c575b7eeb89f ("drm/xen-front: Add support for Xen PV display frontend")

Signed-off-by: Oleksandr Andrushchenko 
---
   drivers/gpu/drm/xen/xen_drm_front_gem.c | 62 +++--
   1 file changed, 48 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
b/drivers/gpu/drm/xen/xen_drm_front_gem.c
index 47ff019d3aef..c592735e49d2 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -33,8 +33,11 @@ struct xen_gem_object {
/* set for buffers allocated by the backend */
bool be_alloc;
-   /* this is for imported PRIME buffer */
-   struct sg_table *sgt_imported;
+   /*
+* this is for imported PRIME buffer or the one allocated via
+* drm_gem_get_pages.
+*/
+   struct sg_table *sgt;
   };
   static inline struct xen_gem_object *
@@ -77,10 +80,21 @@ static struct xen_gem_object *gem_create_obj(struct 
drm_device *dev,
return xen_obj;
   }
+struct sg_table *xen_drm_front_gem_get_sg_table(struct drm_gem_object *gem_obj)
+{
+   struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj);
+
+   if (!xen_obj->pages)
+   return ERR_PTR(-ENOMEM);
+
+   return drm_prime_pages_to_sg(xen_obj->pages, xen_obj->num_pages);
+}
+
   static struct xen_gem_object *gem_create(struct drm_device *dev, size_t size)
   {
struct xen_drm_front_drm_info *drm_info = dev->dev_private;
struct xen_gem_object *xen_obj;
+   struct address_space *mapping;
int ret;
size = round_up(size, PAGE_SIZE);
@@ -113,10 +127,14 @@ static struct xen_gem_object *gem_create(struct 
drm_device *dev, size_t size)
xen_obj->be_alloc = true;
return xen_obj;
}
+
/*
 * need to allocate backing pages now, so we can share those
 * with the backend
 */
+   mapping = xen_obj->base.filp->f_mapping;
+   mapping_set_gfp_mask(mapping, GFP_USER | __GFP_DMA32);
+
xen_obj->num_pages = DIV_ROUND_UP(size, PAGE_SIZE);
xen_obj->pages = drm_gem_get_pages(_obj->base);
if (IS_ERR_OR_NULL(xen_obj->pages)) {
@@ -125,8 +143,27 @@ static struct xen_gem_object *gem_create(struct drm_device 
*dev, size_t size)
goto fail;
}
+   xen_obj->sgt = xen_drm_front_gem_get_sg_table(_obj->base);
+   if (IS_ERR_OR_NULL(xen_obj->sgt)){
+   ret = PTR_ERR(xen_obj->sgt);
+   xen_obj->sgt = NULL;
+   goto fail_put_pages;
+   }
+
+   if (!dma_map_sg(dev->dev, xen_obj->sgt->sgl, xen_obj->sgt->nents,
+   DMA_BIDIRECTIONAL)) {
+   ret = -EFAULT;
+   goto fail_free_sgt;
+   }
+
return xen_obj;
+fail_free_sgt:
+   sg_free_table(xen_obj->sgt);
+   xen_obj->sgt = NULL;
+fail_put_pages:
+   drm_gem_put_pages(_obj->base, xen_obj->pages, true, false);
+   xen_obj->pages = NULL;
   fail:
DRM_ERROR("Failed to allocate buffer with size %zu\n", size);
return ERR_PTR(ret);
@@ -149,7 +186,7 @@ void xen_drm_front_gem_free_object_unlocked(struct 
drm_gem_object *gem_obj)
struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj);
if (xen_obj->base.import_attach) {
-   drm_prime_gem_destroy(_obj->base, xen_obj->sgt_imported);
+   drm_prime_gem_destroy(_obj->base, xen_obj->sgt);
gem_free_pages_array(xen_obj);
} else {
if (xen_obj->pages) {
@@ -158,6 +195,13 @@ void xen_drm_front_gem_free_object_unlocked(struct 
drm_gem_object *gem_obj)

Re: [PATCH] drm/xen-front: Make shmem backed display buffer coherent

2018-12-13 Thread Oleksandr Andrushchenko


Daniel, could you please comment?

Thank you

On 11/27/18 12:32 PM, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 

When GEM backing storage is allocated with drm_gem_get_pages
the backing pages may be cached, thus making it possible that
the backend sees only partial content of the buffer which may
lead to screen artifacts. Make sure that the frontend's
memory is coherent and the backend always sees correct display
buffer content.

Fixes: c575b7eeb89f ("drm/xen-front: Add support for Xen PV display frontend")

Signed-off-by: Oleksandr Andrushchenko 
---
  drivers/gpu/drm/xen/xen_drm_front_gem.c | 62 +++--
  1 file changed, 48 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
b/drivers/gpu/drm/xen/xen_drm_front_gem.c
index 47ff019d3aef..c592735e49d2 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -33,8 +33,11 @@ struct xen_gem_object {
/* set for buffers allocated by the backend */
bool be_alloc;
  
-	/* this is for imported PRIME buffer */

-   struct sg_table *sgt_imported;
+   /*
+* this is for imported PRIME buffer or the one allocated via
+* drm_gem_get_pages.
+*/
+   struct sg_table *sgt;
  };
  
  static inline struct xen_gem_object *

@@ -77,10 +80,21 @@ static struct xen_gem_object *gem_create_obj(struct 
drm_device *dev,
return xen_obj;
  }
  
+struct sg_table *xen_drm_front_gem_get_sg_table(struct drm_gem_object *gem_obj)

+{
+   struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj);
+
+   if (!xen_obj->pages)
+   return ERR_PTR(-ENOMEM);
+
+   return drm_prime_pages_to_sg(xen_obj->pages, xen_obj->num_pages);
+}
+
  static struct xen_gem_object *gem_create(struct drm_device *dev, size_t size)
  {
struct xen_drm_front_drm_info *drm_info = dev->dev_private;
struct xen_gem_object *xen_obj;
+   struct address_space *mapping;
int ret;
  
  	size = round_up(size, PAGE_SIZE);

@@ -113,10 +127,14 @@ static struct xen_gem_object *gem_create(struct 
drm_device *dev, size_t size)
xen_obj->be_alloc = true;
return xen_obj;
}
+
/*
 * need to allocate backing pages now, so we can share those
 * with the backend
 */
+   mapping = xen_obj->base.filp->f_mapping;
+   mapping_set_gfp_mask(mapping, GFP_USER | __GFP_DMA32);
+
xen_obj->num_pages = DIV_ROUND_UP(size, PAGE_SIZE);
xen_obj->pages = drm_gem_get_pages(_obj->base);
if (IS_ERR_OR_NULL(xen_obj->pages)) {
@@ -125,8 +143,27 @@ static struct xen_gem_object *gem_create(struct drm_device 
*dev, size_t size)
goto fail;
}
  
+	xen_obj->sgt = xen_drm_front_gem_get_sg_table(_obj->base);

+   if (IS_ERR_OR_NULL(xen_obj->sgt)){
+   ret = PTR_ERR(xen_obj->sgt);
+   xen_obj->sgt = NULL;
+   goto fail_put_pages;
+   }
+
+   if (!dma_map_sg(dev->dev, xen_obj->sgt->sgl, xen_obj->sgt->nents,
+   DMA_BIDIRECTIONAL)) {
+   ret = -EFAULT;
+   goto fail_free_sgt;
+   }
+
return xen_obj;
  
+fail_free_sgt:

+   sg_free_table(xen_obj->sgt);
+   xen_obj->sgt = NULL;
+fail_put_pages:
+   drm_gem_put_pages(_obj->base, xen_obj->pages, true, false);
+   xen_obj->pages = NULL;
  fail:
DRM_ERROR("Failed to allocate buffer with size %zu\n", size);
return ERR_PTR(ret);
@@ -149,7 +186,7 @@ void xen_drm_front_gem_free_object_unlocked(struct 
drm_gem_object *gem_obj)
struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj);
  
  	if (xen_obj->base.import_attach) {

-   drm_prime_gem_destroy(_obj->base, xen_obj->sgt_imported);
+   drm_prime_gem_destroy(_obj->base, xen_obj->sgt);
gem_free_pages_array(xen_obj);
} else {
if (xen_obj->pages) {
@@ -158,6 +195,13 @@ void xen_drm_front_gem_free_object_unlocked(struct 
drm_gem_object *gem_obj)
xen_obj->pages);
gem_free_pages_array(xen_obj);
} else {
+   if (xen_obj->sgt) {
+   dma_unmap_sg(xen_obj->base.dev->dev,
+xen_obj->sgt->sgl,
+xen_obj->sgt->nents,
+DMA_BIDIRECTIONAL);
+   sg_free_table(xen_obj->sgt);
+   }
drm_gem_put_pages(_obj->base,
  xen_obj->pages, true, fal

Re: [Xen-devel][PATCH v2 2/3] drm/xen-front: Use Xen common shared buffer implementation

2018-12-13 Thread Oleksandr Andrushchenko


bump

On 12/5/18 10:20 AM, Oleksandr Andrushchenko wrote:

Hello, Daniel!

Could you please ack/nack the patch, so either we can merge the

series or I can address your comments if any

Thank you,

Oleksandr

On 11/30/18 9:42 AM, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 

Use page directory based shared buffer implementation
now available as common code for Xen frontend drivers.

Remove flushing of shared buffer on page flip as this
workaround needs a proper fix.

Signed-off-by: Oleksandr Andrushchenko 


---
  drivers/gpu/drm/xen/Kconfig   |   1 +
  drivers/gpu/drm/xen/Makefile  |   1 -
  drivers/gpu/drm/xen/xen_drm_front.c   |  65 ++--
  drivers/gpu/drm/xen/xen_drm_front_gem.c   |   1 -
  drivers/gpu/drm/xen/xen_drm_front_shbuf.c | 414 --
  drivers/gpu/drm/xen/xen_drm_front_shbuf.h |  64 
  6 files changed, 26 insertions(+), 520 deletions(-)
  delete mode 100644 drivers/gpu/drm/xen/xen_drm_front_shbuf.c
  delete mode 100644 drivers/gpu/drm/xen/xen_drm_front_shbuf.h

diff --git a/drivers/gpu/drm/xen/Kconfig b/drivers/gpu/drm/xen/Kconfig
index 4cca160782ab..f969d486855d 100644
--- a/drivers/gpu/drm/xen/Kconfig
+++ b/drivers/gpu/drm/xen/Kconfig
@@ -12,6 +12,7 @@ config DRM_XEN_FRONTEND
  select DRM_KMS_HELPER
  select VIDEOMODE_HELPERS
  select XEN_XENBUS_FRONTEND
+    select XEN_FRONT_PGDIR_SHBUF
  help
    Choose this option if you want to enable a para-virtualized
    frontend DRM/KMS driver for Xen guest OSes.
diff --git a/drivers/gpu/drm/xen/Makefile b/drivers/gpu/drm/xen/Makefile
index 712afff5ffc3..825905f67faa 100644
--- a/drivers/gpu/drm/xen/Makefile
+++ b/drivers/gpu/drm/xen/Makefile
@@ -4,7 +4,6 @@ drm_xen_front-objs := xen_drm_front.o \
    xen_drm_front_kms.o \
    xen_drm_front_conn.o \
    xen_drm_front_evtchnl.o \
-  xen_drm_front_shbuf.o \
    xen_drm_front_cfg.o \
    xen_drm_front_gem.o
  diff --git a/drivers/gpu/drm/xen/xen_drm_front.c 
b/drivers/gpu/drm/xen/xen_drm_front.c

index 6b6d5ab82ec3..4d3d36fc3a5d 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.c
+++ b/drivers/gpu/drm/xen/xen_drm_front.c
@@ -19,6 +19,7 @@
  #include 
  #include 
  +#include 
  #include 
    #include "xen_drm_front.h"
@@ -26,28 +27,20 @@
  #include "xen_drm_front_evtchnl.h"
  #include "xen_drm_front_gem.h"
  #include "xen_drm_front_kms.h"
-#include "xen_drm_front_shbuf.h"
    struct xen_drm_front_dbuf {
  struct list_head list;
  u64 dbuf_cookie;
  u64 fb_cookie;
-    struct xen_drm_front_shbuf *shbuf;
+
+    struct xen_front_pgdir_shbuf shbuf;
  };
  -static int dbuf_add_to_list(struct xen_drm_front_info *front_info,
-    struct xen_drm_front_shbuf *shbuf, u64 dbuf_cookie)
+static void dbuf_add_to_list(struct xen_drm_front_info *front_info,
+ struct xen_drm_front_dbuf *dbuf, u64 dbuf_cookie)
  {
-    struct xen_drm_front_dbuf *dbuf;
-
-    dbuf = kzalloc(sizeof(*dbuf), GFP_KERNEL);
-    if (!dbuf)
-    return -ENOMEM;
-
  dbuf->dbuf_cookie = dbuf_cookie;
-    dbuf->shbuf = shbuf;
  list_add(>list, _info->dbuf_list);
-    return 0;
  }
    static struct xen_drm_front_dbuf *dbuf_get(struct list_head 
*dbuf_list,
@@ -62,15 +55,6 @@ static struct xen_drm_front_dbuf *dbuf_get(struct 
list_head *dbuf_list,

  return NULL;
  }
  -static void dbuf_flush_fb(struct list_head *dbuf_list, u64 fb_cookie)
-{
-    struct xen_drm_front_dbuf *buf, *q;
-
-    list_for_each_entry_safe(buf, q, dbuf_list, list)
-    if (buf->fb_cookie == fb_cookie)
-    xen_drm_front_shbuf_flush(buf->shbuf);
-}
-
  static void dbuf_free(struct list_head *dbuf_list, u64 dbuf_cookie)
  {
  struct xen_drm_front_dbuf *buf, *q;
@@ -78,8 +62,8 @@ static void dbuf_free(struct list_head *dbuf_list, 
u64 dbuf_cookie)

  list_for_each_entry_safe(buf, q, dbuf_list, list)
  if (buf->dbuf_cookie == dbuf_cookie) {
  list_del(>list);
-    xen_drm_front_shbuf_unmap(buf->shbuf);
-    xen_drm_front_shbuf_free(buf->shbuf);
+    xen_front_pgdir_shbuf_unmap(>shbuf);
+    xen_front_pgdir_shbuf_free(>shbuf);
  kfree(buf);
  break;
  }
@@ -91,8 +75,8 @@ static void dbuf_free_all(struct list_head *dbuf_list)
    list_for_each_entry_safe(buf, q, dbuf_list, list) {
  list_del(>list);
-    xen_drm_front_shbuf_unmap(buf->shbuf);
-    xen_drm_front_shbuf_free(buf->shbuf);
+    xen_front_pgdir_shbuf_unmap(>shbuf);
+    xen_front_pgdir_shbuf_free(>shbuf);
  kfree(buf);
  }
  }
@@ -171,9 +155,9 @@ int xen_drm_front_dbuf_create(struct 
xen_drm_front_info *front_info,

    u32 bpp, u64 size, struct page **pages)
  {
  struct xen_drm_front_evtchnl *evtchnl;
-    struct xen_drm_front_shbuf *shbuf;
+

Re: [Xen-devel] [PATCH 5/9] drm/xen/xen_drm_front_gem.c: Convert to use vm_insert_range


On 11/19/18 12:42 PM, Souptick Joarder wrote:

On Mon, Nov 19, 2018 at 3:22 PM Oleksandr Andrushchenko
 wrote:

On 11/15/18 5:49 PM, Souptick Joarder wrote:

Convert to use vm_insert_range() to map range of kernel
memory to user vma.

Signed-off-by: Souptick Joarder 
Reviewed-by: Matthew Wilcox 
---
   drivers/gpu/drm/xen/xen_drm_front_gem.c | 20 ++--
   1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
b/drivers/gpu/drm/xen/xen_drm_front_gem.c
index 47ff019..a3eade6 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -225,8 +225,7 @@ struct drm_gem_object *
   static int gem_mmap_obj(struct xen_gem_object *xen_obj,
   struct vm_area_struct *vma)
   {
- unsigned long addr = vma->vm_start;
- int i;
+ int err;

I would love to keep ret, not err

Sure, will add it in v2.
But I think, err is more appropriate here.


I used "ret" throughout the driver, so this is just to remain consistent:

grep -rnw err drivers/gpu/drm/xen/ | wc -l
0
grep -rnw ret drivers/gpu/drm/xen/ | wc -l
204


   /*
* clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
@@ -247,18 +246,11 @@ static int gem_mmap_obj(struct xen_gem_object *xen_obj,
* FIXME: as we insert all the pages now then no .fault handler must
* be called, so don't provide one
*/
- for (i = 0; i < xen_obj->num_pages; i++) {
- int ret;
-
- ret = vm_insert_page(vma, addr, xen_obj->pages[i]);
- if (ret < 0) {
- DRM_ERROR("Failed to insert pages into vma: %d\n", ret);
- return ret;
- }
-
- addr += PAGE_SIZE;
- }
- return 0;
+ err = vm_insert_range(vma, vma->vm_start, xen_obj->pages,
+ xen_obj->num_pages);
+ if (err < 0)
+ DRM_ERROR("Failed to insert pages into vma: %d\n", err);
+ return err;
   }

   int xen_drm_front_gem_mmap(struct file *filp, struct vm_area_struct *vma)

With the above fixed,

Reviewed-by: Oleksandr Andrushchenko

Re: [Xen-devel] [PATCH 5/9] drm/xen/xen_drm_front_gem.c: Convert to use vm_insert_range


On 11/19/18 12:42 PM, Souptick Joarder wrote:

On Mon, Nov 19, 2018 at 3:22 PM Oleksandr Andrushchenko
 wrote:

On 11/15/18 5:49 PM, Souptick Joarder wrote:

Convert to use vm_insert_range() to map range of kernel
memory to user vma.

Signed-off-by: Souptick Joarder 
Reviewed-by: Matthew Wilcox 
---
   drivers/gpu/drm/xen/xen_drm_front_gem.c | 20 ++--
   1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
b/drivers/gpu/drm/xen/xen_drm_front_gem.c
index 47ff019..a3eade6 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -225,8 +225,7 @@ struct drm_gem_object *
   static int gem_mmap_obj(struct xen_gem_object *xen_obj,
   struct vm_area_struct *vma)
   {
- unsigned long addr = vma->vm_start;
- int i;
+ int err;

I would love to keep ret, not err

Sure, will add it in v2.
But I think, err is more appropriate here.


I used "ret" throughout the driver, so this is just to remain consistent:

grep -rnw err drivers/gpu/drm/xen/ | wc -l
0
grep -rnw ret drivers/gpu/drm/xen/ | wc -l
204


   /*
* clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
@@ -247,18 +246,11 @@ static int gem_mmap_obj(struct xen_gem_object *xen_obj,
* FIXME: as we insert all the pages now then no .fault handler must
* be called, so don't provide one
*/
- for (i = 0; i < xen_obj->num_pages; i++) {
- int ret;
-
- ret = vm_insert_page(vma, addr, xen_obj->pages[i]);
- if (ret < 0) {
- DRM_ERROR("Failed to insert pages into vma: %d\n", ret);
- return ret;
- }
-
- addr += PAGE_SIZE;
- }
- return 0;
+ err = vm_insert_range(vma, vma->vm_start, xen_obj->pages,
+ xen_obj->num_pages);
+ if (err < 0)
+ DRM_ERROR("Failed to insert pages into vma: %d\n", err);
+ return err;
   }

   int xen_drm_front_gem_mmap(struct file *filp, struct vm_area_struct *vma)

With the above fixed,

Reviewed-by: Oleksandr Andrushchenko

Re: [Xen-devel] [PATCH 5/9] drm/xen/xen_drm_front_gem.c: Convert to use vm_insert_range


On 11/15/18 5:49 PM, Souptick Joarder wrote:

Convert to use vm_insert_range() to map range of kernel
memory to user vma.

Signed-off-by: Souptick Joarder 
Reviewed-by: Matthew Wilcox 
---
  drivers/gpu/drm/xen/xen_drm_front_gem.c | 20 ++--
  1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
b/drivers/gpu/drm/xen/xen_drm_front_gem.c
index 47ff019..a3eade6 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -225,8 +225,7 @@ struct drm_gem_object *
  static int gem_mmap_obj(struct xen_gem_object *xen_obj,
struct vm_area_struct *vma)
  {
-   unsigned long addr = vma->vm_start;
-   int i;
+   int err;

I would love to keep ret, not err
  
  	/*

 * clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
@@ -247,18 +246,11 @@ static int gem_mmap_obj(struct xen_gem_object *xen_obj,
 * FIXME: as we insert all the pages now then no .fault handler must
 * be called, so don't provide one
 */
-   for (i = 0; i < xen_obj->num_pages; i++) {
-   int ret;
-
-   ret = vm_insert_page(vma, addr, xen_obj->pages[i]);
-   if (ret < 0) {
-   DRM_ERROR("Failed to insert pages into vma: %d\n", ret);
-   return ret;
-   }
-
-   addr += PAGE_SIZE;
-   }
-   return 0;
+   err = vm_insert_range(vma, vma->vm_start, xen_obj->pages,
+   xen_obj->num_pages);
+   if (err < 0)
+   DRM_ERROR("Failed to insert pages into vma: %d\n", err);
+   return err;
  }
  
  int xen_drm_front_gem_mmap(struct file *filp, struct vm_area_struct *vma)


With the above fixed,

Reviewed-by: Oleksandr Andrushchenko

Re: [Xen-devel] [PATCH 5/9] drm/xen/xen_drm_front_gem.c: Convert to use vm_insert_range