| Message ID | 20220218100403.7028-5-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Review of mode copies | expand |
On 2022-02-18 05:03, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > struct drm_display_mode embeds a list head, so overwriting > the full struct with another one will corrupt the list > (if the destination mode is on a list). Use drm_mode_copy() > instead which explicitly preserves the list head of > the destination mode. > > Even if we know the destination mode is not on any list > using drm_mode_copy() seems decent as it sets a good > example. Bad examples of not using it might eventually > get copied into code where preserving the list head > actually matters. > > Obviously one case not covered here is when the mode > itself is embedded in a larger structure and the whole > structure is copied. But if we are careful when copying > into modes embedded in structures I think we can be a > little more reassured that bogus list heads haven't been > propagated in. > > @is_mode_copy@ > @@ > drm_mode_copy(...) > { > ... > } > > @depends on !is_mode_copy@ > struct drm_display_mode *mode; > expression E, S; > @@ > ( > - *mode = E > + drm_mode_copy(mode, &E) > | > - memcpy(mode, E, S) > + drm_mode_copy(mode, E) > ) > > @depends on !is_mode_copy@ > struct drm_display_mode mode; > expression E; > @@ > ( > - mode = E > + drm_mode_copy(&mode, &E) > | > - memcpy(&mode, E, S) > + drm_mode_copy(&mode, E) > ) > > @@ > struct drm_display_mode *mode; > @@ > - &*mode > + mode > > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Harry Wentland <harry.wentland@amd.com> > Cc: Leo Li <sunpeng.li@amd.com> > Cc: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> > Cc: amd-gfx@lists.freedesktop.org > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Reviewed-by: Harry Wentland <harry.wentland@amd.com> Harry > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 4 ++-- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 +++--- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > index fa20261aa928..673078faa27a 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > @@ -626,7 +626,7 @@ amdgpu_connector_fixup_lcd_native_mode(struct drm_encoder *encoder, > if (mode->type & DRM_MODE_TYPE_PREFERRED) { > if (mode->hdisplay != native_mode->hdisplay || > mode->vdisplay != native_mode->vdisplay) > - memcpy(native_mode, mode, sizeof(*mode)); > + drm_mode_copy(native_mode, mode); > } > } > > @@ -635,7 +635,7 @@ amdgpu_connector_fixup_lcd_native_mode(struct drm_encoder *encoder, > list_for_each_entry_safe(mode, t, &connector->probed_modes, head) { > if (mode->hdisplay == native_mode->hdisplay && > mode->vdisplay == native_mode->vdisplay) { > - *native_mode = *mode; > + drm_mode_copy(native_mode, mode); > drm_mode_set_crtcinfo(native_mode, CRTC_INTERLACE_HALVE_V); > DRM_DEBUG_KMS("Determined LVDS native mode details from EDID\n"); > break; > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index bd23c9e481eb..514280699ad5 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -6318,7 +6318,7 @@ get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector, > } > } > > - aconnector->freesync_vid_base = *m_pref; > + drm_mode_copy(&aconnector->freesync_vid_base, m_pref); > return m_pref; > } > > @@ -6432,8 +6432,8 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector, > recalculate_timing = is_freesync_video_mode(&mode, aconnector); > if (recalculate_timing) { > freesync_mode = get_highest_refresh_rate_mode(aconnector, false); > - saved_mode = mode; > - mode = *freesync_mode; > + drm_mode_copy(&saved_mode, &mode); > + drm_mode_copy(&mode, freesync_mode); > } else { > decide_crtc_timing_for_drm_display_mode( > &mode, preferred_mode, scale);
Applied. Thanks! Alex On Fri, Feb 18, 2022 at 11:32 AM Harry Wentland <harry.wentland@amd.com> wrote: > > > > On 2022-02-18 05:03, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > struct drm_display_mode embeds a list head, so overwriting > > the full struct with another one will corrupt the list > > (if the destination mode is on a list). Use drm_mode_copy() > > instead which explicitly preserves the list head of > > the destination mode. > > > > Even if we know the destination mode is not on any list > > using drm_mode_copy() seems decent as it sets a good > > example. Bad examples of not using it might eventually > > get copied into code where preserving the list head > > actually matters. > > > > Obviously one case not covered here is when the mode > > itself is embedded in a larger structure and the whole > > structure is copied. But if we are careful when copying > > into modes embedded in structures I think we can be a > > little more reassured that bogus list heads haven't been > > propagated in. > > > > @is_mode_copy@ > > @@ > > drm_mode_copy(...) > > { > > ... > > } > > > > @depends on !is_mode_copy@ > > struct drm_display_mode *mode; > > expression E, S; > > @@ > > ( > > - *mode = E > > + drm_mode_copy(mode, &E) > > | > > - memcpy(mode, E, S) > > + drm_mode_copy(mode, E) > > ) > > > > @depends on !is_mode_copy@ > > struct drm_display_mode mode; > > expression E; > > @@ > > ( > > - mode = E > > + drm_mode_copy(&mode, &E) > > | > > - memcpy(&mode, E, S) > > + drm_mode_copy(&mode, E) > > ) > > > > @@ > > struct drm_display_mode *mode; > > @@ > > - &*mode > > + mode > > > > Cc: Alex Deucher <alexander.deucher@amd.com> > > Cc: Harry Wentland <harry.wentland@amd.com> > > Cc: Leo Li <sunpeng.li@amd.com> > > Cc: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> > > Cc: amd-gfx@lists.freedesktop.org > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Reviewed-by: Harry Wentland <harry.wentland@amd.com> > > Harry > > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 4 ++-- > > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 +++--- > > 2 files changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > > index fa20261aa928..673078faa27a 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > > @@ -626,7 +626,7 @@ amdgpu_connector_fixup_lcd_native_mode(struct drm_encoder *encoder, > > if (mode->type & DRM_MODE_TYPE_PREFERRED) { > > if (mode->hdisplay != native_mode->hdisplay || > > mode->vdisplay != native_mode->vdisplay) > > - memcpy(native_mode, mode, sizeof(*mode)); > > + drm_mode_copy(native_mode, mode); > > } > > } > > > > @@ -635,7 +635,7 @@ amdgpu_connector_fixup_lcd_native_mode(struct drm_encoder *encoder, > > list_for_each_entry_safe(mode, t, &connector->probed_modes, head) { > > if (mode->hdisplay == native_mode->hdisplay && > > mode->vdisplay == native_mode->vdisplay) { > > - *native_mode = *mode; > > + drm_mode_copy(native_mode, mode); > > drm_mode_set_crtcinfo(native_mode, CRTC_INTERLACE_HALVE_V); > > DRM_DEBUG_KMS("Determined LVDS native mode details from EDID\n"); > > break; > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > index bd23c9e481eb..514280699ad5 100644 > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > @@ -6318,7 +6318,7 @@ get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector, > > } > > } > > > > - aconnector->freesync_vid_base = *m_pref; > > + drm_mode_copy(&aconnector->freesync_vid_base, m_pref); > > return m_pref; > > } > > > > @@ -6432,8 +6432,8 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector, > > recalculate_timing = is_freesync_video_mode(&mode, aconnector); > > if (recalculate_timing) { > > freesync_mode = get_highest_refresh_rate_mode(aconnector, false); > > - saved_mode = mode; > > - mode = *freesync_mode; > > + drm_mode_copy(&saved_mode, &mode); > > + drm_mode_copy(&mode, freesync_mode); > > } else { > > decide_crtc_timing_for_drm_display_mode( > > &mode, preferred_mode, scale); >
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c index fa20261aa928..673078faa27a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c @@ -626,7 +626,7 @@ amdgpu_connector_fixup_lcd_native_mode(struct drm_encoder *encoder, if (mode->type & DRM_MODE_TYPE_PREFERRED) { if (mode->hdisplay != native_mode->hdisplay || mode->vdisplay != native_mode->vdisplay) - memcpy(native_mode, mode, sizeof(*mode)); + drm_mode_copy(native_mode, mode); } } @@ -635,7 +635,7 @@ amdgpu_connector_fixup_lcd_native_mode(struct drm_encoder *encoder, list_for_each_entry_safe(mode, t, &connector->probed_modes, head) { if (mode->hdisplay == native_mode->hdisplay && mode->vdisplay == native_mode->vdisplay) { - *native_mode = *mode; + drm_mode_copy(native_mode, mode); drm_mode_set_crtcinfo(native_mode, CRTC_INTERLACE_HALVE_V); DRM_DEBUG_KMS("Determined LVDS native mode details from EDID\n"); break; diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index bd23c9e481eb..514280699ad5 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -6318,7 +6318,7 @@ get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector, } } - aconnector->freesync_vid_base = *m_pref; + drm_mode_copy(&aconnector->freesync_vid_base, m_pref); return m_pref; } @@ -6432,8 +6432,8 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector, recalculate_timing = is_freesync_video_mode(&mode, aconnector); if (recalculate_timing) { freesync_mode = get_highest_refresh_rate_mode(aconnector, false); - saved_mode = mode; - mode = *freesync_mode; + drm_mode_copy(&saved_mode, &mode); + drm_mode_copy(&mode, freesync_mode); } else { decide_crtc_timing_for_drm_display_mode( &mode, preferred_mode, scale);