On Mon, May 08, 2017 at 03:50:36PM -0400, Harry Wentland wrote: > > > On 2017-05-08 03:07 PM, Dave Airlie wrote: > > On 9 May 2017 at 04:54, Harry Wentland <harry.wentland@xxxxxxx> 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. Yeah I'm seconding Dave's concern. The entire point of doing this embedding is to convert DC-the-midlayer into DC-the-helper library. The distinguishing desing point is that the former hides stuff, the latter doesn't. So shoveling all the core_stream stuff into dc_stream (or just embedding that one directly into amdgpu_crtc_state as an interim step) is much more in line with this. public/private interfaces for helper libraries don't make much sense as a concept, it's more different levels of details in how much you need to overwrite/control the implemented behaviour. So sliding scale, not binary barrier. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
