Soliciting DRM feedback on latest DC rework

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>
>>>
>>


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux