[Intel-gfx] RMW considered harmful (was: Re: [PATCH 2/2] drm/i915/icl: Enable TRANSCODER PORT SYNC for tiled displays across separate ports)

[Jani Nikula]

On Fri, 22 Mar 2019, Manasi Navare <manasi.d.navare at intel.com> wrote:
> On Fri, Mar 22, 2019 at 09:28:01PM +0200, Jani Nikula wrote:
>> On Fri, 22 Mar 2019, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
>> > On Fri, Mar 22, 2019 at 11:44:21AM -0700, Manasi Navare wrote:
>> >> On Fri, Mar 22, 2019 at 08:09:50PM +0200, Ville Syrjälä wrote:
>> >> > In that case there is no point in doing a rmw.
>> >> 
>> >> But isnt it always a good idea to do rmw? I mean what if the master
>> >> select was set to something else earlier?
>> >
>> > RMW is the root of many evils. It should be avoided unless there is a
>> > really compelling reason to use it.
>> 
>> Hear, hear!
>> 
>> We have the software state that we want to write to the hardware. If we
>> use RMW to do this, it might all work by coincidence due to the old
>> values in the registers, or it might just as well break by coincidence
>> due to some garbage in the registers.
>> 
>> In most cases, there should only be one place that writes a particular
>> display register during modeset. Sometimes this isn't possible, and RMW
>> is required.
>> 
>> Some registers also have reserved bits potentially used by the hardware
>> that must not be changed, and RMW is required. These are documented in
>> bspec.
>> 
>> BR,
>> Jani.
>>
>
> Thanks for the explanation. It does make sense now that we are doing a
> full modeset, we should just be then writing the value directly?  The
> only concern I have is that say DSI code sets this somewhere els ein
> the modeset path, then we would need to modify this to do RMW or
> always make sure DSI also uses the same function for writing to this
> reg.  What do you suggest doing now?

I think all encoders in a tile group are always of the same type.

If the tile grouping in your patch is based purely on EDID, we may need
to enforce this. Surely genlock only works on encoders of the same type?

In any case DSI (at least currently) does not use tile groups, and will
never be mixed up in non-DSI tile groups. The DSI transcoders are
separate from other transcoders, so we're not writing the same registers
here.

---

Looking at the code, I am wondering if this should be pushed to encoder
hooks instead of adding into crtc enable.

BR,
Jani.



-- 
Jani Nikula, Intel Open Source Graphics Center