Re: [Intel-gfx] [PATCH 47/47] drm/i915/guc: Unblock GuC submission on Gen11+

[Martin Peres <martin.peres@free.fr>]

On 02/07/2021 18:07, Michal Wajdeczko wrote:
> 
> 
> On 02.07.2021 10:09, Martin Peres wrote:
>> On 02/07/2021 10:29, Pekka Paalanen wrote:
>>> On Thu, 1 Jul 2021 21:28:06 +0200
>>> Daniel Vetter <daniel@ffwll.ch> wrote:
>>>
>>>> On Thu, Jul 1, 2021 at 8:27 PM Martin Peres <martin.peres@free.fr>
>>>> wrote:
>>>>>
>>>>> On 01/07/2021 11:14, Pekka Paalanen wrote:
>>>>>> On Wed, 30 Jun 2021 11:58:25 -0700
>>>>>> John Harrison <john.c.harrison@intel.com> wrote:
>>>>>>   
>>>>>>> On 6/30/2021 01:22, Martin Peres wrote:
>>>>>>>> On 24/06/2021 10:05, Matthew Brost wrote:
>>>>>>>>> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>>>>>>>
>>>>>>>>> Unblock GuC submission on Gen11+ platforms.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>>>>>>> Signed-off-by: Daniele Ceraolo Spurio
>>>>>>>>> <daniele.ceraolospurio@intel.com>
>>>>>>>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>>>>>>>>> ---
>>>>>>>>>      drivers/gpu/drm/i915/gt/uc/intel_guc.h            |  1 +
>>>>>>>>>      drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c |  8 ++++++++
>>>>>>>>>      drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h |  3 +--
>>>>>>>>>      drivers/gpu/drm/i915/gt/uc/intel_uc.c             | 14
>>>>>>>>> +++++++++-----
>>>>>>>>>      4 files changed, 19 insertions(+), 7 deletions(-)
>>>>>>>>>    
>>>>>>
>>>>>> ...
>>>>>>   
>>>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>>>>>>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>>>>>>>> index 7a69c3c027e9..61be0aa81492 100644
>>>>>>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>>>>>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>>>>>>>> @@ -34,8 +34,15 @@ static void uc_expand_default_options(struct
>>>>>>>>> intel_uc *uc)
>>>>>>>>>              return;
>>>>>>>>>          }
>>>>>>>>>      -    /* Default: enable HuC authentication only */
>>>>>>>>> -    i915->params.enable_guc = ENABLE_GUC_LOAD_HUC;
>>>>>>>>> +    /* Intermediate platforms are HuC authentication only */
>>>>>>>>> +    if (IS_DG1(i915) || IS_ALDERLAKE_S(i915)) {
>>>>>>>>> +        drm_dbg(&i915->drm, "Disabling GuC only due to old
>>>>>>>>> platform\n");
>>>>>>>>
>>>>>>>> This comment does not seem accurate, given that DG1 is barely
>>>>>>>> out, and
>>>>>>>> ADL is not out yet. How about:
>>>>>>>>
>>>>>>>> "Disabling GuC on untested platforms"?
>>>>>>>>    
>>>>>>> Just because something is not in the shops yet does not mean it is
>>>>>>> new.
>>>>>>> Technology is always obsolete by the time it goes on sale.
>>>>>>
>>>>>> That is a very good reason to not use terminology like "new", "old",
>>>>>> "current", "modern" etc. at all.
>>>>>>
>>>>>> End users like me definitely do not share your interpretation of
>>>>>> "old".
>>>>>
>>>>> Yep, old and new is relative. In the end, what matters is the
>>>>> validation
>>>>> effort, which is why I was proposing "untested platforms".
>>>>>
>>>>> Also, remember that you are not writing these messages for Intel
>>>>> engineers, but instead are writing for Linux *users*.
>>>>
>>>> It's drm_dbg. Users don't read this stuff, at least not users with no
>>>> clue what the driver does and stuff like that.
>>>
>>> If I had a problem, I would read it, and I have no clue what anything
>>> of that is.
>>
>> Exactly.
>>
>> This level of defense for what is clearly a bad *debug* message (at the
>> very least, the grammar) makes no sense at all!
>>
>> I don't want to hear arguments like "Not my patch" from a developer
>> literally sending the patch to the ML and who added his SoB to the
>> patch, playing with words, or minimizing the problem of having such a
>> message.
> 
> Agree that 'not my patch' is never a good excuse, but equally we can't
> blame original patch author as patch was updated few times since then.

I never wanted to blame the author here, I was only speaking about the 
handling of feedback on the patch.

> 
> Maybe to avoid confusions and simplify reviews, we could split this
> patch into two smaller: first one that really unblocks GuC submission on
> all Gen11+ (see __guc_submission_supported) and second one that updates
> defaults for Gen12+ (see uc_expand_default_options), as original patch
> (from ~2019) evolved more than what title/commit message says.

Both work for me, as long as it is a collaborative effort.

Cheers,
Martin

> 
> Then we can fix all messaging and make sure it's clear and understood.
> 
> Thanks,
> Michal
> 
>>
>> All of the above are just clear signals for the community to get off
>> your playground, which is frankly unacceptable. Your email address does
>> not matter.
>>
>> In the spirit of collaboration, your response should have been "Good
>> catch, how about XXXX or YYYY?". This would not have wasted everyone's
>> time in an attempt to just have it your way.
>>
>> My level of confidence in this GuC transition was already low, but you
>> guys are working hard to shoot yourself in the foot. Trust should be
>> earned!
>>
>> Martin
>>
>>>
>>>
>>> Thanks,
>>> pq
>>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx