| Message ID | 20180702110729.8171-3-mahesh1.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Mahesh, Thank you for the patch. On Monday, 2 July 2018 14:07:21 EEST Mahesh Kumar wrote: > This patch introduce a callback function "get_crc_sources" which > will be called during read of control node. It is an optional > callback function and if driver implements this callback, driver > should print list of available CRC sources in seq_file privided > as an input to the callback. The commit message seems to be outdated, the callback doesn't take a seq_file anymore. > Changes Since V1: (Daniel) > - return const pointer to an array of crc sources list > - do validation of sources in CRC-core > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> > Cc: dri-devel@lists.freedesktop.org > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/drm_debugfs_crc.c | 20 +++++++++++++++++++- > include/drm/drm_crtc.h | 16 ++++++++++++++++ > 2 files changed, 35 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_debugfs_crc.c > b/drivers/gpu/drm/drm_debugfs_crc.c index c6a725b79ac6..f4d76528d24c 100644 > --- a/drivers/gpu/drm/drm_debugfs_crc.c > +++ b/drivers/gpu/drm/drm_debugfs_crc.c > @@ -67,9 +67,27 @@ > static int crc_control_show(struct seq_file *m, void *data) > { > struct drm_crtc *crtc = m->private; > + size_t count; Count it only used within the if () {} block, you can declare it there. > + > + if (crtc->funcs->get_crc_sources) { > + const char *const *sources = crtc->funcs->get_crc_sources(crtc, > + &count); > + size_t values_cnt; > + int i; I only takes positive values, it can be an unsigned int. > + > + if (count <= 0 || !sources) count is a size_t, it can't be negative. The .get_crc_sources() documentation doesn't clearly specify whether sources should always be NULL when count is zero. I advise updating the documentation, and possibly updating this check accordingly. > + goto out; > + > + seq_puts(m, "["); > + for (i = 0; i < count; i++) > + if (!crtc->funcs->verify_crc_source(crtc, sources[i], > + &values_cnt)) I assume that you verify sources one by one here to avoid having to create a list of sources dynamically in the .get_crc_sources() callback ? If so, I think the .get_crc_sources() callback should document that. You should also document that .verify_crc_source() is required when get_crc_sources() is provided. > + seq_printf(m, "%s ", sources[i]); > + seq_puts(m, "] "); This assumes that source names can't include a space. Isn't that too restrictive ? Shouldn't a different separator be used ? How about one source name per line ? Additionally, shouldn't the active source be marked ? > + } > > +out: > seq_printf(m, "%s\n", crtc->crc.source); > - > return 0; > } > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 1a6dcbf91744..ffaec138aeee 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -676,6 +676,22 @@ struct drm_crtc_funcs { > */ > int (*verify_crc_source)(struct drm_crtc *crtc, const char *source, > size_t *values_cnt); > + /** > + * @get_crc_sources: > + * > + * Driver callback for getting a list of all the available sources for > + * CRC generation. > + * > + * This callback is optional if the driver does not support exporting of > + * possible CRC sources list. CRC-core does the verification of sources. > + * > + * RETURNS: > + * > + * a constant character pointer to the list of all the available CRC > + * sources > + */ > + const char *const *(*get_crc_sources)(struct drm_crtc *crtc, > + size_t *count); > > /** > * @atomic_print_state:
Hi, thanks for the review. On 7/10/2018 4:52 PM, Laurent Pinchart wrote: > Hi Mahesh, > > Thank you for the patch. > > On Monday, 2 July 2018 14:07:21 EEST Mahesh Kumar wrote: >> This patch introduce a callback function "get_crc_sources" which >> will be called during read of control node. It is an optional >> callback function and if driver implements this callback, driver >> should print list of available CRC sources in seq_file privided >> as an input to the callback. > The commit message seems to be outdated, the callback doesn't take a seq_file > anymore. ops, will update. > >> Changes Since V1: (Daniel) >> - return const pointer to an array of crc sources list >> - do validation of sources in CRC-core >> >> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> >> Cc: dri-devel@lists.freedesktop.org >> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/drm_debugfs_crc.c | 20 +++++++++++++++++++- >> include/drm/drm_crtc.h | 16 ++++++++++++++++ >> 2 files changed, 35 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c >> b/drivers/gpu/drm/drm_debugfs_crc.c index c6a725b79ac6..f4d76528d24c 100644 >> --- a/drivers/gpu/drm/drm_debugfs_crc.c >> +++ b/drivers/gpu/drm/drm_debugfs_crc.c >> @@ -67,9 +67,27 @@ >> static int crc_control_show(struct seq_file *m, void *data) >> { >> struct drm_crtc *crtc = m->private; >> + size_t count; > Count it only used within the if () {} block, you can declare it there. agree. > >> + >> + if (crtc->funcs->get_crc_sources) { >> + const char *const *sources = crtc->funcs->get_crc_sources(crtc, >> + &count); >> + size_t values_cnt; >> + int i; > I only takes positive values, it can be an unsigned int. ok > >> + >> + if (count <= 0 || !sources) > count is a size_t, it can't be negative. > > The .get_crc_sources() documentation doesn't clearly specify whether sources > should always be NULL when count is zero. I advise updating the documentation, > and possibly updating this check accordingly. ok will update. > >> + goto out; >> + >> + seq_puts(m, "["); >> + for (i = 0; i < count; i++) >> + if (!crtc->funcs->verify_crc_source(crtc, sources[i], >> + &values_cnt)) > I assume that you verify sources one by one here to avoid having to create a > list of sources dynamically in the .get_crc_sources() callback ? If so, I > think the .get_crc_sources() callback should document that. > > You should also document that .verify_crc_source() is required when > get_crc_sources() is provided. ok sure. > >> + seq_printf(m, "%s ", sources[i]); >> + seq_puts(m, "] "); > This assumes that source names can't include a space. Isn't that too > restrictive ? Shouldn't a different separator be used ? How about one source > name per line ? what about comma separated as I'm putting names inside square-brackets? > > Additionally, shouldn't the active source be marked ? active source is again printed by the code in next few lines. output will be of following format. [space separated list of valid sources] active_source -Mahesh > >> + } >> >> +out: >> seq_printf(m, "%s\n", crtc->crc.source); >> - >> return 0; >> } >> >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >> index 1a6dcbf91744..ffaec138aeee 100644 >> --- a/include/drm/drm_crtc.h >> +++ b/include/drm/drm_crtc.h >> @@ -676,6 +676,22 @@ struct drm_crtc_funcs { >> */ >> int (*verify_crc_source)(struct drm_crtc *crtc, const char *source, >> size_t *values_cnt); >> + /** >> + * @get_crc_sources: >> + * >> + * Driver callback for getting a list of all the available sources for >> + * CRC generation. >> + * >> + * This callback is optional if the driver does not support exporting of >> + * possible CRC sources list. CRC-core does the verification of sources. >> + * >> + * RETURNS: >> + * >> + * a constant character pointer to the list of all the available CRC >> + * sources >> + */ >> + const char *const *(*get_crc_sources)(struct drm_crtc *crtc, >> + size_t *count); >> >> /** >> * @atomic_print_state:
Hi Mahesh, On Tuesday, 10 July 2018 15:01:38 EEST Kumar, Mahesh wrote: > On 7/10/2018 4:52 PM, Laurent Pinchart wrote: > > Hi Mahesh, > > On Monday, 2 July 2018 14:07:21 EEST Mahesh Kumar wrote: > >> This patch introduce a callback function "get_crc_sources" which > >> will be called during read of control node. It is an optional > >> callback function and if driver implements this callback, driver > >> should print list of available CRC sources in seq_file privided > >> as an input to the callback. > > > > The commit message seems to be outdated, the callback doesn't take a > > seq_file anymore. > > ops, will update. > > >> Changes Since V1: (Daniel) > >> > >> - return const pointer to an array of crc sources list > >> - do validation of sources in CRC-core > >> > >> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> > >> Cc: dri-devel@lists.freedesktop.org > >> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > >> > >> drivers/gpu/drm/drm_debugfs_crc.c | 20 +++++++++++++++++++- > >> include/drm/drm_crtc.h | 16 ++++++++++++++++ > >> 2 files changed, 35 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c > >> b/drivers/gpu/drm/drm_debugfs_crc.c index c6a725b79ac6..f4d76528d24c > >> 100644 > >> --- a/drivers/gpu/drm/drm_debugfs_crc.c > >> +++ b/drivers/gpu/drm/drm_debugfs_crc.c > >> @@ -67,9 +67,27 @@ > >> > >> static int crc_control_show(struct seq_file *m, void *data) > >> { > >> > >> struct drm_crtc *crtc = m->private; > >> > >> + size_t count; > > > > Count it only used within the if () {} block, you can declare it there. > > agree. > > >> + > >> + if (crtc->funcs->get_crc_sources) { > >> + const char *const *sources = crtc->funcs->get_crc_sources(crtc, > >> + &count); > >> + size_t values_cnt; > >> + int i; > > > > I only takes positive values, it can be an unsigned int. > > ok > > >> + > >> + if (count <= 0 || !sources) > > > > count is a size_t, it can't be negative. > > > > The .get_crc_sources() documentation doesn't clearly specify whether > > sources should always be NULL when count is zero. I advise updating the > > documentation, and possibly updating this check accordingly. > > ok will update. > > >> + goto out; > >> + > >> + seq_puts(m, "["); > >> + for (i = 0; i < count; i++) > >> + if (!crtc->funcs->verify_crc_source(crtc, sources[i], > >> + &values_cnt)) > > > > I assume that you verify sources one by one here to avoid having to create > > a list of sources dynamically in the .get_crc_sources() callback ? If so, > > I think the .get_crc_sources() callback should document that. > > > > You should also document that .verify_crc_source() is required when > > get_crc_sources() is provided. > > ok sure. > > >> + seq_printf(m, "%s ", sources[i]); > >> + seq_puts(m, "] "); > > > > This assumes that source names can't include a space. Isn't that too > > restrictive ? Shouldn't a different separator be used ? How about one > > source name per line ? > > what about comma separated as I'm putting names inside square-brackets? > > > Additionally, shouldn't the active source be marked ? > > active source is again printed by the code in next few lines. output > will be of following format. > [space separated list of valid sources] active_source I had missed that, my bad. The proposed format seems a bit hackish to me, in the sense that it forbids both spaces and brackets in source names. One source per line would fix both and be easy to parse. We would then need to mark the active source, which could be done by adding a marker to the corresponding line (maybe a * at the end of the line ?). > >> + } > >> > >> +out: > >> seq_printf(m, "%s\n", crtc->crc.source); > >> - > >> return 0; > >> } [snip]
Hi, On 7/10/2018 5:39 PM, Laurent Pinchart wrote: > Hi Mahesh, > > On Tuesday, 10 July 2018 15:01:38 EEST Kumar, Mahesh wrote: >> On 7/10/2018 4:52 PM, Laurent Pinchart wrote: >>> Hi Mahesh, >>> On Monday, 2 July 2018 14:07:21 EEST Mahesh Kumar wrote: >>>> This patch introduce a callback function "get_crc_sources" which >>>> will be called during read of control node. It is an optional >>>> callback function and if driver implements this callback, driver >>>> should print list of available CRC sources in seq_file privided >>>> as an input to the callback. >>> The commit message seems to be outdated, the callback doesn't take a >>> seq_file anymore. >> ops, will update. >> >>>> Changes Since V1: (Daniel) >>>> >>>> - return const pointer to an array of crc sources list >>>> - do validation of sources in CRC-core >>>> >>>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> >>>> Cc: dri-devel@lists.freedesktop.org >>>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> --- >>>> >>>> drivers/gpu/drm/drm_debugfs_crc.c | 20 +++++++++++++++++++- >>>> include/drm/drm_crtc.h | 16 ++++++++++++++++ >>>> 2 files changed, 35 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c >>>> b/drivers/gpu/drm/drm_debugfs_crc.c index c6a725b79ac6..f4d76528d24c >>>> 100644 >>>> --- a/drivers/gpu/drm/drm_debugfs_crc.c >>>> +++ b/drivers/gpu/drm/drm_debugfs_crc.c >>>> @@ -67,9 +67,27 @@ >>>> >>>> static int crc_control_show(struct seq_file *m, void *data) >>>> { >>>> >>>> struct drm_crtc *crtc = m->private; >>>> >>>> + size_t count; >>> Count it only used within the if () {} block, you can declare it there. >> agree. >> >>>> + >>>> + if (crtc->funcs->get_crc_sources) { >>>> + const char *const *sources = crtc->funcs->get_crc_sources(crtc, >>>> + &count); >>>> + size_t values_cnt; >>>> + int i; >>> I only takes positive values, it can be an unsigned int. >> ok >> >>>> + >>>> + if (count <= 0 || !sources) >>> count is a size_t, it can't be negative. >>> >>> The .get_crc_sources() documentation doesn't clearly specify whether >>> sources should always be NULL when count is zero. I advise updating the >>> documentation, and possibly updating this check accordingly. >> ok will update. >> >>>> + goto out; >>>> + >>>> + seq_puts(m, "["); >>>> + for (i = 0; i < count; i++) >>>> + if (!crtc->funcs->verify_crc_source(crtc, sources[i], >>>> + &values_cnt)) >>> I assume that you verify sources one by one here to avoid having to create >>> a list of sources dynamically in the .get_crc_sources() callback ? If so, >>> I think the .get_crc_sources() callback should document that. >>> >>> You should also document that .verify_crc_source() is required when >>> get_crc_sources() is provided. >> ok sure. >> >>>> + seq_printf(m, "%s ", sources[i]); >>>> + seq_puts(m, "] "); >>> This assumes that source names can't include a space. Isn't that too >>> restrictive ? Shouldn't a different separator be used ? How about one >>> source name per line ? >> what about comma separated as I'm putting names inside square-brackets? >> >>> Additionally, shouldn't the active source be marked ? >> active source is again printed by the code in next few lines. output >> will be of following format. >> [space separated list of valid sources] active_source > I had missed that, my bad. > > The proposed format seems a bit hackish to me, in the sense that it forbids > both spaces and brackets in source names. One source per line would fix both > and be easy to parse. We would then need to mark the active source, which > could be done by adding a marker to the corresponding line (maybe a * at the > end of the line ?). sounds good, will do that. -Mahesh > >>>> + } >>>> >>>> +out: >>>> seq_printf(m, "%s\n", crtc->crc.source); >>>> - >>>> return 0; >>>> } > [snip] >
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c index c6a725b79ac6..f4d76528d24c 100644 --- a/drivers/gpu/drm/drm_debugfs_crc.c +++ b/drivers/gpu/drm/drm_debugfs_crc.c @@ -67,9 +67,27 @@ static int crc_control_show(struct seq_file *m, void *data) { struct drm_crtc *crtc = m->private; + size_t count; + + if (crtc->funcs->get_crc_sources) { + const char *const *sources = crtc->funcs->get_crc_sources(crtc, + &count); + size_t values_cnt; + int i; + + if (count <= 0 || !sources) + goto out; + + seq_puts(m, "["); + for (i = 0; i < count; i++) + if (!crtc->funcs->verify_crc_source(crtc, sources[i], + &values_cnt)) + seq_printf(m, "%s ", sources[i]); + seq_puts(m, "] "); + } +out: seq_printf(m, "%s\n", crtc->crc.source); - return 0; } diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 1a6dcbf91744..ffaec138aeee 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -676,6 +676,22 @@ struct drm_crtc_funcs { */ int (*verify_crc_source)(struct drm_crtc *crtc, const char *source, size_t *values_cnt); + /** + * @get_crc_sources: + * + * Driver callback for getting a list of all the available sources for + * CRC generation. + * + * This callback is optional if the driver does not support exporting of + * possible CRC sources list. CRC-core does the verification of sources. + * + * RETURNS: + * + * a constant character pointer to the list of all the available CRC + * sources + */ + const char *const *(*get_crc_sources)(struct drm_crtc *crtc, + size_t *count); /** * @atomic_print_state: