On Thu, Oct 10, 2019 at 04:21:00PM +0200, Maarten Lankhorst wrote:
> Op 08-10-2019 om 19:06 schreef Ville Syrjälä:
> > On Fri, Oct 04, 2019 at 01:34:58PM +0200, Maarten Lankhorst wrote:
> >> We want to split drm_crtc_state into the user visible state
> >> and actual hardware state. To prepare for this, we need some
> >> ground rules what should be in each state:
> >>
> >> In uapi we use:
> >> - crtc, *_changed flags, event, commit, state, mode_blob,
> >> (plane/connector/encoder)_mask.
> >>
> >> In hw state we use what's displayed in hardware:
> >> - enable, active, (adjusted) mode, color property blobs.
> >>
> >> clear_intel_crtc_state and hw readout need to be updated for these rules,
> >> which will allow us to enable 2 joined pipes.
> > I still have hard time with reading this patch. I still think it
> > would be easier to read if we didn't do both the "uapi" and "hw" changes
> > at the same time.
> >
> > step 1.
> > struct drm_crtc_state uapi;
> > struct {
> > // hw state
> > } base;
> >
> > step 2.
> > s/base/hw/
> >
> > I think that would make it more obvious which parts of the code are
> > looking at which state.
>
> It wouldn't I think, but here's
> a dumb change with spatch on this patch.
>
> //+ struct {
> //+ bool active, enable;
> //+ struct drm_property_blob *degamma_lut, *gamma_lut, *ctm;
> //+ struct drm_display_mode mode, adjusted_mode;
> //+ } hw;
>
> @@
> struct intel_crtc_state *T;
> @@
> -T->uapi.active
> +T->hw.active
This doesn't really help me. There is no .uapi in upstream
code. I would like to see just the .base->.uapi changes
alone first so I can review which parts start to look at
the uapi state to make sure we aren't changing too much.
Then I'd like to to see the .base->.hw changes so that I
convince myself we didn't miss anything in the .base->.uapi
conversion.
And all the remaining drm_crtc_state usage is going to
make us miss something for sure, so getting rid of all that
first would probably help.
>
> @@
> struct intel_crtc_state *T;
> @@
> -T->uapi.enable
> +T->hw.enable
>
> @@
> struct intel_crtc_state *T;
> @@
> -T->uapi.degamma_lut
> +T->hw.degamma_lut
>
>
> @@
> struct intel_crtc_state *T;
> @@
> -T->uapi.gamma_lut
> +T->hw.gamma_lut
>
> @@
> struct intel_crtc_state *T;
> @@
> -T->uapi.ctm
> +T->hw.ctm
>
> @@
> struct intel_crtc_state *T;
> @@
> -T->uapi.mode
> +T->hw.mode
>
> @@
> struct intel_crtc_state *T;
> @@
> -T->uapi.adjusted_mode
> +T->hw.adjusted_mode
>
> I replaced all the instances where we use the uapi members instead of the hw members explicitly in this patch, and came up with the following diff below.
>
> Only the intel_color readout is potentially incorrect, the 2 explicit uapi uses in intel_display.c are needed.
> Didn't fix it because of hw readout, it possibly needs slightly more thought.
>
> Does this satisfy the readability requirements? :)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index ab10c33266bf..cbf4c6e6e661 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -11217,7 +11217,7 @@ int intel_get_load_detect_pipe(struct drm_connector *connector,
> goto fail;
> }
>
> - crtc_state->uapi.active = crtc_state->uapi.enable = true;
> + crtc_state->hw.active = crtc_state->hw.enable = true;
>
> if (!mode)
> mode = &load_detect_mode;
> @@ -13578,7 +13578,7 @@ static int intel_atomic_check(struct drm_device *dev,
> if (!needs_modeset(new_crtc_state))
> continue;
>
> - if (!new_crtc_state->uapi.enable) {
> + if (!new_crtc_state->hw.enable) {
> any_ms = true;
> continue;
> }
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index 5586891572f8..52712bb9ed15 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1623,7 +1623,7 @@ static void i9xx_read_luts(struct intel_crtc_state *crtc_state)
> if (!crtc_state->gamma_enable)
> return;
>
> - crtc_state->uapi.gamma_lut = i9xx_read_lut_8(crtc_state);
> + crtc_state->hw.gamma_lut = i9xx_read_lut_8(crtc_state);
> }
>
> static struct drm_property_blob *
> @@ -1673,9 +1673,9 @@ static void i965_read_luts(struct intel_crtc_state *crtc_state)
> return;
>
> if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT)
> - crtc_state->uapi.gamma_lut = i9xx_read_lut_8(crtc_state);
> + crtc_state->hw.gamma_lut = i9xx_read_lut_8(crtc_state);
> else
> - crtc_state->uapi.gamma_lut = i965_read_lut_10p6(crtc_state);
> + crtc_state->hw.gamma_lut = i965_read_lut_10p6(crtc_state);
> }
>
> static struct drm_property_blob *
> @@ -1715,7 +1715,7 @@ chv_read_cgm_lut(const struct intel_crtc_state *crtc_state)
> static void chv_read_luts(struct intel_crtc_state *crtc_state)
> {
> if (crtc_state->cgm_mode & CGM_PIPE_MODE_GAMMA)
> - crtc_state->uapi.gamma_lut = chv_read_cgm_lut(crtc_state);
> + crtc_state->hw.gamma_lut = chv_read_cgm_lut(crtc_state);
> else
> i965_read_luts(crtc_state);
> }
> @@ -1762,9 +1762,9 @@ static void ilk_read_luts(struct intel_crtc_state *crtc_state)
> return;
>
> if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT)
> - crtc_state->uapi.gamma_lut = i9xx_read_lut_8(crtc_state);
> + crtc_state->hw.gamma_lut = i9xx_read_lut_8(crtc_state);
> else
> - crtc_state->uapi.gamma_lut = ilk_read_lut_10(crtc_state);
> + crtc_state->hw.gamma_lut = ilk_read_lut_10(crtc_state);
> }
>
> static struct drm_property_blob *
> @@ -1811,9 +1811,9 @@ static void glk_read_luts(struct intel_crtc_state *crtc_state)
> return;
>
> if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT)
> - crtc_state->uapi.gamma_lut = i9xx_read_lut_8(crtc_state);
> + crtc_state->hw.gamma_lut = i9xx_read_lut_8(crtc_state);
> else
> - crtc_state->uapi.gamma_lut = glk_read_lut_10(crtc_state, PAL_PREC_INDEX_VALUE(0));
> + crtc_state->hw.gamma_lut = glk_read_lut_10(crtc_state, PAL_PREC_INDEX_VALUE(0));
> }
>
> void intel_color_init(struct intel_crtc *crtc)
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
