On Fri, Oct 18, 2019 at 02:09:17PM +0200, Maarten Lankhorst wrote:
> Op 18-10-2019 om 12:36 schreef Ville Syrjälä:
> > On Thu, Oct 17, 2019 at 03:20:55PM +0200, Maarten Lankhorst wrote:
> >> Prepare to split up hw and uapi machinally, by adding a uapi and
> >> hw alias. We will remove the base in a bit. This is a split from the
> >> original uapi/hw patch, which did it all in one go.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> >> ---
> >> drivers/gpu/drm/i915/display/intel_atomic.c | 8 ++++--
> >> drivers/gpu/drm/i915/display/intel_display.c | 2 ++
> >> drivers/gpu/drm/i915/display/intel_display.h | 6 ++---
> >> .../drm/i915/display/intel_display_types.h | 27 ++++++++++++++++++-
> >> 4 files changed, 37 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> >> b/drivers/gpu/drm/i915/display/intel_atomic.c
> >> index e6cb85d41c8d..2cdc92897abd 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> >> @@ -186,9 +186,10 @@ intel_digital_connector_duplicate_state(struct
> >> drm_connector *connector)
> >> struct drm_crtc_state *
> >> intel_crtc_duplicate_state(struct drm_crtc *crtc)
> >> {
> >> + const struct intel_crtc_state *old_crtc_state =
> >> to_intel_crtc_state(crtc->state);
> >> struct intel_crtc_state *crtc_state;
> >>
> >> - crtc_state = kmemdup(crtc->state, sizeof(*crtc_state), GFP_KERNEL);
> >> + crtc_state = kmemdup(old_crtc_state, sizeof(*crtc_state), GFP_KERNEL);
> >> if (!crtc_state)
> >> return NULL;
> >>
> >> @@ -219,7 +220,10 @@ void
> >> intel_crtc_destroy_state(struct drm_crtc *crtc,
> >> struct drm_crtc_state *state)
> >> {
> >> - drm_atomic_helper_crtc_destroy_state(crtc, state);
> >> + struct intel_crtc_state *crtc_state = to_intel_crtc_state(state);
> >> +
> >> + __drm_atomic_helper_crtc_destroy_state(&crtc_state->base);
> >> + kfree(crtc_state);
> >> }
> >>
> >> static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state
> >> *scaler_state,
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> >> b/drivers/gpu/drm/i915/display/intel_display.c
> >> index 5632e13d458d..fa0abfdff2ae 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >> @@ -12282,6 +12282,8 @@ clear_intel_crtc_state(struct intel_crtc_state
> >> *crtc_state)
> >>
> >> /* Keep base drm_crtc_state intact, only clear our extended struct */
> >> BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> >> + BUILD_BUG_ON(offsetof(struct intel_crtc_state, uapi));
> >> + BUILD_BUG_ON(offsetof(struct intel_crtc_state, hw));
> >> memcpy(&crtc_state->base + 1, &saved_state->base + 1,
> >> sizeof(*crtc_state) - sizeof(crtc_state->base));
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.h
> >> b/drivers/gpu/drm/i915/display/intel_display.h
> >> index 90807603987c..9b53f65386b5 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> >> @@ -444,10 +444,10 @@ enum phy_fia {
> >> #define intel_atomic_crtc_state_for_each_plane_state( \
> >> plane, plane_state, \
> >> crtc_state) \
> >> - for_each_intel_plane_mask(((crtc_state)->base.state->dev), (plane), \
> >> - ((crtc_state)->base.plane_mask)) \
> >> + for_each_intel_plane_mask(((crtc_state)->uapi.state->dev), (plane), \
> >> + ((crtc_state)->uapi.plane_mask)) \
> >> for_each_if ((plane_state = \
> >> -
> >> to_intel_plane_state(__drm_atomic_get_current_plane_state((crtc_state)->base.state,
> >> &plane->base))))
> >> +
> >> to_intel_plane_state(__drm_atomic_get_current_plane_state((crtc_state)->uapi.state,
> >> &plane->base))))
> >>
> >> void intel_link_compute_m_n(u16 bpp, int nlanes,
> >> int pixel_clock, int link_clock,
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> >> b/drivers/gpu/drm/i915/display/intel_display_types.h
> >> index 244e881474fb..4d85ea5832d7 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> >> @@ -749,7 +749,32 @@ enum intel_output_format {
> >> };
> >>
> >> struct intel_crtc_state {
> >> + union {
> >> struct drm_crtc_state base;
> >> + /*
> >> + * uapi (drm) state. This is the software state shown to userspace.
> >> + * In particular, the following members are used for bookkeeping:
> >> + * - crtc
> >> + * - state
> >> + * - *_changed
> >> + * - event
> >> + * - commit
> >> + * - mode_blob
> >> + */
> >> + struct drm_crtc_state uapi;
> >> +
> >> + /*
> >> + * actual hardware state, the state we program to the hardware.
> >> + * The following members are used to verify the hardware state:
> >> + * - enable
> >> + * - active
> >> + * - mode / adjusted_mode
> >> + * - color property blobs.
> >> + *
> >> + * During initial hw readout, they need to be copied to uapi.
> >> + */
> >> + struct drm_crtc_state hw;
> >> + };
> > This part confuses me. Can't we just do
> >
> > - struct drm_crtc_state base;
> > + struct drm_crtc_state uapi;
> > + struct {
> > + ...
> > + } base;
> >
> > ?
>
> This is basically union { struct drm_crtc_state uapi,base,hw; }; Making all 3
> aliases until patch 8/14, which kills off base and splits off hw state.
>
> We keep it as an union because base and uapi temporarily have to be kept in
> sync, while we split to uapi and hw. So it's easy to keep hw in sync as well.
>
> In the end it doesn't matter whether we split off hw before or after. The
> diff between before and after is the same.
Sure but the individual patches are very hard to review. Eg. I can see
a bunch of stuff getting converted from base to hw, but since .base
still exists I have no way of knowing whether the patch really changed
all of them or just some of them.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
