[PATCH] drm/crc: Handle opening and closing crc better

[PATCH] drm/crc: Handle opening and closing crc better

[Maarten Lankhorst]

When I was doing a grep . -r /sys/kernel/debug/dri/0 I noticed a WARN
appearing when I aborted the grep with ^C.

After investigating I've also noticed that the error handling was
lacking and there are race conditions involving multiple calls to
open/close simultaneously.

Fix this by setting the opened flag first and using crc->entries to
decide when crc can be collected.

Also call unset crc source before cleaning up, this way there is
no race with a future open().

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_debugfs_crc.c | 46 ++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 1722d8f21449..d0ea4627a093 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -136,21 +136,38 @@ static int crtc_crc_data_count(struct drm_crtc_crc *crc)
 	return CIRC_CNT(crc->head, crc->tail, DRM_CRC_ENTRIES_NR);
 }
 
+static void crtc_crc_cleanup(struct drm_crtc_crc *crc)
+{
+	kfree(crc->entries);
+	crc->entries = NULL;
+	crc->head = 0;
+	crc->tail = 0;
+	crc->values_cnt = 0;
+	crc->opened = false;
+}
+
 static int crtc_crc_open(struct inode *inode, struct file *filep)
 {
 	struct drm_crtc *crtc = inode->i_private;
 	struct drm_crtc_crc *crc = &crtc->crc;
 	struct drm_crtc_crc_entry *entries = NULL;
 	size_t values_cnt;
-	int ret;
+	int ret = 0;
 
-	if (crc->opened)
-		return -EBUSY;
+	spin_lock_irq(&crc->lock);
+	if (!crc->opened)
+		crc->opened = true;
+	else
+		ret = -EBUSY;
+	spin_unlock_irq(&crc->lock);
 
-	ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt);
 	if (ret)
 		return ret;
 
