Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 27 Jan 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 26/01/2023 16:05, Jani Nikula wrote:
>> On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
>>> On Thu, 2023-01-26 at 14:11 +0200, Luca Coelho wrote:
>>>> On Thu, 2023-01-26 at 14:00 +0200, Jani Nikula wrote:
>>>>> On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
>>>>>> On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
>>>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>>>>>>>> index 7d4a15a283a0..63b79c611932 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>>>>>>>> @@ -1559,7 +1559,26 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
>>>>>>>>   	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
>>>>>>>>   }
>>>>>>>>   
>>>>>>>> -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
>>>>>>>> +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
>>>>>>>> +					const struct intel_crtc_state *crtc_state,
>>>>>>>> +					const struct intel_plane_state *plane_state,
>>>>>>>> +					int color_plane)
>>>>>>>> +{
>>>>>>>> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>>>>>>
>>>>>> Should you use i915 instead of dev_priv? I've heard and read elsewhere
>>>>>> that this is generally a desired change.  Much easier to use always the
>>>>>> same local name for this kind of thing.  Though this file is already
>>>>>> interspersed with both versions...
>>>>>
>>>>> Basically the only reason to use dev_priv for new code is to deal with
>>>>> some register macros that still have implicit dev_priv in
>>>>> them. Otherwise, i915 should be used, and when convenient, dev_priv
>>>>> should be converted to i915 while touching the code anyway (in a
>>>>> separate patch, but while you're there).
>>>>
>>>> Thanks for the clarification! In this case we're not using any of the
>>>> macros, AFAICT, so I guess it's better to go with i915 already? And I
>>>> think it should even be in this same patch, since it's a new function
>>>> anyway.
>>>>
>>>>
>>>>> The implicit dev_priv dependencies in the register macros are a bit
>>>>> annoying to fix, and it's been going slow. In retrospect maybe the right
>>>>> thing would have been to just sed the parameter to all of them
>>>>> everywhere and be done with it for good. Not too late now, I guess, and
>>>>> I'd take the patches in a heartbeat if someone were to step up and do
>>>>> it.
>>>>
>>>> I see that there is a boatload of register macros using it... I won't
>>>> promise, but I think it would be a good exercise for a n00b like me to
>>>> make this patch, though I already foresee another boatload of conflicts
>>>> with the internal trees and everything...
>>>
>>> There were actually 10 boatloads of places to change:
>>>
>>>   187 files changed, 12104 insertions(+), 12104 deletions(-)
>>>
>>> ...but it _does_ compile. 😄
>>>
>>> Do you think this is fine? Lots of shuffle, but if you think it's okay,
>>> I can send the patch out now.
>> 
>> Heh, I said I'd take patchES, not everything together! ;)
>> 
>> Rodrigo, Tvrtko, Joonas, thoughts?
>
> IMO if the elimination of implicit dev_priv is not included then I am 
> not sure the churn is worth the effort.
>
> I think one trap is that it is easy to assume solving those conflicts is 
> easy because there is a script, somewhere, whatever, but one needs to be 
> careful with assuming a random person hitting a merge conflict will 
> realize there is a script, know where to find it, and know how to use it 
> against a state where conflict markers are sitting in their local tree. 
> That's a lot of assumed knowledge which my experience tells me is not 
> universally there.
>
> Having said all that, I looked at the occurrence histogram for the 
> proposed churn and gut feel says conflicts wouldn't even be that bad 
> since they seem heavily localized in a handful of files plus the display 
> subdir.
>
> Plus it is upstream.. so we are allowed not to care too much about 
> backporting woes. I would still hope implicit dev_priv, albeit 
> orthogonal, would be coming somewhat together with the rename. For that 
> warm fuzzy feeling that the churn was really really worth it.

I was mostly talking about the implicit dev_priv removal. It's somewhat
easy, because you can always assume dev_priv is around when the macros
in question are used.

The above is a dependency to any renames. I don't think the renames are
as important as removing the implicit dev_priv, and the renames are
easier to handle piecemeal, say a file at a time or something.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center