> > This feature adds an extra IDVS group size field to the JM_CONFIG > > register. In kbase, the value is configurable via the device tree; kbase > > uses 0xF as a default if no value is specified. Until we find a device > > demanding otherwise, let's always set the 0xF default on devices which > > support this feature mimicking kbase's behaviour. > > This is a performance thing - so I don't think it will break anything if > this is wrong, it just won't be optimal. Then interpret my remarks as hardcoding the default until we find a device where setting to something other than 0xF improves performance nontrivially. (Read: I am lazy and do not want to write dt-bindings for something nobody will ever use.) > > As JM_CONFIG is an undocumented register, it's not clear to me what > > happens if we fail to include this handling. Index-driven vertex shading > > already works on Bifrost boards with this feature without this handling. > > Perhaps this has performance implications? Patch untested for the > > moment, wanted to give Steven a chance to comment. > > As it's a performance thing you shouldn't see correctness issues with > not setting it. But 0xF seems to have been chosen as it gave the best > overall performance (although for individual test content this can > vary). AFAICT the performance impact isn't massive either. Good to know, will update the commit message accordingly. > Reviewed-by: Steven Price <steven.price@xxxxxxx> > > Since you've tagged this RFC I won't merge it now, but it looks correct > to me. Thanks for the review... I hope you like reviewing Panfrost patches because I have a Valhall bring-up series waiting o:) When I get a chance to uprev the kernel on my G52 board I'll see if I can benchmark the impact of this change, so far this is only compile-tested. Even if there's no impact the patch should likely go in to stay consistent with kbase, but hopefully there's a win from this. At that point I'll send a v2 with your reviewed-by (and hopefully no changes other than the commit message) and we'll land that.
