| Message ID | 20180202142743.68527-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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. :)
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?
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. :)
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, };
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(+)