Message ID | 20191018081323.5836-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/i915: Perform manual conversions for crtc uapi/hw split, v2. | expand |
On Fri, Oct 18, 2019 at 10:13:23AM +0200, Maarten Lankhorst wrote: > intel_get_load_detect_pipe() needs to set uapi active, > uapi enable is set by the call to drm_atomic_set_mode_for_crtc(), > so we can remove it. > > intel_pipe_config_compare() needs to look at hw state, but I didn't > change spatch to look at it. It's easy enough to do manually. > > intel_atomic_check() definitely needs to check for uapi enable, > otherwise intel_modeset_pipe_config cannot copy uapi state to hw. We seem to have three totally separate things in this one patch. > > Changes since v1: > - Actually set uapi.active in get_load_detect_pipe(). > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 42 ++++++++++---------- > 1 file changed, 21 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index fa0abfdff2ae..bbac6b764d92 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -11214,7 +11214,7 @@ int intel_get_load_detect_pipe(struct drm_connector *connector, > goto fail; > } > > - crtc_state->base.active = crtc_state->base.enable = true; > + crtc_state->uapi.active = true; > > if (!mode) > mode = &load_detect_mode; > @@ -12754,19 +12754,19 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, > > PIPE_CONF_CHECK_X(output_types); > > - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay); > - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal); > - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_start); > - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_end); > - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_start); > - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_end); > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_hdisplay); > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_htotal); > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_hblank_start); > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_hblank_end); > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_hsync_start); > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_hsync_end); > > - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vdisplay); > - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vtotal); > - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vblank_start); > - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vblank_end); > - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vsync_start); > - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vsync_end); > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vdisplay); > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vtotal); > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vblank_start); > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vblank_end); > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vsync_start); > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vsync_end); > > PIPE_CONF_CHECK_I(pixel_multiplier); > PIPE_CONF_CHECK_I(output_format); > @@ -12783,17 +12783,17 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, > > PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio); > > - PIPE_CONF_CHECK_FLAGS(base.adjusted_mode.flags, > + PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags, > DRM_MODE_FLAG_INTERLACE); > > if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS)) { > - PIPE_CONF_CHECK_FLAGS(base.adjusted_mode.flags, > + PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags, > DRM_MODE_FLAG_PHSYNC); > - PIPE_CONF_CHECK_FLAGS(base.adjusted_mode.flags, > + PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags, > DRM_MODE_FLAG_NHSYNC); > - PIPE_CONF_CHECK_FLAGS(base.adjusted_mode.flags, > + PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags, > DRM_MODE_FLAG_PVSYNC); > - PIPE_CONF_CHECK_FLAGS(base.adjusted_mode.flags, > + PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags, > DRM_MODE_FLAG_NVSYNC); > } > > @@ -12832,7 +12832,7 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, > > bp_gamma = intel_color_get_gamma_bit_precision(pipe_config); > if (bp_gamma) > - PIPE_CONF_CHECK_COLOR_LUT(gamma_mode, base.gamma_lut, bp_gamma); > + PIPE_CONF_CHECK_COLOR_LUT(gamma_mode, hw.gamma_lut, bp_gamma); > > } > > @@ -12877,7 +12877,7 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, > if (IS_G4X(dev_priv) || INTEL_GEN(dev_priv) >= 5) > PIPE_CONF_CHECK_I(pipe_bpp); > > - PIPE_CONF_CHECK_CLOCK_FUZZY(base.adjusted_mode.crtc_clock); > + PIPE_CONF_CHECK_CLOCK_FUZZY(hw.adjusted_mode.crtc_clock); > PIPE_CONF_CHECK_CLOCK_FUZZY(port_clock); > > PIPE_CONF_CHECK_I(min_voltage_level); > @@ -13572,7 +13572,7 @@ static int intel_atomic_check(struct drm_device *dev, > if (!needs_modeset(new_crtc_state)) > continue; > > - if (!new_crtc_state->base.enable) { > + if (!new_crtc_state->uapi.enable) { > any_ms = true; > continue; > } > -- > 2.23.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 22-10-2019 om 20:16 schreef Ville Syrjälä: > On Fri, Oct 18, 2019 at 10:13:23AM +0200, Maarten Lankhorst wrote: >> intel_get_load_detect_pipe() needs to set uapi active, >> uapi enable is set by the call to drm_atomic_set_mode_for_crtc(), >> so we can remove it. >> >> intel_pipe_config_compare() needs to look at hw state, but I didn't >> change spatch to look at it. It's easy enough to do manually. >> >> intel_atomic_check() definitely needs to check for uapi enable, >> otherwise intel_modeset_pipe_config cannot copy uapi state to hw. > We seem to have three totally separate things in this one patch. The patch is about the manual conversions that need to be done because the automated checks get them wrong. So it touches 3 spots but does 1 thing, if you want I can split it into 3 patches. Will do so if required. ~Maarten
On Thu, Oct 24, 2019 at 02:12:46PM +0200, Maarten Lankhorst wrote: > Op 22-10-2019 om 20:16 schreef Ville Syrjälä: > > On Fri, Oct 18, 2019 at 10:13:23AM +0200, Maarten Lankhorst wrote: > >> intel_get_load_detect_pipe() needs to set uapi active, > >> uapi enable is set by the call to drm_atomic_set_mode_for_crtc(), > >> so we can remove it. > >> > >> intel_pipe_config_compare() needs to look at hw state, but I didn't > >> change spatch to look at it. It's easy enough to do manually. > >> > >> intel_atomic_check() definitely needs to check for uapi enable, > >> otherwise intel_modeset_pipe_config cannot copy uapi state to hw. > > We seem to have three totally separate things in this one patch. > > The patch is about the manual conversions that need to be done because the automated > > checks get them wrong. Does that mean the series is not actually bisectable? > So it touches 3 spots but does 1 thing, if you want I can split it into 3 patches. > > Will do so if required. > > ~Maarten
Op 24-10-2019 om 14:23 schreef Ville Syrjälä: > On Thu, Oct 24, 2019 at 02:12:46PM +0200, Maarten Lankhorst wrote: >> Op 22-10-2019 om 20:16 schreef Ville Syrjälä: >>> On Fri, Oct 18, 2019 at 10:13:23AM +0200, Maarten Lankhorst wrote: >>>> intel_get_load_detect_pipe() needs to set uapi active, >>>> uapi enable is set by the call to drm_atomic_set_mode_for_crtc(), >>>> so we can remove it. >>>> >>>> intel_pipe_config_compare() needs to look at hw state, but I didn't >>>> change spatch to look at it. It's easy enough to do manually. >>>> >>>> intel_atomic_check() definitely needs to check for uapi enable, >>>> otherwise intel_modeset_pipe_config cannot copy uapi state to hw. >>> We seem to have three totally separate things in this one patch. >> The patch is about the manual conversions that need to be done because the automated >> >> checks get them wrong. > Does that mean the series is not actually bisectable? I've checked all places manually and it should be correct. For any regression we will likely end up at patch 8/14 as it formalizes the split. You can investigate further by changing all hw references in a file to uapi. Realistically you won't hit any issues until bigjoiner is introduced, and hw and uapi can contain wildly different values. This is mostly noticeable for plane_state, because plane_state->uapi.fb/crtc will be null for a slave plane.
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index fa0abfdff2ae..bbac6b764d92 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -11214,7 +11214,7 @@ int intel_get_load_detect_pipe(struct drm_connector *connector, goto fail; } - crtc_state->base.active = crtc_state->base.enable = true; + crtc_state->uapi.active = true; if (!mode) mode = &load_detect_mode; @@ -12754,19 +12754,19 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, PIPE_CONF_CHECK_X(output_types); - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay); - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal); - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_start); - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_end); - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_start); - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_end); + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_hdisplay); + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_htotal); + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_hblank_start); + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_hblank_end); + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_hsync_start); + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_hsync_end); - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vdisplay); - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vtotal); - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vblank_start); - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vblank_end); - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vsync_start); - PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vsync_end); + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vdisplay); + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vtotal); + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vblank_start); + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vblank_end); + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vsync_start); + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vsync_end); PIPE_CONF_CHECK_I(pixel_multiplier); PIPE_CONF_CHECK_I(output_format); @@ -12783,17 +12783,17 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio); - PIPE_CONF_CHECK_FLAGS(base.adjusted_mode.flags, + PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags, DRM_MODE_FLAG_INTERLACE); if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS)) { - PIPE_CONF_CHECK_FLAGS(base.adjusted_mode.flags, + PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags, DRM_MODE_FLAG_PHSYNC); - PIPE_CONF_CHECK_FLAGS(base.adjusted_mode.flags, + PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags, DRM_MODE_FLAG_NHSYNC); - PIPE_CONF_CHECK_FLAGS(base.adjusted_mode.flags, + PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags, DRM_MODE_FLAG_PVSYNC); - PIPE_CONF_CHECK_FLAGS(base.adjusted_mode.flags, + PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags, DRM_MODE_FLAG_NVSYNC); } @@ -12832,7 +12832,7 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, bp_gamma = intel_color_get_gamma_bit_precision(pipe_config); if (bp_gamma) - PIPE_CONF_CHECK_COLOR_LUT(gamma_mode, base.gamma_lut, bp_gamma); + PIPE_CONF_CHECK_COLOR_LUT(gamma_mode, hw.gamma_lut, bp_gamma); } @@ -12877,7 +12877,7 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, if (IS_G4X(dev_priv) || INTEL_GEN(dev_priv) >= 5) PIPE_CONF_CHECK_I(pipe_bpp); - PIPE_CONF_CHECK_CLOCK_FUZZY(base.adjusted_mode.crtc_clock); + PIPE_CONF_CHECK_CLOCK_FUZZY(hw.adjusted_mode.crtc_clock); PIPE_CONF_CHECK_CLOCK_FUZZY(port_clock); PIPE_CONF_CHECK_I(min_voltage_level); @@ -13572,7 +13572,7 @@ static int intel_atomic_check(struct drm_device *dev, if (!needs_modeset(new_crtc_state)) continue; - if (!new_crtc_state->base.enable) { + if (!new_crtc_state->uapi.enable) { any_ms = true; continue; }
intel_get_load_detect_pipe() needs to set uapi active, uapi enable is set by the call to drm_atomic_set_mode_for_crtc(), so we can remove it. intel_pipe_config_compare() needs to look at hw state, but I didn't change spatch to look at it. It's easy enough to do manually. intel_atomic_check() definitely needs to check for uapi enable, otherwise intel_modeset_pipe_config cannot copy uapi state to hw. Changes since v1: - Actually set uapi.active in get_load_detect_pipe(). Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 42 ++++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-)