[PATCH] drm/crc: Add support for polling on the data fd.

[PATCH] drm/crc: Add support for polling on the data fd.

[Maarten Lankhorst]

This will make it possible for userspace to know whether reading
will block, without blocking on the fd. This makes it possible to
drain all queued CRC's in blocking mode, without having to reopen
the fd.

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

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 9dd879589a2c..8af1a74ec64d 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -307,10 +307,29 @@ static ssize_t crtc_crc_read(struct file *filep, char __user *user_buf,
 	return LINE_LEN(crc->values_cnt);
 }
 
+static unsigned int crtc_crc_poll(struct file *file, poll_table *wait)
+{
+	struct drm_crtc *crtc = file->f_inode->i_private;
+	struct drm_crtc_crc *crc = &crtc->crc;
+	unsigned ret;
+
+	poll_wait(file, &crc->wq, wait);
+
+	spin_lock_irq(&crc->lock);
+	if (crc->source && crtc_crc_data_count(crc))
+		ret = POLLIN;
+	else
+		ret = 0;
+	spin_unlock_irq(&crc->lock);
+
+	return ret;
+}
+
 static const struct file_operations drm_crtc_crc_data_fops = {
 	.owner = THIS_MODULE,
 	.open = crtc_crc_open,
 	.read = crtc_crc_read,
+	.poll = crtc_crc_poll,
 	.release = crtc_crc_release,
 };
 
-- 
2.15.1

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

Re: [PATCH] drm/crc: Add support for polling on the data fd.

[Ville Syrjälä]

On Fri, Feb 02, 2018 at 03:27:43PM +0100, Maarten Lankhorst wrote:
> This will make it possible for userspace to know whether reading
> will block, without blocking on the fd. This makes it possible to
> drain all queued CRC's in blocking mode, without having to reopen
> the fd.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_debugfs_crc.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> index 9dd879589a2c..8af1a74ec64d 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -307,10 +307,29 @@ static ssize_t crtc_crc_read(struct file *filep, char __user *user_buf,
>  	return LINE_LEN(crc->values_cnt);
>  }
>  
> +static unsigned int crtc_crc_poll(struct file *file, poll_table *wait)
> +{
> +	struct drm_crtc *crtc = file->f_inode->i_private;
> +	struct drm_crtc_crc *crc = &crtc->crc;
> +	unsigned ret;
> +
> +	poll_wait(file, &crc->wq, wait);
> +
> +	spin_lock_irq(&crc->lock);
> +	if (crc->source && crtc_crc_data_count(crc))
> +		ret = POLLIN;

Most places seem to also set POLLRDNORM. Maybe we should do it as well?

Apart from that this seems good to me.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Could replace the usleep() loop in igt read_one_crc() with
poll/select() I suppose? Either that or we should switch between
blocking and nonblocking dynamically.

> +	else
> +		ret = 0;
> +	spin_unlock_irq(&crc->lock);
> +
> +	return ret;
> +}
> +
>  static const struct file_operations drm_crtc_crc_data_fops = {
>  	.owner = THIS_MODULE,
>  	.open = crtc_crc_open,
>  	.read = crtc_crc_read,
> +	.poll = crtc_crc_poll,
>  	.release = crtc_crc_release,
>  };
>  
> -- 
> 2.15.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm/crc: Add support for polling on the data fd.

[Patchwork]

== Series Details ==

Series: drm/crc: Add support for polling on the data fd.
URL   : https://patchwork.freedesktop.org/series/37550/
State : warning

== Summary ==

Series 37550v1 drm/crc: Add support for polling on the data fd.
https://patchwork.freedesktop.org/api/1.0/series/37550/revisions/1/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                incomplete -> PASS       (fi-snb-2520m) fdo#103713 +1
Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> DMESG-WARN (fi-skl-guc)
        Subgroup basic-s4-devices:
                incomplete -> PASS       (fi-bdw-5557u) fdo#104162

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#104162 https://bugs.freedesktop.org/show_bug.cgi?id=104162

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:420s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:420s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:371s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:483s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:284s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:480s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:476s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:464s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:451s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:566s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:413s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:278s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:508s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:386s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:399s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:411s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:445s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:411s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:458s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:495s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:451s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:501s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:570s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:424s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:506s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:527s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:488s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:476s
fi-skl-guc       total:288  pass:259  dwarn:1   dfail:0   fail:0   skip:28  time:412s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:430s
fi-snb-2520m     total:245  pass:211  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:392s
Blacklisted hosts:
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:467s

bde4e003ce549b8225142843e0d5aee19dd37d13 drm-tip: 2018y-02m-02d-15h-03m-15s UTC integration manifest
507e2c4c0b73 drm/crc: Add support for polling on the data fd.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7862/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/crc: Add support for polling on the data fd.

[Maarten Lankhorst]

