On Mon, Mar 13, 2017 at 05:35:42PM +0100, Maarten Lankhorst wrote:
> Op 13-03-17 om 17:22 schreef Ville Syrjälä:
> > On Mon, Mar 13, 2017 at 05:10:28PM +0100, Maarten Lankhorst wrote:
> >> As a proof of concept, first try to convert intel_tv, which is a rarely
> >> used connector. It has 5 properties, tv format and 4 margins.
> >>
> >> I'm less certain about the state behavior itself, should we pass a size
> >> parameter to intel_connector_alloc instead, so duplicate_state
> >> can be done globally if it can be blindly copied?
> >>
> >> Can we also have a atomic_check function for connectors, so the
> >> crtc_state->connectors_changed can be set there? It would be cleaner
> >> and more atomic-like.
> >>
> >> To match the legacy behavior, format can be changed by probing just like
> >> in legacy mode.
> >>
> >> Changes since v1:
> >> - Add intel_encoder->swap_state to allow updating connector state.
> >> - Add intel_tv->format for detect_mode and mode_valid, updated on atomic commit.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/i915/intel_display.c | 15 ++
> >> drivers/gpu/drm/i915/intel_drv.h | 4 +
> >> drivers/gpu/drm/i915/intel_tv.c | 259 +++++++++++++++++++++--------------
> >> 3 files changed, 176 insertions(+), 102 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index ac25c9bc8b81..18b7e7546ee1 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -12954,6 +12954,20 @@ static void intel_atomic_track_fbs(struct drm_atomic_state *state)
> >> to_intel_plane(plane)->frontbuffer_bit);
> >> }
> >>
> >> +static void swap_connector_state(struct drm_atomic_state *state)
> >> +{
> >> + struct drm_connector *connector;
> >> + struct drm_connector_state *new_conn_state;
> >> + int i;
> >> +
> >> + for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> >> + struct intel_connector *conn = to_intel_connector(connector);
> >> +
> >> + if (conn->swap_state)
> >> + conn->swap_state(conn, new_conn_state);
> >> + }
> >> +}
> >> +
> >> /**
> >> * intel_atomic_commit - commit validated state object
> >> * @dev: DRM device
> >> @@ -13012,6 +13026,7 @@ static int intel_atomic_commit(struct drm_device *dev,
> >> dev_priv->cdclk.logical = intel_state->cdclk.logical;
> >> dev_priv->cdclk.actual = intel_state->cdclk.actual;
> >> }
> >> + swap_connector_state(state);
> >>
> >> drm_atomic_state_get(state);
> >> INIT_WORK(&state->commit_work,
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 937623ff6d7c..b7b93799d288 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -307,6 +307,10 @@ struct intel_connector {
> >> * and active (i.e. dpms ON state). */
> >> bool (*get_hw_state)(struct intel_connector *);
> >>
> >> + /* Update device state with the new atomic state. */
> >> + void (*swap_state)(struct intel_connector *,
> >> + struct drm_connector_state *);
> >> +
> >> /* Panel info for eDP and LVDS */
> >> struct intel_panel panel;
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> >> index 6ed1a3ce47b7..b87d51e61975 100644
> >> --- a/drivers/gpu/drm/i915/intel_tv.c
> >> +++ b/drivers/gpu/drm/i915/intel_tv.c
> >> @@ -48,8 +48,6 @@ struct intel_tv {
> >> struct intel_encoder base;
> >>
> >> int type;
> >> - const char *tv_format;
> >> - int margin[4];
> >> u32 save_TV_H_CTL_1;
> >> u32 save_TV_H_CTL_2;
> >> u32 save_TV_H_CTL_3;
> >> @@ -83,8 +81,20 @@ struct intel_tv {
> >>
> >> u32 save_TV_DAC;
> >> u32 save_TV_CTL;
> >> +
> >> + int format;
> >> +};
> >> +
> >> +struct intel_tv_connector_state {
> >> + struct drm_connector_state base;
> >> +
> >> + int format;
> >> + int margin[4];
> >> };
> >>
> >> +#define to_intel_tv_connector_state(state) \
> >> + container_of((state), struct intel_tv_connector_state, base)
> >> +
> >> struct video_levels {
> >> u16 blank, black;
> >> u8 burst;
> >> @@ -848,6 +858,17 @@ intel_tv_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe)
> >> }
> >>
> >> static void
> >> +intel_tv_swap_state(struct intel_connector *connector,
> >> + struct drm_connector_state *conn_state)
> >> +{
> >> + struct intel_tv *intel_tv = intel_attached_tv(&connector->base);
> >> + struct intel_tv_connector_state *tv_state =
> >> + to_intel_tv_connector_state(conn_state);
> >> +
> >> + intel_tv->format = tv_state->format;
> >> +}
> >> +
> >> +static void
> >> intel_enable_tv(struct intel_encoder *encoder,
> >> struct intel_crtc_state *pipe_config,
> >> struct drm_connector_state *conn_state)
> >> @@ -873,32 +894,25 @@ intel_disable_tv(struct intel_encoder *encoder,
> >> I915_WRITE(TV_CTL, I915_READ(TV_CTL) & ~TV_ENC_ENABLE);
> >> }
> >>
> >> -static const struct tv_mode *
> >> -intel_tv_mode_lookup(const char *tv_format)
> >> +static const struct tv_mode *intel_tv_mode_find(struct drm_connector_state *conn_state)
> >> {
> >> - int i;
> >> -
> >> - for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
> >> - const struct tv_mode *tv_mode = &tv_modes[i];
> >> + int format = to_intel_tv_connector_state(conn_state)->format;
> >>
> >> - if (!strcmp(tv_format, tv_mode->name))
> >> - return tv_mode;
> >> - }
> >> - return NULL;
> >> + return &tv_modes[format];
> >> }
> >>
> >> -static const struct tv_mode *
> >> -intel_tv_mode_find(struct intel_tv *intel_tv)
> >> +static const struct tv_mode *intel_tv_mode_find_committed(struct drm_connector *connector)
> >> {
> >> - return intel_tv_mode_lookup(intel_tv->tv_format);
> >> + int format = intel_attached_tv(connector)->format;
> >> +
> >> + return &tv_modes[format];
> >> }
> >>
> >> static enum drm_mode_status
> >> intel_tv_mode_valid(struct drm_connector *connector,
> >> struct drm_display_mode *mode)
> >> {
> >> - struct intel_tv *intel_tv = intel_attached_tv(connector);
> >> - const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> >> + const struct tv_mode *tv_mode = intel_tv_mode_find_committed(connector);
> >> int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
> >>
> >> if (mode->clock > max_dotclk)
> >> @@ -925,8 +939,7 @@ intel_tv_compute_config(struct intel_encoder *encoder,
> >> struct intel_crtc_state *pipe_config,
> >> struct drm_connector_state *conn_state)
> >> {
> >> - struct intel_tv *intel_tv = enc_to_tv(encoder);
> >> - const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> >> + const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
> >>
> >> if (!tv_mode)
> >> return false;
> >> @@ -1032,7 +1045,7 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
> >> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> >> struct intel_tv *intel_tv = enc_to_tv(encoder);
> >> - const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> >> + const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
> >> u32 tv_ctl;
> >> u32 scctl1, scctl2, scctl3;
> >> int i, j;
> >> @@ -1041,6 +1054,8 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
> >> bool burst_ena;
> >> int xpos = 0x0, ypos = 0x0;
> >> unsigned int xsize, ysize;
> >> + struct intel_tv_connector_state *tv_state =
> >> + to_intel_tv_connector_state(conn_state);
> >>
> >> if (!tv_mode)
> >> return; /* can't happen (mode_prepare prevents this) */
> >> @@ -1135,12 +1150,12 @@ static void intel_tv_pre_enable(struct intel_encoder *encoder,
> >> else
> >> ysize = 2*tv_mode->nbr_end + 1;
> >>
> >> - xpos += intel_tv->margin[TV_MARGIN_LEFT];
> >> - ypos += intel_tv->margin[TV_MARGIN_TOP];
> >> - xsize -= (intel_tv->margin[TV_MARGIN_LEFT] +
> >> - intel_tv->margin[TV_MARGIN_RIGHT]);
> >> - ysize -= (intel_tv->margin[TV_MARGIN_TOP] +
> >> - intel_tv->margin[TV_MARGIN_BOTTOM]);
> >> + xpos += tv_state->margin[TV_MARGIN_LEFT];
> >> + ypos += tv_state->margin[TV_MARGIN_TOP];
> >> + xsize -= (tv_state->margin[TV_MARGIN_LEFT] +
> >> + tv_state->margin[TV_MARGIN_RIGHT]);
> >> + ysize -= (tv_state->margin[TV_MARGIN_TOP] +
> >> + tv_state->margin[TV_MARGIN_BOTTOM]);
> >> I915_WRITE(TV_WIN_POS, (xpos<<16)|ypos);
> >> I915_WRITE(TV_WIN_SIZE, (xsize<<16)|ysize);
> >>
> >> @@ -1288,7 +1303,7 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
> >> static void intel_tv_find_better_format(struct drm_connector *connector)
> >> {
> >> struct intel_tv *intel_tv = intel_attached_tv(connector);
> >> - const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
> >> + const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
> > intel_tv_mode_find_committed() ?
> Either works here. We're holding the relevant lock.
Oh you moved the call. Hmm. Do we even need to do that? I thought we
could just leave the in detect as is.
> >
> >> int i;
> >>
> >> if ((intel_tv->type == DRM_MODE_CONNECTOR_Component) ==
> >> @@ -1304,9 +1319,8 @@ static void intel_tv_find_better_format(struct drm_connector *connector)
> >> break;
> >> }
> >>
> >> - intel_tv->tv_format = tv_mode->name;
> >> - drm_object_property_set_value(&connector->base,
> >> - connector->dev->mode_config.tv_mode_property, i);
> >> + to_intel_tv_connector_state(connector->state)->format = i;
> > I was thinking we'd do this in duplicate_state.
> This would cause get_property to return stale values since it would not duplicate the state.
Hmm. So it would return the "old" value between detect and commit, once
commit happens it would again return the value detect() gave us
(assuming the user didn't override it). Is that bad behaviour or not?
Not sure.
I guess I'll need to go read the ddx code to see what, if anything, it
might do with this stuff...
> > With these two changes we shouldn't have to access ->state in detect()
> > AFAICS.
> I think it's the only thing that makes sense.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
