On 07/06/2019 14:44, Jani Nikula wrote:
On Fri, 07 Jun 2019, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
On 07/06/2019 14:11, Jani Nikula wrote:
On Fri, 07 Jun 2019, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
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@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
