On 2017-05-09 04:24 AM, Daniel Vetter wrote:
On Mon, May 08, 2017 at 02:54:22PM -0400, Harry Wentland 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 {
...
}
Any objections to doing something like this instead:
#ifdef LINUX
#include "drm_crtc.h"
#endif
struct dc_stream {
#ifdef LINUX
struct drm_crtc_state base;
#endif
...
}
The ifdefs are then removed on Linux versions of DC, while other OSs
wouldn't compile with the LINUX flag.
- validate_context->res_ctx->pool looks extremely fishy. In principle,
refcounting does not mesh with the duplicate, check, then
free-or-commit semantics of atomic. Are you sure this works
correctly when doing TEST_ONLY commits? igt or weston (with Daniel's
patches) are your friend (or enemy) here :-)
The common approach all other drivers implement:
- Same duplicate semantics for private objects as for core drm
objects. The private obj helpers will help with cutting down the
boiler-plate.
- No refcounts, instead an allocation bitmask in the object one up
in the hierarchy (usually global state, but sometimes also
crtc/stream).
- If you want to keep the current design, then you
must do a deep copy of pool (i.e. not just the struct, but also
every struct that might change it points too). The above
accomplishes that essentially, except in standard atomic patterns.
- You'll probably need a bunch of for_each_foo_in_context macros,
built using the private state helpers.
Trying to review correctness of atomic_check when private resources
are refcounted is real hard. The only case we have is vc4, and it's
kinda not pretty (it has it's own manual rollback hacks in the
release functions). Going entirely with free-standing stuff is much
better.
Good points here. I think I'll ultimately have to get IGT running against
our code with TEST_ONLY commits and see what happens. Ultimately we probably
have to deep copy, one way or another. I don't really want any rollback
stuff as that would be near impossible to maintain.
This shouldn't be a huge conceptual change in the DC design (it
already has checks for "is this unchanged" and then ignore that
object), just quite a bit more invasive than sprinkling for_each_*
macros all over the place. From a glane it could be that you'd get
80% there by having a for_each_changed_*_in_context set of macros,
with the current code everywhere else, and the "jumps over unchanged
obj because they're not in drm_atomic_state/dc_validation_context"
logic on drm atomic.
Yeah, this should fit mostly with DC design. Will evaluate this once we link
DC objects to DRM state objects.
@@ -1640,6 +1644,8 @@ static void dm_crtc_helper_disable(struct drm_crtc *crtc)
{
}
+/* no dummy funcs pls, counts everywhere */
+
Does the comment apply to the preceding or next function? What do you mean
with "counts everywhere"?
In general you have a lot of callbacks which are either just {} or {
return 0; }, aka empty/dummy implementations.
We've refactored atomic helpers a lot to make all of these optional, pls
remove them. This was a somewhat recent development, I guess initial
atomic DC still had to have all these dummy callbacks.
That makes sense. We haven't really revisited these since our initial
work on atomic more than a year ago.
static int dm_crtc_helper_atomic_check(
struct drm_crtc *crtc,
struct drm_crtc_state *state)
@@ -3077,6 +3102,15 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
acrtc = to_amdgpu_crtc(crtc);
+ /* This is the wrong way round. If you have a requirement for a
+ * 1:1 connector/crtc mapping, then loop over connectors and
+ * grab the crtc from there. Plus make sure there's really only
+ * 1 connector per crtc (but the possible clones stuff should
+ * catch that for you I think, at least with latest atomic
+ * helpers.
+ *
+ * Similar for the same loop in commit.
+ */
aconnector = amdgpu_dm_find_first_crct_matching_connector(state, crtc, true);
action = get_dm_commit_action(crtc_state);
Good point. This code is still a bit of a mess.
I think once you do the embedding and switch the state/obj iterators to
the atomic versions there should be a pile more places to clean up. Atomic
That's pretty much what I'm thinking. I've noticed how good cleanups
tend to snowball, allowing us to clean up more stuff.
helper library should be a good place to read up on best practices
(especially since the recent merge of the extended iterator macros from
Maarten).
Loop over connectors and chase one pointer (often
connector_state->best_encoder) is a fairly common pattern, and if you have
a 1:1 crtc/connector routing (and it's enforced) then I think in many
places a natural loop pattern will go over connectors and then chase
connector_state->crtc. i915 also has plenty of examples in drm-tip (only
that one has Maarten's patches which switch all the modeset code over to
the new iterators).
Thanks. Will take a look there.
Harry
-Daniel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