+	ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt);
+	if (ret)
+		goto err;
+
 	if (WARN_ON(values_cnt > DRM_MAX_CRC_NR)) {
 		ret = -EINVAL;
 		goto err_disable;
@@ -170,7 +187,6 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
 	spin_lock_irq(&crc->lock);
 	crc->entries = entries;
 	crc->values_cnt = values_cnt;
-	crc->opened = true;
 
 	/*
 	 * Only return once we got a first frame, so userspace doesn't have to
@@ -182,12 +198,17 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
 						crc->lock);
 	spin_unlock_irq(&crc->lock);
 
-	WARN_ON(ret);
+	if (ret)
+		goto err_disable;
 
 	return 0;
 
 err_disable:
 	crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
+err:
+	spin_lock_irq(&crc->lock);
+	crtc_crc_cleanup(crc);
+	spin_unlock_irq(&crc->lock);
 	return ret;
 }
 
@@ -197,17 +218,12 @@ static int crtc_crc_release(struct inode *inode, struct file *filep)
 	struct drm_crtc_crc *crc = &crtc->crc;
 	size_t values_cnt;
 
+	crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
+
 	spin_lock_irq(&crc->lock);
-	kfree(crc->entries);
-	crc->entries = NULL;
-	crc->head = 0;
-	crc->tail = 0;
-	crc->values_cnt = 0;
-	crc->opened = false;
+	crtc_crc_cleanup(crc);
 	spin_unlock_irq(&crc->lock);
 
-	crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
-
 	return 0;
 }
 
@@ -334,7 +350,7 @@ int drm_crtc_add_crc_entry(struct drm_crtc *crtc, bool has_frame,
 	spin_lock(&crc->lock);
 
 	/* Caller may not have noticed yet that userspace has stopped reading */
-	if (!crc->opened) {
+	if (!crc->entries) {
 		spin_unlock(&crc->lock);
 		return -EINVAL;
 	}
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/crc: Handle opening and closing crc better

[Tomeu Vizoso]

Looks good to me:

Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

I guess you have tested this with IGT? In any case, I think it would
be good to mention how a patch has been tested in the changelog. That
can be very useful to others if things go wrong at some point.

Thanks,

Tomeu




On 21 June 2017 at 13:00, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> When I was doing a grep . -r /sys/kernel/debug/dri/0 I noticed a WARN
> appearing when I aborted the grep with ^C.
>
> After investigating I've also noticed that the error handling was
> lacking and there are race conditions involving multiple calls to
> open/close simultaneously.
>
> Fix this by setting the opened flag first and using crc->entries to
> decide when crc can be collected.
>
> Also call unset crc source before cleaning up, this way there is
> no race with a future open().
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_debugfs_crc.c | 46 ++++++++++++++++++++++++++-------------
>  1 file changed, 31 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> index 1722d8f21449..d0ea4627a093 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -136,21 +136,38 @@ static int crtc_crc_data_count(struct drm_crtc_crc *crc)
>         return CIRC_CNT(crc->head, crc->tail, DRM_CRC_ENTRIES_NR);
>  }
>
> +static void crtc_crc_cleanup(struct drm_crtc_crc *crc)
> +{
> +       kfree(crc->entries);
> +       crc->entries = NULL;
> +       crc->head = 0;
> +       crc->tail = 0;
> +       crc->values_cnt = 0;
> +       crc->opened = false;
> +}
> +
>  static int crtc_crc_open(struct inode *inode, struct file *filep)
>  {
>         struct drm_crtc *crtc = inode->i_private;
>         struct drm_crtc_crc *crc = &crtc->crc;
>         struct drm_crtc_crc_entry *entries = NULL;
>         size_t values_cnt;
> -       int ret;
> +       int ret = 0;
>
> -       if (crc->opened)
> -               return -EBUSY;
> +       spin_lock_irq(&crc->lock);
> +       if (!crc->opened)
> +               crc->opened = true;
> +       else
> +               ret = -EBUSY;
> +       spin_unlock_irq(&crc->lock);
>
> -       ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt);
>         if (ret)
>                 return ret;
>
> +       ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt);
> +       if (ret)
> +               goto err;
> +
>         if (WARN_ON(values_cnt > DRM_MAX_CRC_NR)) {
>                 ret = -EINVAL;
>                 goto err_disable;
> @@ -170,7 +187,6 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
>         spin_lock_irq(&crc->lock);
>         crc->entries = entries;
>         crc->values_cnt = values_cnt;
> -       crc->opened = true;
>
>         /*
>          * Only return once we got a first frame, so userspace doesn't have to
> @@ -182,12 +198,17 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
>                                                 crc->lock);
>         spin_unlock_irq(&crc->lock);
>
> -       WARN_ON(ret);
> +       if (ret)
> +               goto err_disable;
>
>         return 0;
>
>  err_disable:
>         crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
> +err:
> +       spin_lock_irq(&crc->lock);
> +       crtc_crc_cleanup(crc);
> +       spin_unlock_irq(&crc->lock);
>         return ret;
>  }
>
> @@ -197,17 +218,12 @@ static int crtc_crc_release(struct inode *inode, struct file *filep)
>         struct drm_crtc_crc *crc = &crtc->crc;
>         size_t values_cnt;
>
> +       crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
> +
>         spin_lock_irq(&crc->lock);
> -       kfree(crc->entries);
> -       crc->entries = NULL;
> -       crc->head = 0;
> -       crc->tail = 0;
> -       crc->values_cnt = 0;
> -       crc->opened = false;
> +       crtc_crc_cleanup(crc);
>         spin_unlock_irq(&crc->lock);
>
> -       crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
> -
>         return 0;
>  }
>
> @@ -334,7 +350,7 @@ int drm_crtc_add_crc_entry(struct drm_crtc *crtc, bool has_frame,
>         spin_lock(&crc->lock);
>
>         /* Caller may not have noticed yet that userspace has stopped reading */
> -       if (!crc->opened) {
> +       if (!crc->entries) {
>                 spin_unlock(&crc->lock);
>                 return -EINVAL;
>         }
> --
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/crc: Only open CRC on atomic drivers when the CRTC is active.

[Maarten Lankhorst]

Op 06-07-17 om 13:09 schreef Tomeu Vizoso:
> Looks good to me:
>
> Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>
> I guess you have tested this with IGT? In any case, I think it would
> be good to mention how a patch has been tested in the changelog. That
> can be very useful to others if things go wrong at some point.
Testcase: debugfs_test.read_all_entries

But I hit it by doing a recursive grep, which I guess is the same thing here. :)

One further improvement I wanted to do was reject opening the CRC with -EIO
when the crtc is not active, that way the above test will not hang.
Does the below patch also look good to you?

----8<-----
Commit e8fa5671183c ("drm: crc: Wait for a frame before returning
from open()") adds a wait for CRC frame, but with the CRTC off
this will never be generated. For atomic drivers we know if a CRTC
is active through crtc_state->active, so when inactive reject the
open with -EIO.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: e8fa5671183c ("drm: crc: Wait for a frame before returning from open()")
Testcase: debugfs_test.read_all_entries
---
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index d0ea4627a093..f9e26dda56d6 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -154,6 +154,19 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
 	size_t values_cnt;
 	int ret = 0;
 
+	if (drm_drv_uses_atomic_modeset(crtc->dev)) {
+		ret = drm_modeset_lock_interruptible(&crtc->mutex, NULL);
+		if (ret)
+			return ret;
+
+		if (!crtc->state->active)
+			ret = -EIO;
+		drm_modeset_unlock(&crtc->mutex);
+
+		if (ret)
+			return ret;
+	}
+
 	spin_lock_irq(&crc->lock);
 	if (!crc->opened)
 		crc->opened = true;

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/crc: Only open CRC on atomic drivers when the CRTC is active.

[Daniel Vetter]

On Thu, Jul 06, 2017 at 03:03:15PM +0200, Maarten Lankhorst wrote:
> Op 06-07-17 om 13:09 schreef Tomeu Vizoso:
> > Looks good to me:
> >
> > Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> >
> > I guess you have tested this with IGT? In any case, I think it would
> > be good to mention how a patch has been tested in the changelog. That
> > can be very useful to others if things go wrong at some point.
> Testcase: debugfs_test.read_all_entries
> 
> But I hit it by doing a recursive grep, which I guess is the same thing here. :)
> 
> One further improvement I wanted to do was reject opening the CRC with -EIO
> when the crtc is not active, that way the above test will not hang.
> Does the below patch also look good to you?
> 
> ----8<-----
> Commit e8fa5671183c ("drm: crc: Wait for a frame before returning
> from open()") adds a wait for CRC frame, but with the CRTC off
> this will never be generated. For atomic drivers we know if a CRTC
> is active through crtc_state->active, so when inactive reject the
> open with -EIO.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: e8fa5671183c ("drm: crc: Wait for a frame before returning from open()")
> Testcase: debugfs_test.read_all_entries

At least for the semantics I think this makes sense. Opening the CRC file
when the crtc is off is undefined.

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

But pls get Tomeu's ack too.

Thanks, Daniel


> ---
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> index d0ea4627a093..f9e26dda56d6 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -154,6 +154,19 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
>  	size_t values_cnt;
>  	int ret = 0;
>  
> +	if (drm_drv_uses_atomic_modeset(crtc->dev)) {
> +		ret = drm_modeset_lock_interruptible(&crtc->mutex, NULL);
> +		if (ret)
> +			return ret;
> +
> +		if (!crtc->state->active)
> +			ret = -EIO;
> +		drm_modeset_unlock(&crtc->mutex);
> +
> +		if (ret)
> +			return ret;
> +	}
> +
>  	spin_lock_irq(&crc->lock);
>  	if (!crc->opened)
>  		crc->opened = true;
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/crc: Only open CRC on atomic drivers when the CRTC is active.

[Maarten Lankhorst]

Op 07-07-17 om 13:01 schreef Daniel Vetter:
> On Thu, Jul 06, 2017 at 03:03:15PM +0200, Maarten Lankhorst wrote:
>> Op 06-07-17 om 13:09 schreef Tomeu Vizoso:
>>> Looks good to me:
>>>
>>> Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>>
>>> I guess you have tested this with IGT? In any case, I think it would
>>> be good to mention how a patch has been tested in the changelog. That
>>> can be very useful to others if things go wrong at some point.
>> Testcase: debugfs_test.read_all_entries
>>
>> But I hit it by doing a recursive grep, which I guess is the same thing here. :)
>>
>> One further improvement I wanted to do was reject opening the CRC with -EIO
>> when the crtc is not active, that way the above test will not hang.
>> Does the below patch also look good to you?
>>
>> ----8<-----
>> Commit e8fa5671183c ("drm: crc: Wait for a frame before returning
>> from open()") adds a wait for CRC frame, but with the CRTC off
>> this will never be generated. For atomic drivers we know if a CRTC
>> is active through crtc_state->active, so when inactive reject the
>> open with -EIO.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Fixes: e8fa5671183c ("drm: crc: Wait for a frame before returning from open()")
>> Testcase: debugfs_test.read_all_entries
> At least for the semantics I think this makes sense. Opening the CRC file
> when the crtc is off is undefined.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> But pls get Tomeu's ack too.
Tomeu, can you ack? :)

I did some testing on IGT with both patches applied on all tests with
CRC in their name, no problems with opening CRC as far as I can see,
and all tests except kms_ccs and kms_mmap_write_crc succeed. The
former needs render compression, the latter fails on a crc failure,
so it can't have been caused by this patch.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx