[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

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