[Intel-gfx] [PATCH v8 1/6] drm/i915 : Unifying seq_puts messages for feature support

[Sujaritha]

On 10/29/2017 09:49 PM, Sagar Arun Kamble wrote:
>
>
> On 10/26/2017 11:24 PM, Daniele Ceraolo Spurio wrote:
>>
>>
>> On 25/10/17 06:31, Michal Wajdeczko wrote:
>>> On Tue, 24 Oct 2017 19:21:20 +0200, Sujaritha Sundaresan 
>>> <sujaritha.sundaresan at intel.com> wrote:
>>>
>>>> Unifying the various seq_puts messages in debugfs to the simplest 
>>>> one for
>>>> feature support.
> As Michal noted in the v7 review, if the goal is to unification of 
> consistent output then I see some more in the debugfs that
> might need to be updated: *_wm_latency_open, i915_ipc_status, 
> i915_runtime_pm_status(returning early could be done later)
> i915_llc (add change to return early).
> Also, I think this patch can be separated from this series as it has 
> very little dependency.

Sure, I will include the other updates to the patch. But I feel that it 
might be better to keep
the patch with series, since I'm including the change to HAS_GUC here.
>>>>
>>>> v2: Clarifying the commit message (Anusha)
>>>>
>>>> v3: Re-factoring code as per review (Michal)
>>>>
>>>> v4: Rebase
>>>>
>>>> v5: Split from following patch
>>>>
>>>> v6: Re-factoring code (Michal, Sagar)
>>>>     Clarifying commit message (Sagar)
>>>>
>>>> v7: Generalizing subject to drm/i915 (Sagar)
>>>>
>>>> v8: Omitting DRRS seq_puts unification (Michal)
>>>>
>>>> Suggested by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan at intel.com>
>>>> Cc: Anusha Srivatsa <anusha.srivatsa at intel.com>
>>>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>>> Cc: Oscar Mateo <oscar.mateo at intel.com>
>>>> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++++-----
>>>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>>> index c65e381..8edd029 100644
>>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>>> @@ -1641,7 +1641,7 @@ static int i915_fbc_status(struct seq_file 
>>>> *m, void *unused)
>>>>      struct drm_i915_private *dev_priv = node_to_i915(m->private);
>>>>     if (!HAS_FBC(dev_priv)) {
>>>> -        seq_puts(m, "FBC unsupported on this chipset\n");
>>>> +        seq_puts(m, "not supported\n");
>>>>          return 0;
>>>>      }
>>>> @@ -1809,7 +1809,7 @@ static int i915_ring_freq_table(struct 
>>>> seq_file *m, void *unused)
>>>>      unsigned int max_gpu_freq, min_gpu_freq;
>>>>     if (!HAS_LLC(dev_priv)) {
>>>> -        seq_puts(m, "unsupported on this chipset\n");
>>>> +        seq_puts(m, "not supported\n");
>>>>          return 0;
>>>>      }
>>>> @@ -2361,8 +2361,10 @@ static int i915_huc_load_status_info(struct 
>>>> seq_file *m, void *data)
>>>>      struct drm_i915_private *dev_priv = node_to_i915(m->private);
>>>>      struct drm_printer p;
>>>> -    if (!HAS_HUC_UCODE(dev_priv))
>>>> +    if (!HAS_GUC(dev_priv)) {
>>>
>>> Hmm, I think that in above code we should use HAS_HUC defined as:
>>>
>>> /* HuC is inherent part of the GuC ... */
>>> #define HAS_HUC(dev_priv)    HAS_GUC(dev_priv)
>>>
>>> to make it clear that code checks HuC sub-feature (not other part
>>> of the GuC or GuC itself). And additionally we can use above define
>>> to explicitly document relation between GuC and HuC.
>>>
>>> Michal
>>>
>>
>> The suggested comment feels confusing to me. HuC is a different 
>> microcontroller and not a part of GuC. The only tie the 2 have is 
>> that GuC needs to do the authentication. It is however true that any 
>> platform that has a GuC also has a HuC so the suggested define itself 
>> is fine.
>>
>> Daniele
>>
>>>> +        seq_puts(m, "not supported\n");
>>>>          return 0;
>>>> +    }
>>>>     p = drm_seq_file_printer(m);
>>>>      intel_uc_fw_dump(&dev_priv->huc.fw, &p);
>>>> @@ -2380,8 +2382,10 @@ static int i915_guc_load_status_info(struct 
>>>> seq_file *m, void *data)
>>>>      struct drm_printer p;
>>>>      u32 tmp, i;
>>>> -    if (!HAS_GUC_UCODE(dev_priv))
>>>> +    if (!HAS_GUC(dev_priv)) {
>>>> +        seq_puts(m, "not supported\n");
>>>>          return 0;
>>>> +    }
>>>>     p = drm_seq_file_printer(m);
>>>>      intel_uc_fw_dump(&dev_priv->guc.fw, &p);
>>>> @@ -2650,7 +2654,7 @@ static int i915_edp_psr_status(struct 
>>>> seq_file *m, void *data)
>>>>      bool enabled = false;
>>>>     if (!HAS_PSR(dev_priv)) {
>>>> -        seq_puts(m, "PSR not supported\n");
>>>> +        seq_puts(m, "not supported\n");
>>>>          return 0;
>>>>      }
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Thanks for the review,

Regards,
Sujaritha