Re: [RFC 07/12] drm/i915: Remove I915_READ8

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 07/06/2019 14:44, Jani Nikula wrote:
> On Fri, 07 Jun 2019, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>> On 07/06/2019 14:11, Jani Nikula wrote:
>>> On Fri, 07 Jun 2019, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> Only a few call sites remain which have been converted to uncore mmio
>>>> accessors and so the macro can be removed.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> I have some reservations about this one and patch 11. Again, I'm fine
>>> with nuking I915_READ8 and I915_READ_NOTRACE (in patch 11). I think
>>> they're special cases and it's okay if they grow into a bit longer lines
>>> or multiple lines.
>>>
>>> The problem is converting regular I915_READ and I915_WRITE in display
>>> code while at it.
>>>
>>> I don't think en masse conversion of them to intel_uncore_read and
>>> intel_uncore_write directly is going to happen in display code, because
>>> there's too much code that gets turned too ugly with the increase in
>>> function name length and the extra passed parameter. I think many of
>>> those places are pretty ugly as it is already. That's my feeling anyway.
>>>
>>> I understand your reasoning is to avoid the mixed use of intel_uncore_*
>>
>> Exactly.
>>
>>> and I915_*. But I fear using intel_uncore_read and intel_uncore_write
>>> now is going to promote their use all over the place, and *that* will
>>> lead to mixed use. Which is not optimal if the overall feeling is that
>>> we need to come up with a shorter function/macro for display code reads
>>> and writes.
>>
>> I am fine with the argument that you may desire something shorter for
>> display. But I don't think converting whole functions would create any
>> more future mixed use than converting functions partially.
>>
>> Btw have you seen the latest RFC from Daniele?
> 
> Yes, but haven't had the time to digest it.
> 
>> That would allow that you
>> only replace the assignemnts at the top of functions perhaps like from:
>>
>> 	struct intel_uncore *uncore = &dev_priv->uncore;
>>
>> to:
>>
>> 	struct intel_uncore *uncore = &dev_priv->display_uncore;
>>
>> But sure, if you desire shorter accessors then lets first have a
>> definitive decision of that.
> 
> If there were display accessors they might just take i915 as pointer:
> 
> #define FOO_READ(i915, reg) intel_uncore_read(&i915->display_uncore, reg)
> 
> Dunno.
> 
>>
>>> I presume Ville has something to say about the gt vs. display stuff as
>>> well; does the whole series make that harder? I hope not, because I do
>>> like the rest of what's being done here.
>>
>> It doesn't make it harder. I can easily drop the bits you don't like if
>> that will be the final decision. In fact, I should probably do that
>> straight away if that would unblock the remaining two patches because
>> then I can proceed with other refactorings on top.
> 
> Hum, you know what, it's not *that* big of a deal. Ack on the whole
> series, we can tidy up later on if needed.
> 
> I guess I'd like to get Ville's ack on the series too. Ville?

So I have patches 7 & 11 with acks from Jani and no r-b here.

Depending on the opinion from Ville I can either ask for upgrade to r-b, 
or refactor them to minimize intel_uncore_read/write in favour of 
leaving the affected functions mixed (I915_READ/WRITE + 
intel_uncore_"more-exotic-accessors".

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx