On Fri, Jun 16, 2017 at 03:51:27PM -0400, Leo wrote: > Hello, > > > I'm looking for ideas and suggestions on how to enable IGT to work on > planes with multiple possible_crtcs during atomic commit. Amdgpu > currently has features that rely on this, and planes with multiple > crtc's are not being properly committed when tests run an atomic commit. > > > To explain, DRM has a field on planes called "possible_crtcs" that > allows a plane to be valid for multiple CRTC's. In igt_display_init(), > it is used to determine whether a igt_plane should be attached to a > pipe. Therefore, when a plane specifies a possible_crtcs of say, 0xFF, > igt_display_init() will create *copies* of this plane and attach it to > all available pipes. > > The problem arises when committing changes via igt_atomic_commit (which > eventually calls drmModeAtomicCommit). Say we have two pipes (A and > B) and a single plane with possible_crtcs=0xFF. igt_display_init() will > create copies of this plane and store it within both A and B, as 0xFF > makes it a valid plane for both pipes. Keyword here is "copies", since a > change to A.plane will not be reflected in B.plane. > > For example, say we call igt_plane_set_fb() using A.plane, changing its > fb_id. After this call, A.plane.fb_id will be updated, and B.plane.fb_id > will remain stale. We then call igt_atomic_commit() to commit the fb > changes. When preparing the set of properties to commit, the updated > fb_id from pipe A will first be set, followed by the stale fb_id from > pipe B. DRM then removes duplicates from the property set before calling > the atomic commit ioctl. Since the algorithm keeps the duplicate item > with the largest index, the update to A.plane gets overridden by > B.plane's stale fb_id property, causing unexpected behavior. Hey Leo, Seems like it wasn't designed for such use in the first place, so rework is the most welcome :-) CCing Petri as he may have some ideas. > One possible solution is to have igt_pipe_t use an array of pointers to > igt_plane_t, and have the set of unique igt_plane_t's stored elsewhere > (perhaps under igt_display_t). Changes to A.plane will then be properly > reflected on B.plane since it is by reference. However, > igt_display_init() will have to be modified, and igt_plane_t's pipe and > index fields will have a different meaning. Pipe nad index fields are considered private anyway, so the semantics change of those shouldn't hit code outside of igt_kms.c at all. > Another possibility is to introduce a field within igt_plane_t to > reference the "one true copy". Any read or write to the plane's fields > will then have to first check if it is a copy, and reference the one > true copy in the event that it is. This can get quite messy. Yup, looks like it can get pretty messy pretty quick. I prefer the first option, and I don't have better idea of my own. > The modifications seem to be quite wide, and I'd like some input on this > before I jump in. Any ideas and/or suggestions will be very much > appreciated! I am not very familiar with this part of IGT yet. I would like to see rough and ready RFC patchset, just to see the direction the changes are taking. After that people should be able to give more detailed feedback as you hone the implementation. Thanks! -- Cheers, Arek
