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 
 ---
  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 = >crc;
 +  unsigned ret;
 +
 +  poll_wait(file, >wq, wait);
 +
 +  spin_lock_irq(>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ä 
>> 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. :)

___
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.

[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 
> >> ---
> >>  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 = >crc;
> >> +  unsigned ret;
> >> +
> >> +  poll_wait(file, >wq, wait);
> >> +
> >> +  spin_lock_irq(>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ä 
> 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
___
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 
>> ---
>>  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 = >crc;
>> +unsigned ret;
>> +
>> +poll_wait(file, >wq, wait);
>> +
>> +spin_lock_irq(>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ä 
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. :)
___
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.

[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 
> ---
>  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 = >crc;
> + unsigned ret;
> +
> + poll_wait(file, >wq, wait);
> +
> + spin_lock_irq(>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ä 

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(>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