On Fri, 26. Jun 22:13, Evgenii Burenchev wrote:
> Fixes: 5d945cbcd4b1 ("drm/amd/display: Create a file dedicated to planes")
> Fixes: 473683a03495 ("drm/amd/display: Create a file dedicated for CRTC")
> Fixes: e7b07ceef2a6 ("drm/amd/display: Merge amdgpu_dm_types and amdgpu_dm")
> Signed-off-by: Evgenii Burenchev <[email protected]>
Having three different Fixes tags implies the big patch could be split up
into three separate patches which do one thing at a time. They can be
combined in a series for ease of handling.
> @@ -8151,33 +8151,41 @@ static void amdgpu_dm_connector_destroy(struct
> drm_connector *connector)
>
> void amdgpu_dm_connector_funcs_reset(struct drm_connector *connector)
> {
> - struct dm_connector_state *state =
> + /* Remember the old state */
> + struct dm_connector_state *old_state =
> to_dm_connector_state(connector->state);
>
> + struct dm_connector_state *state;
No empty lines inside local variable declaration block, please.
> +
> + /* Allocate new state */
Well, all the comments added with the patch - IMO they duplicate what the
code is doing - that doesn't add any real value and just bloats the
codebase.
> + state = kzalloc_obj(*state);
> + if (WARN_ON(!state))
> + return;
It's not common to WARN on memory allocation errors. If this code is ever
fuzzed with fault-injections enabled, that'd be one of the first issues to
pop up.
I think if the system is in a state when it can't allocate a bunch of
GFP_KERNEL memory, there'd definitely be some noticeable activity in
dmesg. Some (random) assertion triggered inside amdgpu won't help much -
that will only halt those machines booted with panic_on_warn=1.
