On 2017-05-08 03:07 PM, Dave Airlie wrote: > On 9 May 2017 at 04:54, Harry Wentland <harry.wentland at amd.com> wrote: >> Hi Daniel, >> >> Thanks for taking the time to look at DC. >> >> I had a couple more questions/comments in regard to the patch you posted on >> IRC: http://paste.debian.net/plain/930704 >> >> My impression is that this item is the most important next step for us: >> >>> From a quick glance I think what we want instead is: >>> - amdgpu_crtc_state and amdgpu_plane_state as following: >>> >>> struct amdgpu_crtc_state { >>> struct drm_crtc_state base; >>> struct dc_stream; >>> }; >> >> >> Unfortunately as sketched here it doesn't quite mesh with what we currently >> have, which is: >> >> struct stream { >> struct core_stream protected; >> ... >> } >> >> struct core_stream { >> struct dc_stream public; >> ... >> } >> >> struct dc_stream { >> ... >> } >> > > Is there any major reason to keep all those abstractions? > > Could you collapse everything into struct dc_stream? > > I haven't looked recently but I didn't get the impression there was a > lot of design around what was public/protected, more whatever needed > to be used by someone else was in public. > Yeah, I've been thinking the same recently. The thought behind public/protected was that DM can modify public but would never touch protected. We don't really want to give DM an opportunity to touch core_stream directly. If it does we might lose a lot of our confidence in DC correctness which comes from running the same code on different OSes. That said, this could be caught during code review and collapsing these structs would simplify things a bit. For one, it would allow me to take Daniel's proposal as-is. I'll think about it some more and maybe mock up a couple patches and see how they're received internally. Harry > Dave. >
