Soliciting DRM feedback on latest DC rework

[Daniel Vetter]

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 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.

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