[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 Thu, 28 Mar 2019, Manasi Navare <manasi.d.navare at intel.com> wrote:
> On Thu, Mar 28, 2019 at 11:18:56AM +0200, Jani Nikula wrote:
>> 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.
>
> Yes all the encoders in  tile group are always 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?
>>
>
> So all the slaves and their master will always be of same type and yes it is
> based on the EDID tile block parsing.
> But just to double sure I think when i assign the master slave pointers, I should
> check that the connector type is the same.
>  
>> 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.
>
> As per the Bspec sequence, this needs to happen before enabling the
> TRANS_DDI_FUNC_CTL and after the link training, so I put in the
> crtc_enable hook, which encoder hooks are you suggesting adding this?

Maybe go with what you have now first, this can be pushed to encoders
later if needed. (I hope I don't regret this. ;)

BR,
Jani.



>
> Regards
> Manasi
>> 
>> BR,
>> Jani.
>> 
>> 
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center