Re: [PATCH 01/31] drm/i915/debugfs: Create generic string tokenize function and update CRC control parsing

Sagar Arun Kamble
2017-09-28T09:10:48Z

Thanks for the review Michal. Will update as suggested.

On 9/21/2017 8:42 PM, Michal Wajdeczko wrote:
> On Tue, 19 Sep 2017 19:41:37 +0200, Sagar Arun Kamble
> <sagar.a.kamble@intel.com> wrote:
>
>> Input string parsing used in CRC control parameter parsing is generic
>> and can be reused for other debugfs interfaces. Hence name it as
>> buffer_tokenize instead of tieing to display_crc. Also fix the function
>> desciption for CRC control parsing that was misplaced at tokenize
>> function.
>>
>> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Acked-by: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h       |  1 +
>>  drivers/gpu/drm/i915/intel_pipe_crc.c | 88
>> +++++++++++++++++------------------
>>  2 files changed, 45 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 6d7d871..4d5ffde 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3847,6 +3847,7 @@ u32 i915_gem_fence_alignment(struct
>> drm_i915_private *dev_priv, u32 size,
>>  int i915_debugfs_register(struct drm_i915_private *dev_priv);
>>  int i915_debugfs_connector_add(struct drm_connector *connector);
>>  void intel_display_crc_init(struct drm_i915_private *dev_priv);
>> +int buffer_tokenize(char *buf, char *words[], int max_words);
>>  #else
>>  static inline int i915_debugfs_register(struct drm_i915_private
>> *dev_priv) {return 0;}
>>  static inline int i915_debugfs_connector_add(struct drm_connector
>> *connector)
>> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
>> b/drivers/gpu/drm/i915/intel_pipe_crc.c
>> index 96043a5..2e312b8 100644
>> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
>> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
>> @@ -710,49 +710,6 @@ static int pipe_crc_set_source(struct
>> drm_i915_private *dev_priv,
>>      return ret;
>>  }
>> -/*
>> - * Parse pipe CRC command strings:
>> - *   command: wsp* object wsp+ name wsp+ source wsp*
>> - *   object: 'pipe'
>> - *   name: (A | B | C)
>> - *   source: (none | plane1 | plane2 | pf)
>> - *   wsp: (#0x20 | #0x9 | #0xA)+
>> - *
>> - * eg.:
>> - *  "pipe A plane1"  ->  Start CRC computations on plane1 of pipe A
>> - *  "pipe A none"    ->  Stop CRC
>> - */
>> -static int display_crc_ctl_tokenize(char *buf, char *words[], int
>> max_words)
>> -{
>> -    int n_words = 0;
>> -
>> -    while (*buf) {
>> -        char *end;
>> -
>> -        /* skip leading white space */
>> -        buf = skip_spaces(buf);
>> -        if (!*buf)
>> -            break;    /* end of buffer */
>> -
>> -        /* find end of word */
>> -        for (end = buf; *end && !isspace(*end); end++)
>> -            ;
>> -
>> -        if (n_words == max_words) {
>> -            DRM_DEBUG_DRIVER("too many words, allowed <= %d\n",
>> -                     max_words);
>> -            return -EINVAL;    /* ran out of words[] before bytes */
>> -        }
>> -
>> -        if (*end)
>> -            *end++ = '\0';
>> -        words[n_words++] = buf;
>> -        buf = end;
>> -    }
>> -
>> -    return n_words;
>> -}
>> -
>>  enum intel_pipe_crc_object {
>>      PIPE_CRC_OBJECT_PIPE,
>>  };
>> @@ -806,6 +763,49 @@ static int display_crc_ctl_parse_pipe(const char
>> *buf, enum pipe *pipe)
>>      return -EINVAL;
>>  }
>> +int buffer_tokenize(char *buf, char *words[], int max_words)
>> +{
>> +    int n_words = 0;
>> +
>> +    while (*buf) {
>> +        char *end;
>> +
>> +        /* skip leading white space */
>> +        buf = skip_spaces(buf);
>> +        if (!*buf)
>> +            break;    /* end of buffer */
>> +
>> +        /* find end of word */
>> +        for (end = buf; *end && !isspace(*end); end++)
>> +            ;
>> +
>> +        if (n_words == max_words) {
>> +            DRM_DEBUG_DRIVER("too many words, allowed <= %d\n",
>> +                     max_words);
>> +            return -EINVAL;    /* ran out of words[] before bytes */
>> +        }
>> +
>> +        if (*end)
>> +            *end++ = '\0';
>> +        words[n_words++] = buf;
>> +        buf = end;
>> +    }
>> +
>> +    return n_words;
>> +}
>
> You should move this function to i915_debugfs.c
>
>> +
>> +/*
>> + * Parse pipe CRC command strings:
>> + *   command: wsp* object wsp+ name wsp+ source wsp*
>> + *   object: 'pipe'
>> + *   name: (A | B | C)
>> + *   source: (none | plane1 | plane2 | pf)
>> + *   wsp: (#0x20 | #0x9 | #0xA)+
>> + *
>> + * eg.:
>> + *  "pipe A plane1"  ->  Start CRC computations on plane1 of pipe A
>> + *  "pipe A none"    ->  Stop CRC
>> + */
>>  static int display_crc_ctl_parse(struct drm_i915_private *dev_priv,
>>                   char *buf, size_t len)
>>  {
>> @@ -816,7 +816,7 @@ static int display_crc_ctl_parse(struct
>> drm_i915_private *dev_priv,
>>      enum intel_pipe_crc_object object;
>>      enum intel_pipe_crc_source source;
>> -    n_words = display_crc_ctl_tokenize(buf, words, N_WORDS);
>> +    n_words = buffer_tokenize(buf, words, N_WORDS);
>>      if (n_words != N_WORDS) {
>>          DRM_DEBUG_DRIVER("tokenize failed, a command is %d words\n",
>>                   N_WORDS);