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"?
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.
Harry
On 2017-05-03 03:12 PM, Harry Wentland wrote:
On 2017-05-03 11:02 AM, Daniel Vetter wrote:
On Wed, May 03, 2017 at 04:26:51PM +0200, Christian König wrote:
Hi Harry,
while this looks more and more like it could work something which would
really help would be to have a set of patches squashed together and
rebased
on drm-next.
The dc-drm-next-atomic-wip looks like a start, but we need more
something
like:
drm/amdgpu: add base DC components
drm/amdgpu: add DCE8 support for DC
drm/amdgpu: add DCE9 support for DC
drm/amdgpu: add DCE10 support for DC
...
drm/amdgpu: enable DC globally
drm/amdgpu: remove old display infrastructure
Otherwise I fear that people will just be lost in the mass amount of
patches
we have in the branch.
I think for a quick first feedback round simply something that's based on
top of recent drm-next is good enough, since I'll probably only look at
the aggregate diff anyway (or resulting tree).
This was basically what I figured which is why I didn't invest more time
to squash changes. I agree with Christian that eventually we probably
want something like his suggested split but for now I'd rather focus on
tying a dc_surface to a drm_plane (and same for other objects). Starting
to tie our objects to drm objects has been very eye-opening, to say the
least.
Daniel, do you think you'll find time to take a look at this this week
or next?
Harry
-Daniel
Regards,
Christian.
Am 03.05.2017 um 16:13 schrieb Harry Wentland:
Hi all,
Over the last few months we (mostly Andrey and myself) have taken and
addressed some of the feedback received from December's DC RFC. A
lot of
our work so far centers around atomic. We were able to take a whole
bunch of the areas where we rolled our own solution and use DRM atomic
helpers instead.
You can find our most recent drm-next based tree at
https://cgit.freedesktop.org/~hwentland/linux/log/?h=dc-drm-next-atomic-wip
An outline of the changes with commit hashes from that tree are listed
below. They aren't necessarily in order but are grouped by
functionality.
I would like to solicit input on the changes and the current state of
amd/display in general.
I'm on IRC every weekday from 9-5 eastern time, sometimes at other
hours. Feel free to ask questions, discuss, leave comments. Let me know
if there's anything else I can do to facilitate review.
We know there's more work to be done but would much rather prioritize
that work based on community feedback than merely our own impressions.
We haven't finished plumbing drm types to the dc types yet, and there
are still a bunch of other things in progress. We are not looking to
re-hash the previous discussion, but rather we'd like some feedback on
our work so far.
The list of changes (trimmed commit tags):
== Use common helpers for pageflip ==
144da239b047 Use pflip prepare and submit parts (v2)
ff7ac264a9a1 Fix page flip after daniel's acquire_ctx change
== implement atomic_get_properties ==
cf4a84df7189 Fallback on legacy properties in atomic_get_properties
01f96706b6ca get_atomic_property missing for drm_connector_funcs
== Use common helpers for gamma ==
3f547d7098de Use atomic helpers for gamma
== Use atomic helpers for commit ==
41831f55bd58 Refactor atomic commit implementation. (v2)
6c67dd3c5cd5 Refactor headless to use atomic commit.
eb22ef1ecb16 Remove page_fleep_needed function.
== Use atomic helpers for S3 ==
5a6ae6f76249 Switch to DRM helpers in s3.
== Simmplify mapping between DRM and DC objects ==
84a3ee023b9b Remove get_connector_for_link.
6d8978a98b40 Remove get_connector_for_sink.
== Use DRM EDID read instead of DC ==
566969dacfad Fix i2c write flag.
527c3699ff3c Refactor edid read.
5ac51023d275 Fix missing irq refactor causing potential i2c race
== Save DC validate_ctx in atomic_check and use in commit ==
8c194d8e4ee9 pull commit_surfaces out of atomic_commit into helper
function
27eef98b38c8 Return context from validate_context
ca3ee10e915b Add DM global atomic state
8ba1ca856532 Hook dm private state into atomic_check
10160455ac6d Add correct retain/release for objs in dm_atomic_state
0f1b2e2aecbb Commit validation set from state
258e6a91fc61 Add validate_context to atomic_state
64f569b5df20 Use validate_context from atomic_check in commit
== Start multi-plane implementation ==
601ff4e70b7c decouple per-crtc-plane model
a0b859a0b114 Fix cleanup in amdgpu_dm_initialize_drm_device
ee02010d7a82 update plane functionalities
3b7345fd1abb Allow planes on all crtcs
d9cf37462156 initialize YUV plane capabilities
248f06b2613e Universal cursor plane hook-up.
== Minor cleanups ==
d99bf02cb8fc Rename atomic_commit parameter.
f15fb9726502 Use amdgpu mode funcs statically
d3e37fa70643 Remove unused define from amdgpu_dm_types
80a38ad58125 Add lock around updating freesync property
8c7f16853824 Use new drm_dp_find_vcpi_slots to compute slots
Regards,
Harry
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
