[PATCH -fixes 1/5] drm/vmwgfx: don't check for old_crtc_state enable status

[PATCH -fixes 1/5] drm/vmwgfx: don't check for old_crtc_state enable status

[Thomas Hellstrom]

From: Deepak Rawat <drawat@vmware.com>

During atomic check to prepare the new topology no need to check if
old_crtc_state was enabled or not. This will cause atomic_check to fail
because due to connector routing a crtc can be in atomic_state even if
there was no change to enable status.

Detected this issue with igt run.

Signed-off-by: Deepak Rawat <drawat@vmware.com>
Reviewed-by: Sinclair Yeh <syeh@vmware.com>
Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 23beff5d8e3c..636b962849c8 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -1615,7 +1615,7 @@ static int vmw_kms_check_topology(struct drm_device *dev,
 		struct drm_connector_state *conn_state;
 		struct vmw_connector_state *vmw_conn_state;
 
-		if (!new_crtc_state->enable && old_crtc_state->enable) {
+		if (!new_crtc_state->enable) {
 			rects[i].x1 = 0;
 			rects[i].y1 = 0;
 			rects[i].x2 = 0;
-- 
2.19.0.rc1

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

[PATCH -fixes 2/5] drm/vmwgfx: limit screen size to stdu_max during check_modeset

[Thomas Hellstrom]

From: Deepak Rawat <drawat@vmware.com>

For STDU individual screen target size is limited by
SVGA_REG_SCREENTARGET_MAX_WIDTH/HEIGHT registers so add that limit
during atomic check_modeset.

An additional limit is placed in the update_layout ioctl to avoid
requesting layouts that current user-space typically can't support.
Also modified the comments to reflect current limitation on topology.

Signed-off-by: Deepak Rawat <drawat@vmware.com>
Reviewed-by: Sinclair Yeh <syeh@vmware.com>
Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>
Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 30 +++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 636b962849c8..12a41b039167 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -1512,21 +1512,19 @@ static int vmw_kms_check_display_memory(struct drm_device *dev,
 					struct drm_rect *rects)
 {
 	struct vmw_private *dev_priv = vmw_priv(dev);
-	struct drm_mode_config *mode_config = &dev->mode_config;
 	struct drm_rect bounding_box = {0};
 	u64 total_pixels = 0, pixel_mem, bb_mem;
 	int i;
 
 	for (i = 0; i < num_rects; i++) {
 		/*
-		 * Currently this check is limiting the topology within max
-		 * texture/screentarget size. This should change in future when
-		 * user-space support multiple fb with topology.
+		 * For STDU only individual screen (screen target) is limited by
+		 * SCREENTARGET_MAX_WIDTH/HEIGHT registers.
 		 */
-		if (rects[i].x1 < 0 ||  rects[i].y1 < 0 ||
-		    rects[i].x2 > mode_config->max_width ||
-		    rects[i].y2 > mode_config->max_height) {
-			DRM_ERROR("Invalid GUI layout.\n");
+		if (dev_priv->active_display_unit == vmw_du_screen_target &&
+		    (drm_rect_width(&rects[i]) > dev_priv->stdu_max_width ||
+		     drm_rect_height(&rects[i]) > dev_priv->stdu_max_height)) {
+			DRM_ERROR("Screen size not supported.\n");
 			return -EINVAL;
 		}
 
@@ -2376,6 +2374,7 @@ int vmw_kms_update_layout_ioctl(struct drm_device *dev, void *data,
 				struct drm_file *file_priv)
 {
 	struct vmw_private *dev_priv = vmw_priv(dev);
+	struct drm_mode_config *mode_config = &dev->mode_config;
 	struct drm_vmw_update_layout_arg *arg =
 		(struct drm_vmw_update_layout_arg *)data;
 	void __user *user_rects;
@@ -2421,6 +2420,21 @@ int vmw_kms_update_layout_ioctl(struct drm_device *dev, void *data,
 		drm_rects[i].y1 = curr_rect.y;
 		drm_rects[i].x2 = curr_rect.x + curr_rect.w;
 		drm_rects[i].y2 = curr_rect.y + curr_rect.h;
+
+		/*
+		 * Currently this check is limiting the topology within
+		 * mode_config->max (which actually is max texture size
+		 * supported by virtual device). This limit is here to address
+		 * window managers that create a big framebuffer for whole
+		 * topology.
+		 */
+		if (drm_rects[i].x1 < 0 ||  drm_rects[i].y1 < 0 ||
+		    drm_rects[i].x2 > mode_config->max_width ||
+		    drm_rects[i].y2 > mode_config->max_height) {
+			DRM_ERROR("Invalid GUI layout.\n");
+			ret = -EINVAL;
+			goto out_free;
+		}
 	}
 
 	ret = vmw_kms_check_display_memory(dev, arg->num_outputs, drm_rects);
-- 
2.19.0.rc1

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

[PATCH -fixes 3/5] drm/vmwgfx: limit mode size for all display unit to texture_max

[Thomas Hellstrom]

From: Deepak Rawat <drawat@vmware.com>

For all display units, limit mode size exposed to texture_max_width/
height as this is the maximum framebuffer size that virtual device can
create.

Signed-off-by: Deepak Rawat <drawat@vmware.com>
Reviewed-by: Sinclair Yeh <syeh@vmware.com>
Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>
Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 12a41b039167..6a712a8d59e9 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -2214,12 +2214,16 @@ int vmw_du_connector_fill_modes(struct drm_connector *connector,
 	if (dev_priv->assume_16bpp)
 		assumed_bpp = 2;
 
+	max_width  = min(max_width,  dev_priv->texture_max_width);
+	max_height = min(max_height, dev_priv->texture_max_height);
+
+	/*
+	 * For STDU extra limit for a mode on SVGA_REG_SCREENTARGET_MAX_WIDTH/
+	 * HEIGHT registers.
+	 */
 	if (dev_priv->active_display_unit == vmw_du_screen_target) {
 		max_width  = min(max_width,  dev_priv->stdu_max_width);
-		max_width  = min(max_width,  dev_priv->texture_max_width);
-
 		max_height = min(max_height, dev_priv->stdu_max_height);
-		max_height = min(max_height, dev_priv->texture_max_height);
 	}
 
 	/* Add preferred mode */
-- 
2.19.0.rc1

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

[PATCH -fixes 4/5] drm/vmwgfx: Don't impose STDU limits on framebuffer size

[Thomas Hellstrom]

From: Deepak Rawat <drawat@vmware.com>

If framebuffers are larger, we create bounce surfaces that are within
STDU limits.

Signed-off-by: Deepak Rawat <drawat@vmware.com>
Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>
Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c    | 25 -------------------------
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 24 ++++++++++++++----------
 2 files changed, 14 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index 93f6b96ca7bb..f30e839f7bfd 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -1600,31 +1600,6 @@ int vmw_kms_stdu_init_display(struct vmw_private *dev_priv)
 
 	dev_priv->active_display_unit = vmw_du_screen_target;
 
-	if (dev_priv->capabilities & SVGA_CAP_3D) {
-		/*
-		 * For 3D VMs, display (scanout) buffer size is the smaller of
-		 * max texture and max STDU
-		 */
-		uint32_t max_width, max_height;
-
-		max_width = min(dev_priv->texture_max_width,
-				dev_priv->stdu_max_width);
-		max_height = min(dev_priv->texture_max_height,
-				 dev_priv->stdu_max_height);
-
-		dev->mode_config.max_width = max_width;
-		dev->mode_config.max_height = max_height;
-	} else {
-		/*
-		 * Given various display aspect ratios, there's no way to
-		 * estimate these using prim_bb_mem.  So just set these to
-		 * something arbitrarily large and we will reject any layout
-		 * that doesn't fit prim_bb_mem later
-		 */
-		dev->mode_config.max_width = 8192;
-		dev->mode_config.max_height = 8192;
-	}
-
 	vmw_kms_create_implicit_placement_property(dev_priv, false);
 
 	for (i = 0; i < VMWGFX_NUM_DISPLAY_UNITS; ++i) {
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index e125233e074b..80a01cd4c051 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -1404,22 +1404,17 @@ int vmw_surface_gb_priv_define(struct drm_device *dev,
 	*srf_out = NULL;
 
 	if (for_scanout) {
-		uint32_t max_width, max_height;
-
 		if (!svga3dsurface_is_screen_target_format(format)) {
 			DRM_ERROR("Invalid Screen Target surface format.");
 			return -EINVAL;
 		}
 
-		max_width = min(dev_priv->texture_max_width,
-				dev_priv->stdu_max_width);
-		max_height = min(dev_priv->texture_max_height,
-				 dev_priv->stdu_max_height);
-
-		if (size.width > max_width || size.height > max_height) {
+		if (size.width > dev_priv->texture_max_width ||
+		    size.height > dev_priv->texture_max_height) {
 			DRM_ERROR("%ux%u\n, exceeds max surface size %ux%u",
 				  size.width, size.height,
-				  max_width, max_height);
+				  dev_priv->texture_max_width,
+				  dev_priv->texture_max_height);
 			return -EINVAL;
 		}
 	} else {
@@ -1495,8 +1490,17 @@ int vmw_surface_gb_priv_define(struct drm_device *dev,
 	if (srf->flags & SVGA3D_SURFACE_BIND_STREAM_OUTPUT)
 		srf->res.backup_size += sizeof(SVGA3dDXSOState);
 
+	/*
+	 * Don't set SVGA3D_SURFACE_SCREENTARGET flag for a scanout surface with
+	 * size greater than STDU max width/height. This is really a workaround
+	 * to support creation of big framebuffer requested by some user-space
+	 * for whole topology. That big framebuffer won't really be used for
+	 * binding with screen target as during prepare_fb a separate surface is
+	 * created so it's safe to ignore SVGA3D_SURFACE_SCREENTARGET flag.
+	 */
 	if (dev_priv->active_display_unit == vmw_du_screen_target &&
-	    for_scanout)
+	    for_scanout && size.width <= dev_priv->stdu_max_width &&
+	    size.height <= dev_priv->stdu_max_height)
 		srf->flags |= SVGA3D_SURFACE_SCREENTARGET;
 
 	/*
-- 
2.19.0.rc1

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

[PATCH -fixes 5/5] drm/vmwgfx: Fix a buffer object eviction regression

[Thomas Hellstrom]

Commit 4eb085e42fde ("drm/vmwgfx: Convert to new IDA API") indroduced
an incorrect return value from the function vmw_gmrid_man_get_node(),
when we run out if integer ids. Instead of returning 0 (meaning
non-fatal error) we forward the ida_simple_get error code -ENOSPC.
This causes TTM not to retry allocation after buffer eviction and
instead return -ENOSPC to user-space.

Fix this by returning 0 when ida_simple_get() returns -ENOSPC.

Tested using glretrace.

Cc: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Charmaine Lee <charmainel@vmware.com>
Reviewed-by: Deepak Rawat <drawat@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c
index b93c558dd86e..a38a0c3777f7 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c
@@ -57,7 +57,7 @@ static int vmw_gmrid_man_get_node(struct ttm_mem_type_manager *man,
 
 	id = ida_alloc_max(&gman->gmr_ida, gman->max_gmr_ids - 1, GFP_KERNEL);
 	if (id < 0)
-		return id;
+		return (id == -ENOSPC ? 0 : id);
 
 	spin_lock(&gman->lock);
 
-- 
2.19.0.rc1

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

Re: [PATCH -fixes 5/5] drm/vmwgfx: Fix a buffer object eviction regression

[Matthew Wilcox]

On Thu, Sep 13, 2018 at 01:58:37PM +0200, Thomas Hellstrom wrote:
> Commit 4eb085e42fde ("drm/vmwgfx: Convert to new IDA API") indroduced
> an incorrect return value from the function vmw_gmrid_man_get_node(),
> when we run out if integer ids. Instead of returning 0 (meaning
> non-fatal error) we forward the ida_simple_get error code -ENOSPC.
> This causes TTM not to retry allocation after buffer eviction and
> instead return -ENOSPC to user-space.
> 
> Fix this by returning 0 when ida_simple_get() returns -ENOSPC.

Thanks.  I got confused by the convoluted code that was there before ;-(

I think this could be better though ... if ida_alloc() ever starts
returning a different errno in the future, you'll hit the same problem,
right?  So how about this ...

 	id = ida_alloc_max(&gman->gmr_ida, gman->max_gmr_ids - 1, GFP_KERNEL);
+	if (id == -ENOMEM)
+		return -ENOMEM;
+	if (id < 0)
+		return 0;
  
 	spin_lock(&gman->lock);

But I wonder ... why is -ENOMEM seen as a fatal error?  If you free up
some memory, you'll free up an ID, so the next time around you should
be able to allocate an ID.  So shouldn't this function just have
been doing this all along?

 	id = ida_alloc_max(&gman->gmr_ida, gman->max_gmr_ids - 1, GFP_KERNEL);
+	if (id < 0)
+		return 0;

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

Re: [PATCH -fixes 5/5] drm/vmwgfx: Fix a buffer object eviction regression

[Thomas Hellstrom]

Hi,

On 09/13/2018 04:10 PM, Matthew Wilcox wrote:
> On Thu, Sep 13, 2018 at 01:58:37PM +0200, Thomas Hellstrom wrote:
>> Commit 4eb085e42fde ("drm/vmwgfx: Convert to new IDA API") indroduced
>> an incorrect return value from the function vmw_gmrid_man_get_node(),
>> when we run out if integer ids. Instead of returning 0 (meaning
>> non-fatal error) we forward the ida_simple_get error code -ENOSPC.
>> This causes TTM not to retry allocation after buffer eviction and
>> instead return -ENOSPC to user-space.
>>
>> Fix this by returning 0 when ida_simple_get() returns -ENOSPC.
> Thanks.  I got confused by the convoluted code that was there before ;-(
>
> I think this could be better though ... if ida_alloc() ever starts
> returning a different errno in the future, you'll hit the same problem,
> right?  So how about this ...
>
>   	id = ida_alloc_max(&gman->gmr_ida, gman->max_gmr_ids - 1, GFP_KERNEL);
> +	if (id == -ENOMEM)
> +		return -ENOMEM;
> +	if (id < 0)
> +		return 0;
>    
>   	spin_lock(&gman->lock);
>
> But I wonder ... why is -ENOMEM seen as a fatal error?  If you free up
> some memory, you'll free up an ID, so the next time around you should
> be able to allocate an ID.  So shouldn't this function just have
> been doing this all along?
>
>   	id = ida_alloc_max(&gman->gmr_ida, gman->max_gmr_ids - 1, GFP_KERNEL);
> +	if (id < 0)
> +		return 0;
>
Non-fatal errors are errors that can be remedied by GPU buffer eviction, 
and buffer eviction will free up IDA space, so basically we need to 
target only the error code that indicates we've run out of IDA space.

If we're worried that ida_alloc_max() will change return value, I guess 
we will have to increase the IDA space and detect the error ourselves:  
error if (id >= gman->max_gmr_ids)

/Thomas


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

Re: [PATCH -fixes 5/5] drm/vmwgfx: Fix a buffer object eviction regression

[Matthew Wilcox]

On Thu, Sep 13, 2018 at 04:56:53PM +0200, Thomas Hellstrom wrote:
> Hi,
> 
> On 09/13/2018 04:10 PM, Matthew Wilcox wrote:
> > On Thu, Sep 13, 2018 at 01:58:37PM +0200, Thomas Hellstrom wrote:
> > > Commit 4eb085e42fde ("drm/vmwgfx: Convert to new IDA API") indroduced
> > > an incorrect return value from the function vmw_gmrid_man_get_node(),
> > > when we run out if integer ids. Instead of returning 0 (meaning
> > > non-fatal error) we forward the ida_simple_get error code -ENOSPC.
> > > This causes TTM not to retry allocation after buffer eviction and
> > > instead return -ENOSPC to user-space.
> > > 
> > > Fix this by returning 0 when ida_simple_get() returns -ENOSPC.
> > Thanks.  I got confused by the convoluted code that was there before ;-(
> > 
> > I think this could be better though ... if ida_alloc() ever starts
> > returning a different errno in the future, you'll hit the same problem,
> > right?  So how about this ...
> > 
> >   	id = ida_alloc_max(&gman->gmr_ida, gman->max_gmr_ids - 1, GFP_KERNEL);
> > +	if (id == -ENOMEM)
> > +		return -ENOMEM;
> > +	if (id < 0)
> > +		return 0;
> >   	spin_lock(&gman->lock);
> > 
> > But I wonder ... why is -ENOMEM seen as a fatal error?  If you free up
> > some memory, you'll free up an ID, so the next time around you should
> > be able to allocate an ID.  So shouldn't this function just have
> > been doing this all along?
> > 
> >   	id = ida_alloc_max(&gman->gmr_ida, gman->max_gmr_ids - 1, GFP_KERNEL);
> > +	if (id < 0)
> > +		return 0;
> > 
> Non-fatal errors are errors that can be remedied by GPU buffer eviction, and
> buffer eviction will free up IDA space, so basically we need to target only
> the error code that indicates we've run out of IDA space.

Yes, but the following situation can happen:

 - Allocate 1024 IDs
 - Run very low on memory
 - Allocating ID 1025 will fail (very very unlikely)
 - ida_alloc_max() returns -ENOMEM

In this situation, we want ttm_mem_evict_first() to be called which will
free up one of the 1024 existing IDs and then we can allocate that ID for
our new node.

I'm assuming we're analysing the behaviour of ttm_bo_mem_force_space()
here.

> If we're worried that ida_alloc_max() will change return value, I guess we
> will have to increase the IDA space and detect the error ourselves:  error
> if (id >= gman->max_gmr_ids)

My point was that your solution (detect the one error which should be
deemed as non-fatal) was not as robust as its inverse (detect the one
error which the previous code deemed as fatal).  But I now believe no
error from the IDA should be seen as fatal.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH -fixes 5/5] drm/vmwgfx: Fix a buffer object eviction regression

[Thomas Hellstrom]

On 09/13/2018 05:28 PM, Matthew Wilcox wrote:
> On Thu, Sep 13, 2018 at 04:56:53PM +0200, Thomas Hellstrom wrote:
>> Hi,
>>
>> On 09/13/2018 04:10 PM, Matthew Wilcox wrote:
>>> On Thu, Sep 13, 2018 at 01:58:37PM +0200, Thomas Hellstrom wrote:
>>>> Commit 4eb085e42fde ("drm/vmwgfx: Convert to new IDA API") indroduced
>>>> an incorrect return value from the function vmw_gmrid_man_get_node(),
>>>> when we run out if integer ids. Instead of returning 0 (meaning
>>>> non-fatal error) we forward the ida_simple_get error code -ENOSPC.
>>>> This causes TTM not to retry allocation after buffer eviction and
>>>> instead return -ENOSPC to user-space.
>>>>
>>>> Fix this by returning 0 when ida_simple_get() returns -ENOSPC.
>>> Thanks.  I got confused by the convoluted code that was there before ;-(
>>>
>>> I think this could be better though ... if ida_alloc() ever starts
>>> returning a different errno in the future, you'll hit the same problem,
>>> right?  So how about this ...
>>>
>>>    	id = ida_alloc_max(&gman->gmr_ida, gman->max_gmr_ids - 1, GFP_KERNEL);
>>> +	if (id == -ENOMEM)
>>> +		return -ENOMEM;
>>> +	if (id < 0)
>>> +		return 0;
>>>    	spin_lock(&gman->lock);
>>>
>>> But I wonder ... why is -ENOMEM seen as a fatal error?  If you free up
>>> some memory, you'll free up an ID, so the next time around you should
>>> be able to allocate an ID.  So shouldn't this function just have
>>> been doing this all along?
>>>
>>>    	id = ida_alloc_max(&gman->gmr_ida, gman->max_gmr_ids - 1, GFP_KERNEL);
>>> +	if (id < 0)
>>> +		return 0;
>>>
>> Non-fatal errors are errors that can be remedied by GPU buffer eviction, and
>> buffer eviction will free up IDA space, so basically we need to target only
>> the error code that indicates we've run out of IDA space.
> Yes, but the following situation can happen:
>
>   - Allocate 1024 IDs
>   - Run very low on memory
>   - Allocating ID 1025 will fail (very very unlikely)
>   - ida_alloc_max() returns -ENOMEM
>
> In this situation, we want ttm_mem_evict_first() to be called which will
> free up one of the 1024 existing IDs and then we can allocate that ID for
> our new node.
>
> I'm assuming we're analysing the behaviour of ttm_bo_mem_force_space()
> here.

Well, that's true, but that situation depends I guess very much on the 
radix tree implementation of IDA? Also I would expect the eviction paths 
to try to allocate more memory here and there, so to me the preferred 
option when -ENOMEM happens, is really to back off as soon as possible 
to avoid interfering with shrinker work going on etc.

>> If we're worried that ida_alloc_max() will change return value, I guess we
>> will have to increase the IDA space and detect the error ourselves:  error
>> if (id >= gman->max_gmr_ids)
> My point was that your solution (detect the one error which should be
> deemed as non-fatal) was not as robust as its inverse (detect the one
> error which the previous code deemed as fatal).  But I now believe no
> error from the IDA should be seen as fatal.

If you insist, I can test on -ENOMEM instead of -ENOSPC to mimic the 
pre-change behaviour. We should really focus on the IDA api changes 
here, and defer changing -ENOMEM to non-fatal to a follow-up patch if 
needed.

Thanks,

Thomas


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

Re: [PATCH -fixes 5/5] drm/vmwgfx: Fix a buffer object eviction regression

[Matthew Wilcox]

On Thu, Sep 13, 2018 at 06:52:43PM +0200, Thomas Hellstrom wrote:
> On 09/13/2018 05:28 PM, Matthew Wilcox wrote:
> > On Thu, Sep 13, 2018 at 04:56:53PM +0200, Thomas Hellstrom wrote:
> > > On 09/13/2018 04:10 PM, Matthew Wilcox wrote:
> > > > I think this could be better though ... if ida_alloc() ever starts
> > > > returning a different errno in the future, you'll hit the same problem,
> > > > right?  So how about this ...
> > > > 
> > > >    	id = ida_alloc_max(&gman->gmr_ida, gman->max_gmr_ids - 1, GFP_KERNEL);
> > > > +	if (id == -ENOMEM)
> > > > +		return -ENOMEM;
> > > > +	if (id < 0)
> > > > +		return 0;
> > > >    	spin_lock(&gman->lock);
> > > > 
> > > > But I wonder ... why is -ENOMEM seen as a fatal error?  If you free up
> > > > some memory, you'll free up an ID, so the next time around you should
> > > > be able to allocate an ID.  So shouldn't this function just have
> > > > been doing this all along?
> > > > 
> > > >    	id = ida_alloc_max(&gman->gmr_ida, gman->max_gmr_ids - 1, GFP_KERNEL);
> > > > +	if (id < 0)
> > > > +		return 0;
> > > > 
> > > Non-fatal errors are errors that can be remedied by GPU buffer eviction, and
> > > buffer eviction will free up IDA space, so basically we need to target only
> > > the error code that indicates we've run out of IDA space.
> > Yes, but the following situation can happen:
> > 
> >   - Allocate 1024 IDs
> >   - Run very low on memory
> >   - Allocating ID 1025 will fail (very very unlikely)
> >   - ida_alloc_max() returns -ENOMEM
> > 
> > In this situation, we want ttm_mem_evict_first() to be called which will
> > free up one of the 1024 existing IDs and then we can allocate that ID for
> > our new node.
> > 
> > I'm assuming we're analysing the behaviour of ttm_bo_mem_force_space()
> > here.
> 
> Well, that's true, but that situation depends I guess very much on the radix
> tree implementation of IDA?

The specific number 1024 depends on the current implementation, but
generally speaking at some point, the IDA has to allocate memory to store
one extra ID.  Because the IDA cannot allocate memory when freeing an
ID, there is no more compact representation of the allocated IDs smaller
than a bitmap.

> Also I would expect the eviction paths to try to
> allocate more memory here and there, so to me the preferred option when
> -ENOMEM happens, is really to back off as soon as possible to avoid
> interfering with shrinker work going on etc.

I would be surprised if freeing resources needs memory to be allocated.
That's not supposed to happen; the filesystems go to great lengths to
pre-allocate enough memory that they can always write at least one dirty
page back to storage without allocating any memory, for example.  Maybe
the DRM subsystem is different; I'm not an expert in your subsystem.

> > My point was that your solution (detect the one error which should be
> > deemed as non-fatal) was not as robust as its inverse (detect the one
> > error which the previous code deemed as fatal).  But I now believe no
> > error from the IDA should be seen as fatal.
> 
> If you insist, I can test on -ENOMEM instead of -ENOSPC to mimic the
> pre-change behaviour. We should really focus on the IDA api changes here,
> and defer changing -ENOMEM to non-fatal to a follow-up patch if needed.

I'd be comfortable with that solution for now.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH -fixes 5/5] drm/vmwgfx: Fix a buffer object eviction regression

[Thomas Hellstrom]

On 09/13/2018 07:38 PM, Matthew Wilcox wrote:
> On Thu, Sep 13, 2018 at 06:52:43PM +0200, Thomas Hellstrom wrote:
>> On 09/13/2018 05:28 PM, Matthew Wilcox wrote:
>>> On Thu, Sep 13, 2018 at 04:56:53PM +0200, Thomas Hellstrom wrote:
>>>> On 09/13/2018 04:10 PM, Matthew Wilcox wrote:
>>>>> I think this could be better though ... if ida_alloc() ever starts
>>>>> returning a different errno in the future, you'll hit the same problem,
>>>>> right?  So how about this ...
>>>>>
>>>>>     	id = ida_alloc_max(&gman->gmr_ida, gman->max_gmr_ids - 1, GFP_KERNEL);
>>>>> +	if (id == -ENOMEM)
>>>>> +		return -ENOMEM;
>>>>> +	if (id < 0)
>>>>> +		return 0;
>>>>>     	spin_lock(&gman->lock);
>>>>>
>>>>> But I wonder ... why is -ENOMEM seen as a fatal error?  If you free up
>>>>> some memory, you'll free up an ID, so the next time around you should
>>>>> be able to allocate an ID.  So shouldn't this function just have
>>>>> been doing this all along?
>>>>>
>>>>>     	id = ida_alloc_max(&gman->gmr_ida, gman->max_gmr_ids - 1, GFP_KERNEL);
>>>>> +	if (id < 0)
>>>>> +		return 0;
>>>>>
>>>> Non-fatal errors are errors that can be remedied by GPU buffer eviction, and
>>>> buffer eviction will free up IDA space, so basically we need to target only
>>>> the error code that indicates we've run out of IDA space.
>>> Yes, but the following situation can happen:
>>>
>>>    - Allocate 1024 IDs
>>>    - Run very low on memory
>>>    - Allocating ID 1025 will fail (very very unlikely)
>>>    - ida_alloc_max() returns -ENOMEM
>>>
>>> In this situation, we want ttm_mem_evict_first() to be called which will
>>> free up one of the 1024 existing IDs and then we can allocate that ID for
>>> our new node.
>>>
>>> I'm assuming we're analysing the behaviour of ttm_bo_mem_force_space()
>>> here.
>> Well, that's true, but that situation depends I guess very much on the radix
>> tree implementation of IDA?
> The specific number 1024 depends on the current implementation, but
> generally speaking at some point, the IDA has to allocate memory to store
> one extra ID.  Because the IDA cannot allocate memory when freeing an
> ID, there is no more compact representation of the allocated IDs smaller
> than a bitmap.
>
>> Also I would expect the eviction paths to try to
>> allocate more memory here and there, so to me the preferred option when
>> -ENOMEM happens, is really to back off as soon as possible to avoid
>> interfering with shrinker work going on etc.
> I would be surprised if freeing resources needs memory to be allocated.
> That's not supposed to happen; the filesystems go to great lengths to
> pre-allocate enough memory that they can always write at least one dirty
> page back to storage without allocating any memory, for example.  Maybe
> the DRM subsystem is different; I'm not an expert in your subsystem.

It's different. In particular when evicting a large buffer from VRAM 
(which is on-card memory) to system memory, the subsystem may allocate a 
huge amount of memory. But this particular case is not evicting from 
VRAM (although it may lead to it).

>>> My point was that your solution (detect the one error which should be
>>> deemed as non-fatal) was not as robust as its inverse (detect the one
>>> error which the previous code deemed as fatal).  But I now believe no
>>> error from the IDA should be seen as fatal.
>> If you insist, I can test on -ENOMEM instead of -ENOSPC to mimic the
>> pre-change behaviour. We should really focus on the IDA api changes here,
>> and defer changing -ENOMEM to non-fatal to a follow-up patch if needed.
> I'd be comfortable with that solution for now.

OK, I'll respin and check for -ENOMEM instead.

Thanks,
Thomas


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