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

[Manasi Navare]

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?

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