drm/crc: Add support for polling on the data fd.
diff mbox

Message ID 20180202142743.68527-1-maarten.lankhorst@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst Feb. 2, 2018, 2:27 p.m. UTC
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(+)

Comments

Ville Syrjälä Feb. 2, 2018, 2:46 p.m. UTC | #1
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
Maarten Lankhorst Feb. 5, 2018, 12:59 p.m. UTC | #2
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. :)
Ville Syrjälä Feb. 5, 2018, 2:16 p.m. UTC | #3
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?
Maarten Lankhorst Feb. 5, 2018, 2:52 p.m. UTC | #4
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. :)

Patch
diff mbox

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,
 };