On Thu, 28 Mar 2019, Manasi Navare <manasi.d.navare@xxxxxxxxx> wrote: > On Thu, Mar 28, 2019 at 11:18:56AM +0200, Jani Nikula wrote: >> On Fri, 22 Mar 2019, Manasi Navare <manasi.d.navare@xxxxxxxxx> wrote: >> > On Fri, Mar 22, 2019 at 09:28:01PM +0200, Jani Nikula wrote: >> >> On Fri, 22 Mar 2019, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