Op 02-02-18 om 15:46 schreef Ville Syrjälä:
> On Fri, Feb 02, 2018 at 03:27:43PM +0100, Maarten Lankhorst wrote:
>> This will make it possible for userspace to know whether reading
>> will block, without blocking on the fd. This makes it possible to
>> drain all queued CRC's in blocking mode, without having to reopen
>> the fd.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_debugfs_crc.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
>> index 9dd879589a2c..8af1a74ec64d 100644
>> --- a/drivers/gpu/drm/drm_debugfs_crc.c
>> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
>> @@ -307,10 +307,29 @@ static ssize_t crtc_crc_read(struct file *filep, char __user *user_buf,
>>  	return LINE_LEN(crc->values_cnt);
>>  }
>>  
>> +static unsigned int crtc_crc_poll(struct file *file, poll_table *wait)
>> +{
>> +	struct drm_crtc *crtc = file->f_inode->i_private;
>> +	struct drm_crtc_crc *crc = &crtc->crc;
>> +	unsigned ret;
>> +
>> +	poll_wait(file, &crc->wq, wait);
>> +
>> +	spin_lock_irq(&crc->lock);
>> +	if (crc->source && crtc_crc_data_count(crc))
>> +		ret = POLLIN;
> Most places seem to also set POLLRDNORM. Maybe we should do it as well?
>
> Apart from that this seems good to me.
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Yeah, changed it and pushed, thanks for the suggestion. :)
> Could replace the usleep() loop in igt read_one_crc() with
> poll/select() I suppose? Either that or we should switch between
> blocking and nonblocking dynamically.
It could, but it would use 100% of cpu on older kernels that don't support poll(), if that's not a problem we could do it. :)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/crc: Add support for polling on the data fd.

[Ville Syrjälä]

On Mon, Feb 05, 2018 at 01:59:05PM +0100, Maarten Lankhorst wrote:
> Op 02-02-18 om 15:46 schreef Ville Syrjälä:
> > On Fri, Feb 02, 2018 at 03:27:43PM +0100, Maarten Lankhorst wrote:
> >> This will make it possible for userspace to know whether reading
> >> will block, without blocking on the fd. This makes it possible to
> >> drain all queued CRC's in blocking mode, without having to reopen
> >> the fd.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/drm_debugfs_crc.c | 19 +++++++++++++++++++
> >>  1 file changed, 19 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> >> index 9dd879589a2c..8af1a74ec64d 100644
> >> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> >> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> >> @@ -307,10 +307,29 @@ static ssize_t crtc_crc_read(struct file *filep, char __user *user_buf,
> >>  	return LINE_LEN(crc->values_cnt);
> >>  }
> >>  
> >> +static unsigned int crtc_crc_poll(struct file *file, poll_table *wait)
> >> +{
> >> +	struct drm_crtc *crtc = file->f_inode->i_private;
> >> +	struct drm_crtc_crc *crc = &crtc->crc;
> >> +	unsigned ret;
> >> +
> >> +	poll_wait(file, &crc->wq, wait);
> >> +
> >> +	spin_lock_irq(&crc->lock);
> >> +	if (crc->source && crtc_crc_data_count(crc))
> >> +		ret = POLLIN;
> > Most places seem to also set POLLRDNORM. Maybe we should do it as well?
> >
> > Apart from that this seems good to me.
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Yeah, changed it and pushed, thanks for the suggestion. :)
> > Could replace the usleep() loop in igt read_one_crc() with
> > poll/select() I suppose? Either that or we should switch between
> > blocking and nonblocking dynamically.
> It could, but it would use 100% of cpu on older kernels that don't support poll(), if that's not a problem we could do it. :)

Maybe we can probe for poll support when we create the pipe_crc object?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/crc: Add support for polling on the data fd.

[Maarten Lankhorst]

Op 05-02-18 om 15:16 schreef Ville Syrjälä:
> On Mon, Feb 05, 2018 at 01:59:05PM +0100, Maarten Lankhorst wrote:
>> Op 02-02-18 om 15:46 schreef Ville Syrjälä:
>>> On Fri, Feb 02, 2018 at 03:27:43PM +0100, Maarten Lankhorst wrote:
>>>> This will make it possible for userspace to know whether reading
>>>> will block, without blocking on the fd. This makes it possible to
>>>> drain all queued CRC's in blocking mode, without having to reopen
>>>> the fd.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_debugfs_crc.c | 19 +++++++++++++++++++
>>>>  1 file changed, 19 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
>>>> index 9dd879589a2c..8af1a74ec64d 100644
>>>> --- a/drivers/gpu/drm/drm_debugfs_crc.c
>>>> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
>>>> @@ -307,10 +307,29 @@ static ssize_t crtc_crc_read(struct file *filep, char __user *user_buf,
>>>>  	return LINE_LEN(crc->values_cnt);
>>>>  }
>>>>  
>>>> +static unsigned int crtc_crc_poll(struct file *file, poll_table *wait)
>>>> +{
>>>> +	struct drm_crtc *crtc = file->f_inode->i_private;
>>>> +	struct drm_crtc_crc *crc = &crtc->crc;
>>>> +	unsigned ret;
>>>> +
>>>> +	poll_wait(file, &crc->wq, wait);
>>>> +
>>>> +	spin_lock_irq(&crc->lock);
>>>> +	if (crc->source && crtc_crc_data_count(crc))
>>>> +		ret = POLLIN;
>>> Most places seem to also set POLLRDNORM. Maybe we should do it as well?
>>>
>>> Apart from that this seems good to me.
>>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Yeah, changed it and pushed, thanks for the suggestion. :)
>>> Could replace the usleep() loop in igt read_one_crc() with
>>> poll/select() I suppose? Either that or we should switch between
>>> blocking and nonblocking dynamically.
>> It could, but it would use 100% of cpu on older kernels that don't support poll(), if that's not a problem we could do it. :)
> Maybe we can probe for poll support when we create the pipe_crc object?
>
I fear that will make a mess since you would need to support the fallback path anyway. I think blindly touching the fd with fcntl is better. :)

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