* [PATCH 0/3] drm/i915: Convert tv/dp_mst and crt connector properties to atomic. @ 2017-03-09 13:06 Maarten Lankhorst 2017-03-09 13:06 ` [PATCH 1/3] drm/i915: Convert intel_tv " Maarten Lankhorst ` (3 more replies) 0 siblings, 4 replies; 25+ messages in thread From: Maarten Lankhorst @ 2017-03-09 13:06 UTC (permalink / raw) To: intel-gfx intel_tv is a proof of concept conversion to discuss the API. The other 2 connectors are straightforward. Maarten Lankhorst (3): drm/i915: Convert intel_tv connector properties to atomic. drm/i915: Convert intel_dp_mst connector properties to atomic. drm/i915: Convert intel_crt connector properties to atomic. drivers/gpu/drm/i915/intel_crt.c | 9 +- drivers/gpu/drm/i915/intel_dp_mst.c | 10 +- drivers/gpu/drm/i915/intel_tv.c | 238 ++++++++++++++++++++---------------- 3 files changed, 138 insertions(+), 119 deletions(-) -- 2.7.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic. 2017-03-09 13:06 [PATCH 0/3] drm/i915: Convert tv/dp_mst and crt connector properties to atomic Maarten Lankhorst @ 2017-03-09 13:06 ` Maarten Lankhorst 2017-03-09 17:36 ` Ville Syrjälä 2017-03-09 13:06 ` [PATCH 2/3] drm/i915: Convert intel_dp_mst connector properties to atomic Maarten Lankhorst ` (2 subsequent siblings) 3 siblings, 1 reply; 25+ messages in thread From: Maarten Lankhorst @ 2017-03-09 13:06 UTC (permalink / raw) To: intel-gfx 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. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_tv.c | 238 +++++++++++++++++++++++----------------- 1 file changed, 136 insertions(+), 102 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c index 6ed1a3ce47b7..0fb1d8621fe8 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; @@ -85,6 +83,16 @@ struct intel_tv { u32 save_TV_CTL; }; +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; @@ -873,32 +881,18 @@ 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; + int format = to_intel_tv_connector_state(conn_state)->format; - for (i = 0; i < ARRAY_SIZE(tv_modes); i++) { - const struct tv_mode *tv_mode = &tv_modes[i]; - - if (!strcmp(tv_format, tv_mode->name)) - return tv_mode; - } - return NULL; -} - -static const struct tv_mode * -intel_tv_mode_find(struct intel_tv *intel_tv) -{ - return intel_tv_mode_lookup(intel_tv->tv_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(connector->state); int max_dotclk = to_i915(connector->dev)->max_dotclk_freq; if (mode->clock > max_dotclk) @@ -925,8 +919,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 +1025,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 +1034,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 +1130,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 +1283,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); int i; if ((intel_tv->type == DRM_MODE_CONNECTOR_Component) == @@ -1304,9 +1299,7 @@ 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; } /** @@ -1344,18 +1337,17 @@ intel_tv_detect(struct drm_connector *connector, bool force) } else status = connector_status_unknown; + if (status == connector_status_connected) { + intel_tv->type = type; + intel_tv_find_better_format(connector); + } + drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx); - } else - return connector->status; - if (status != connector_status_connected) return status; - - intel_tv->type = type; - intel_tv_find_better_format(connector); - - return connector_status_connected; + } else + return connector->status; } static const struct input_res { @@ -1378,8 +1370,7 @@ static void intel_tv_chose_preferred_modes(struct drm_connector *connector, struct drm_display_mode *mode_ptr) { - 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); if (tv_mode->nbr_end < 480 && mode_ptr->vdisplay == 480) mode_ptr->type |= DRM_MODE_TYPE_PREFERRED; @@ -1403,8 +1394,7 @@ static int intel_tv_get_modes(struct drm_connector *connector) { struct drm_display_mode *mode_ptr; - 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); int j, count = 0; u64 tmp; @@ -1462,56 +1452,97 @@ intel_tv_destroy(struct drm_connector *connector) kfree(connector); } - static int -intel_tv_set_property(struct drm_connector *connector, struct drm_property *property, - uint64_t val) +intel_tv_atomic_set_property(struct drm_connector *connector, + struct drm_connector_state *conn_state, + struct drm_property *property, + uint64_t val) { struct drm_device *dev = connector->dev; - struct intel_tv *intel_tv = intel_attached_tv(connector); - struct drm_crtc *crtc = intel_tv->base.base.crtc; - int ret = 0; + + struct intel_tv_connector_state *tv_state = + to_intel_tv_connector_state(conn_state); bool changed = false; - ret = drm_object_property_set_value(&connector->base, property, val); - if (ret < 0) - goto out; - - if (property == dev->mode_config.tv_left_margin_property && - intel_tv->margin[TV_MARGIN_LEFT] != val) { - intel_tv->margin[TV_MARGIN_LEFT] = val; - changed = true; - } else if (property == dev->mode_config.tv_right_margin_property && - intel_tv->margin[TV_MARGIN_RIGHT] != val) { - intel_tv->margin[TV_MARGIN_RIGHT] = val; - changed = true; - } else if (property == dev->mode_config.tv_top_margin_property && - intel_tv->margin[TV_MARGIN_TOP] != val) { - intel_tv->margin[TV_MARGIN_TOP] = val; - changed = true; - } else if (property == dev->mode_config.tv_bottom_margin_property && - intel_tv->margin[TV_MARGIN_BOTTOM] != val) { - intel_tv->margin[TV_MARGIN_BOTTOM] = val; - changed = true; + if (property == dev->mode_config.tv_left_margin_property) { + changed = tv_state->margin[TV_MARGIN_LEFT] != val; + + tv_state->margin[TV_MARGIN_LEFT] = val; + } else if (property == dev->mode_config.tv_right_margin_property) { + changed = tv_state->margin[TV_MARGIN_RIGHT] != val; + + tv_state->margin[TV_MARGIN_RIGHT] = val; + } else if (property == dev->mode_config.tv_top_margin_property) { + changed = tv_state->margin[TV_MARGIN_TOP] != val; + + tv_state->margin[TV_MARGIN_TOP] = val; + } else if (property == dev->mode_config.tv_bottom_margin_property) { + changed = tv_state->margin[TV_MARGIN_BOTTOM] != val; + + tv_state->margin[TV_MARGIN_BOTTOM] = val; } else if (property == dev->mode_config.tv_mode_property) { if (val >= ARRAY_SIZE(tv_modes)) { - ret = -EINVAL; - goto out; + DRM_DEBUG_ATOMIC("value %llu out of tv_modes array bounds\n", val); + + return -EINVAL; } - if (!strcmp(intel_tv->tv_format, tv_modes[val].name)) - goto out; - intel_tv->tv_format = tv_modes[val].name; - changed = true; + changed = tv_state->format != val; + tv_state->format = val; + } else { + DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name); + return -EINVAL; + } + + /* Trigger a modeset when connector properties are changed. */ + if (changed && conn_state->crtc) { + struct drm_crtc_state *crtc_state = + drm_atomic_get_existing_crtc_state(conn_state->state, + conn_state->crtc); + + crtc_state->connectors_changed = true; + } + return 0; +} + +static int +intel_tv_atomic_get_property(struct drm_connector *connector, + const struct drm_connector_state *state, + struct drm_property *property, + uint64_t *val) +{ + struct drm_device *dev = connector->dev; + struct intel_tv_connector_state *tv_state = + to_intel_tv_connector_state(state); + + if (property == dev->mode_config.tv_left_margin_property) + *val = tv_state->margin[TV_MARGIN_LEFT]; + else if (property == dev->mode_config.tv_right_margin_property) + *val = tv_state->margin[TV_MARGIN_RIGHT]; + else if (property == dev->mode_config.tv_top_margin_property) + *val = tv_state->margin[TV_MARGIN_TOP]; + else if (property == dev->mode_config.tv_bottom_margin_property) + *val = tv_state->margin[TV_MARGIN_BOTTOM]; + else if (property == dev->mode_config.tv_mode_property) { + *val = tv_state->format; } else { - ret = -EINVAL; - goto out; + DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name); + return -EINVAL; } - if (changed && crtc) - intel_crtc_restore_mode(crtc); -out: - return ret; + return 0; +} + +static struct drm_connector_state * +intel_tv_duplicate_state(struct drm_connector *connector) +{ + struct intel_tv_connector_state *state; + + state = kmemdup(&connector->state, sizeof(*state), GFP_KERNEL); + if (state) + __drm_atomic_helper_connector_duplicate_state(connector, &state->base); + + return &state->base; } static const struct drm_connector_funcs intel_tv_connector_funcs = { @@ -1520,11 +1551,12 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = { .late_register = intel_connector_register, .early_unregister = intel_connector_unregister, .destroy = intel_tv_destroy, - .set_property = intel_tv_set_property, - .atomic_get_property = intel_connector_atomic_get_property, + .set_property = drm_atomic_helper_connector_set_property, + .atomic_set_property = intel_tv_atomic_set_property, + .atomic_get_property = intel_tv_atomic_get_property, .fill_modes = drm_helper_probe_single_connector_modes, .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, + .atomic_duplicate_state = intel_tv_duplicate_state, }; static const struct drm_connector_helper_funcs intel_tv_connector_helper_funcs = { @@ -1547,6 +1579,7 @@ intel_tv_init(struct drm_i915_private *dev_priv) u32 tv_dac_on, tv_dac_off, save_tv_dac; const char *tv_format_names[ARRAY_SIZE(tv_modes)]; int i, initial_mode = 0; + struct intel_tv_connector_state *state; if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED) return; @@ -1580,16 +1613,17 @@ intel_tv_init(struct drm_i915_private *dev_priv) return; intel_tv = kzalloc(sizeof(*intel_tv), GFP_KERNEL); - if (!intel_tv) { - return; - } - - intel_connector = intel_connector_alloc(); - if (!intel_connector) { + intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL); + state = kzalloc(sizeof(*state), GFP_KERNEL); + if (!intel_tv || !intel_connector || !state) { + kfree(intel_connector); + kfree(state); kfree(intel_tv); return; } + __drm_atomic_helper_connector_reset(&intel_connector->base, &state->base); + intel_encoder = &intel_tv->base; connector = &intel_connector->base; @@ -1629,12 +1663,12 @@ intel_tv_init(struct drm_i915_private *dev_priv) intel_tv->type = DRM_MODE_CONNECTOR_Unknown; /* BIOS margin values */ - intel_tv->margin[TV_MARGIN_LEFT] = 54; - intel_tv->margin[TV_MARGIN_TOP] = 36; - intel_tv->margin[TV_MARGIN_RIGHT] = 46; - intel_tv->margin[TV_MARGIN_BOTTOM] = 37; + state->margin[TV_MARGIN_LEFT] = 54; + state->margin[TV_MARGIN_TOP] = 36; + state->margin[TV_MARGIN_RIGHT] = 46; + state->margin[TV_MARGIN_BOTTOM] = 37; - intel_tv->tv_format = tv_modes[initial_mode].name; + state->format = initial_mode; drm_connector_helper_add(connector, &intel_tv_connector_helper_funcs); connector->interlace_allowed = false; @@ -1648,17 +1682,17 @@ intel_tv_init(struct drm_i915_private *dev_priv) tv_format_names); drm_object_attach_property(&connector->base, dev->mode_config.tv_mode_property, - initial_mode); + state->format); drm_object_attach_property(&connector->base, dev->mode_config.tv_left_margin_property, - intel_tv->margin[TV_MARGIN_LEFT]); + state->margin[TV_MARGIN_LEFT]); drm_object_attach_property(&connector->base, dev->mode_config.tv_top_margin_property, - intel_tv->margin[TV_MARGIN_TOP]); + state->margin[TV_MARGIN_TOP]); drm_object_attach_property(&connector->base, dev->mode_config.tv_right_margin_property, - intel_tv->margin[TV_MARGIN_RIGHT]); + state->margin[TV_MARGIN_RIGHT]); drm_object_attach_property(&connector->base, dev->mode_config.tv_bottom_margin_property, - intel_tv->margin[TV_MARGIN_BOTTOM]); + state->margin[TV_MARGIN_BOTTOM]); } -- 2.7.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic. 2017-03-09 13:06 ` [PATCH 1/3] drm/i915: Convert intel_tv " Maarten Lankhorst @ 2017-03-09 17:36 ` Ville Syrjälä 2017-03-13 9:22 ` Maarten Lankhorst 0 siblings, 1 reply; 25+ messages in thread From: Ville Syrjälä @ 2017-03-09 17:36 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx On Thu, Mar 09, 2017 at 02:06:15PM +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. Since it's so rare, if you want someone to actually test the code it'll probably make sense to pick another connector ;) > > 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. Hmm. I think it migth be really useful only if we have some interactions between multiple properties that really need to be checked. We might have those already I suppose but we don't seem to check any of it currently. So as a first step I guess we can just keep ignoring any such issues. > > To match the legacy behavior, format can be changed by probing just like > in legacy mode. Self modifying state irks me, but it's what we've been doing so I guess we should keep it. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_tv.c | 238 +++++++++++++++++++++++----------------- > 1 file changed, 136 insertions(+), 102 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c > index 6ed1a3ce47b7..0fb1d8621fe8 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; > @@ -85,6 +83,16 @@ struct intel_tv { > u32 save_TV_CTL; > }; > > +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; > @@ -873,32 +881,18 @@ 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; > + int format = to_intel_tv_connector_state(conn_state)->format; > > - for (i = 0; i < ARRAY_SIZE(tv_modes); i++) { > - const struct tv_mode *tv_mode = &tv_modes[i]; > - > - if (!strcmp(tv_format, tv_mode->name)) > - return tv_mode; > - } > - return NULL; > -} > - > -static const struct tv_mode * > -intel_tv_mode_find(struct intel_tv *intel_tv) > -{ > - return intel_tv_mode_lookup(intel_tv->tv_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(connector->state); It feels a bit fishy to use the state here. Generally that's a no-no. But in this case I wonder if it's the right choice after all. Not sure if some kind of "automatic" enum value might also work. It would at least avoid the self modifying property problem. Although I wonder if the user would still like to know what was actually used if they chose they automatic mode, so we might need a self modifying RO property for the current mode anyway. But that still leaves the problem of how the user would know which modes they should be able to use if .get_modes()/.mode_valid() doesn't respect the users choice of the tv format. Hmm, tricky. Might be the self modifying property is the only good choice. But if we would use the state here, what's the story with locking going to be? connection_mutex is what protects this stuff, but we're not holding that during mode enumeration. > int max_dotclk = to_i915(connector->dev)->max_dotclk_freq; > > if (mode->clock > max_dotclk) > @@ -925,8 +919,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 +1025,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 +1034,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 +1130,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 +1283,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); > int i; > > if ((intel_tv->type == DRM_MODE_CONNECTOR_Component) == > @@ -1304,9 +1299,7 @@ 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; > } > > /** > @@ -1344,18 +1337,17 @@ intel_tv_detect(struct drm_connector *connector, bool force) > } else > status = connector_status_unknown; > > + if (status == connector_status_connected) { > + intel_tv->type = type; > + intel_tv_find_better_format(connector); > + } > + > drm_modeset_drop_locks(&ctx); > drm_modeset_acquire_fini(&ctx); > - } else > - return connector->status; > > - if (status != connector_status_connected) > return status; > - > - intel_tv->type = type; > - intel_tv_find_better_format(connector); > - > - return connector_status_connected; > + } else > + return connector->status; > } > > static const struct input_res { > @@ -1378,8 +1370,7 @@ static void > intel_tv_chose_preferred_modes(struct drm_connector *connector, > struct drm_display_mode *mode_ptr) > { > - 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); > > if (tv_mode->nbr_end < 480 && mode_ptr->vdisplay == 480) > mode_ptr->type |= DRM_MODE_TYPE_PREFERRED; > @@ -1403,8 +1394,7 @@ static int > intel_tv_get_modes(struct drm_connector *connector) > { > struct drm_display_mode *mode_ptr; > - 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); > int j, count = 0; > u64 tmp; > > @@ -1462,56 +1452,97 @@ intel_tv_destroy(struct drm_connector *connector) > kfree(connector); > } > > - > static int > -intel_tv_set_property(struct drm_connector *connector, struct drm_property *property, > - uint64_t val) > +intel_tv_atomic_set_property(struct drm_connector *connector, > + struct drm_connector_state *conn_state, > + struct drm_property *property, > + uint64_t val) > { > struct drm_device *dev = connector->dev; > - struct intel_tv *intel_tv = intel_attached_tv(connector); > - struct drm_crtc *crtc = intel_tv->base.base.crtc; > - int ret = 0; > + > + struct intel_tv_connector_state *tv_state = > + to_intel_tv_connector_state(conn_state); > bool changed = false; > > - ret = drm_object_property_set_value(&connector->base, property, val); > - if (ret < 0) > - goto out; > - > - if (property == dev->mode_config.tv_left_margin_property && > - intel_tv->margin[TV_MARGIN_LEFT] != val) { > - intel_tv->margin[TV_MARGIN_LEFT] = val; > - changed = true; > - } else if (property == dev->mode_config.tv_right_margin_property && > - intel_tv->margin[TV_MARGIN_RIGHT] != val) { > - intel_tv->margin[TV_MARGIN_RIGHT] = val; > - changed = true; > - } else if (property == dev->mode_config.tv_top_margin_property && > - intel_tv->margin[TV_MARGIN_TOP] != val) { > - intel_tv->margin[TV_MARGIN_TOP] = val; > - changed = true; > - } else if (property == dev->mode_config.tv_bottom_margin_property && > - intel_tv->margin[TV_MARGIN_BOTTOM] != val) { > - intel_tv->margin[TV_MARGIN_BOTTOM] = val; > - changed = true; > + if (property == dev->mode_config.tv_left_margin_property) { > + changed = tv_state->margin[TV_MARGIN_LEFT] != val; > + > + tv_state->margin[TV_MARGIN_LEFT] = val; > + } else if (property == dev->mode_config.tv_right_margin_property) { > + changed = tv_state->margin[TV_MARGIN_RIGHT] != val; > + > + tv_state->margin[TV_MARGIN_RIGHT] = val; > + } else if (property == dev->mode_config.tv_top_margin_property) { > + changed = tv_state->margin[TV_MARGIN_TOP] != val; > + > + tv_state->margin[TV_MARGIN_TOP] = val; > + } else if (property == dev->mode_config.tv_bottom_margin_property) { > + changed = tv_state->margin[TV_MARGIN_BOTTOM] != val; > + > + tv_state->margin[TV_MARGIN_BOTTOM] = val; > } else if (property == dev->mode_config.tv_mode_property) { > if (val >= ARRAY_SIZE(tv_modes)) { > - ret = -EINVAL; > - goto out; > + DRM_DEBUG_ATOMIC("value %llu out of tv_modes array bounds\n", val); > + > + return -EINVAL; > } > - if (!strcmp(intel_tv->tv_format, tv_modes[val].name)) > - goto out; > > - intel_tv->tv_format = tv_modes[val].name; > - changed = true; > + changed = tv_state->format != val; > + tv_state->format = val; > + } else { > + DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name); > + return -EINVAL; > + } > + > + /* Trigger a modeset when connector properties are changed. */ > + if (changed && conn_state->crtc) { > + struct drm_crtc_state *crtc_state = > + drm_atomic_get_existing_crtc_state(conn_state->state, > + conn_state->crtc); > + > + crtc_state->connectors_changed = true; > + } > + return 0; > +} > + > +static int > +intel_tv_atomic_get_property(struct drm_connector *connector, > + const struct drm_connector_state *state, > + struct drm_property *property, > + uint64_t *val) > +{ > + struct drm_device *dev = connector->dev; > + struct intel_tv_connector_state *tv_state = > + to_intel_tv_connector_state(state); > + > + if (property == dev->mode_config.tv_left_margin_property) > + *val = tv_state->margin[TV_MARGIN_LEFT]; > + else if (property == dev->mode_config.tv_right_margin_property) > + *val = tv_state->margin[TV_MARGIN_RIGHT]; > + else if (property == dev->mode_config.tv_top_margin_property) > + *val = tv_state->margin[TV_MARGIN_TOP]; > + else if (property == dev->mode_config.tv_bottom_margin_property) > + *val = tv_state->margin[TV_MARGIN_BOTTOM]; > + else if (property == dev->mode_config.tv_mode_property) { > + *val = tv_state->format; > } else { > - ret = -EINVAL; > - goto out; > + DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name); > + return -EINVAL; > } > > - if (changed && crtc) > - intel_crtc_restore_mode(crtc); > -out: > - return ret; > + return 0; > +} > + > +static struct drm_connector_state * > +intel_tv_duplicate_state(struct drm_connector *connector) > +{ > + struct intel_tv_connector_state *state; > + > + state = kmemdup(&connector->state, sizeof(*state), GFP_KERNEL); > + if (state) > + __drm_atomic_helper_connector_duplicate_state(connector, &state->base); > + > + return &state->base; > } > > static const struct drm_connector_funcs intel_tv_connector_funcs = { > @@ -1520,11 +1551,12 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = { > .late_register = intel_connector_register, > .early_unregister = intel_connector_unregister, > .destroy = intel_tv_destroy, > - .set_property = intel_tv_set_property, > - .atomic_get_property = intel_connector_atomic_get_property, > + .set_property = drm_atomic_helper_connector_set_property, > + .atomic_set_property = intel_tv_atomic_set_property, > + .atomic_get_property = intel_tv_atomic_get_property, > .fill_modes = drm_helper_probe_single_connector_modes, > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > + .atomic_duplicate_state = intel_tv_duplicate_state, > }; > > static const struct drm_connector_helper_funcs intel_tv_connector_helper_funcs = { > @@ -1547,6 +1579,7 @@ intel_tv_init(struct drm_i915_private *dev_priv) > u32 tv_dac_on, tv_dac_off, save_tv_dac; > const char *tv_format_names[ARRAY_SIZE(tv_modes)]; > int i, initial_mode = 0; > + struct intel_tv_connector_state *state; > > if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED) > return; > @@ -1580,16 +1613,17 @@ intel_tv_init(struct drm_i915_private *dev_priv) > return; > > intel_tv = kzalloc(sizeof(*intel_tv), GFP_KERNEL); > - if (!intel_tv) { > - return; > - } > - > - intel_connector = intel_connector_alloc(); > - if (!intel_connector) { > + intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL); > + state = kzalloc(sizeof(*state), GFP_KERNEL); > + if (!intel_tv || !intel_connector || !state) { > + kfree(intel_connector); > + kfree(state); > kfree(intel_tv); > return; > } > > + __drm_atomic_helper_connector_reset(&intel_connector->base, &state->base); > + > intel_encoder = &intel_tv->base; > connector = &intel_connector->base; > > @@ -1629,12 +1663,12 @@ intel_tv_init(struct drm_i915_private *dev_priv) > intel_tv->type = DRM_MODE_CONNECTOR_Unknown; > > /* BIOS margin values */ > - intel_tv->margin[TV_MARGIN_LEFT] = 54; > - intel_tv->margin[TV_MARGIN_TOP] = 36; > - intel_tv->margin[TV_MARGIN_RIGHT] = 46; > - intel_tv->margin[TV_MARGIN_BOTTOM] = 37; > + state->margin[TV_MARGIN_LEFT] = 54; > + state->margin[TV_MARGIN_TOP] = 36; > + state->margin[TV_MARGIN_RIGHT] = 46; > + state->margin[TV_MARGIN_BOTTOM] = 37; > > - intel_tv->tv_format = tv_modes[initial_mode].name; > + state->format = initial_mode; > > drm_connector_helper_add(connector, &intel_tv_connector_helper_funcs); > connector->interlace_allowed = false; > @@ -1648,17 +1682,17 @@ intel_tv_init(struct drm_i915_private *dev_priv) > tv_format_names); > > drm_object_attach_property(&connector->base, dev->mode_config.tv_mode_property, > - initial_mode); > + state->format); > drm_object_attach_property(&connector->base, > dev->mode_config.tv_left_margin_property, > - intel_tv->margin[TV_MARGIN_LEFT]); > + state->margin[TV_MARGIN_LEFT]); > drm_object_attach_property(&connector->base, > dev->mode_config.tv_top_margin_property, > - intel_tv->margin[TV_MARGIN_TOP]); > + state->margin[TV_MARGIN_TOP]); > drm_object_attach_property(&connector->base, > dev->mode_config.tv_right_margin_property, > - intel_tv->margin[TV_MARGIN_RIGHT]); > + state->margin[TV_MARGIN_RIGHT]); > drm_object_attach_property(&connector->base, > dev->mode_config.tv_bottom_margin_property, > - intel_tv->margin[TV_MARGIN_BOTTOM]); > + state->margin[TV_MARGIN_BOTTOM]); > } > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic. 2017-03-09 17:36 ` Ville Syrjälä @ 2017-03-13 9:22 ` Maarten Lankhorst 2017-03-13 9:29 ` Ville Syrjälä 0 siblings, 1 reply; 25+ messages in thread From: Maarten Lankhorst @ 2017-03-13 9:22 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx Op 09-03-17 om 18:36 schreef Ville Syrjälä: > On Thu, Mar 09, 2017 at 02:06:15PM +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. > Since it's so rare, if you want someone to actually test the code > it'll probably make sense to pick another connector ;) Yeah but the properties are among the most annoying, with the self modifying code and using state in mode_detect(). >> 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. > Hmm. I think it migth be really useful only if we have some > interactions between multiple properties that really need to be > checked. We might have those already I suppose but we don't seem > to check any of it currently. So as a first step I guess we can > just keep ignoring any such issues. Well it might be, for example not all properties may be set yet so you can only do a sane check in a separate step. >> To match the legacy behavior, format can be changed by probing just like >> in legacy mode. > Self modifying state irks me, but it's what we've been doing so I guess > we should keep it. Yeah, I hate it too. >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_tv.c | 238 +++++++++++++++++++++++----------------- >> 1 file changed, 136 insertions(+), 102 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c >> index 6ed1a3ce47b7..0fb1d8621fe8 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; >> @@ -85,6 +83,16 @@ struct intel_tv { >> u32 save_TV_CTL; >> }; >> >> +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; >> @@ -873,32 +881,18 @@ 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; >> + int format = to_intel_tv_connector_state(conn_state)->format; >> >> - for (i = 0; i < ARRAY_SIZE(tv_modes); i++) { >> - const struct tv_mode *tv_mode = &tv_modes[i]; >> - >> - if (!strcmp(tv_format, tv_mode->name)) >> - return tv_mode; >> - } >> - return NULL; >> -} >> - >> -static const struct tv_mode * >> -intel_tv_mode_find(struct intel_tv *intel_tv) >> -{ >> - return intel_tv_mode_lookup(intel_tv->tv_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(connector->state); > It feels a bit fishy to use the state here. Generally that's a no-no. > But in this case I wonder if it's the right choice after all. > > Not sure if some kind of "automatic" enum value might also work. It > would at least avoid the self modifying property problem. Although I > wonder if the user would still like to know what was actually used > if they chose they automatic mode, so we might need a self modifying > RO property for the current mode anyway. > > But that still leaves the problem of how the user would know which modes > they should be able to use if .get_modes()/.mode_valid() doesn't respect > the users choice of the tv format. Hmm, tricky. Might be the self > modifying property is the only good choice. > > But if we would use the state here, what's the story with locking going > to be? connection_mutex is what protects this stuff, but we're not > holding that during mode enumeration. Yeah locking is tricky, honestly I have no idea what would be the right thing to do here.. I don't really see a good solution, or at least one that would work correctly with atomic properties. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic. 2017-03-13 9:22 ` Maarten Lankhorst @ 2017-03-13 9:29 ` Ville Syrjälä 2017-03-13 10:38 ` Maarten Lankhorst 0 siblings, 1 reply; 25+ messages in thread From: Ville Syrjälä @ 2017-03-13 9:29 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx On Mon, Mar 13, 2017 at 10:22:51AM +0100, Maarten Lankhorst wrote: > Op 09-03-17 om 18:36 schreef Ville Syrjälä: > > On Thu, Mar 09, 2017 at 02:06:15PM +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. > > Since it's so rare, if you want someone to actually test the code > > it'll probably make sense to pick another connector ;) > Yeah but the properties are among the most annoying, with the self modifying code and using state in mode_detect(). > >> 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. > > Hmm. I think it migth be really useful only if we have some > > interactions between multiple properties that really need to be > > checked. We might have those already I suppose but we don't seem > > to check any of it currently. So as a first step I guess we can > > just keep ignoring any such issues. > Well it might be, for example not all properties may be set yet so you can only do a sane check in a separate step. > >> To match the legacy behavior, format can be changed by probing just like > >> in legacy mode. > > Self modifying state irks me, but it's what we've been doing so I guess > > we should keep it. > Yeah, I hate it too. > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_tv.c | 238 +++++++++++++++++++++++----------------- > >> 1 file changed, 136 insertions(+), 102 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c > >> index 6ed1a3ce47b7..0fb1d8621fe8 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; > >> @@ -85,6 +83,16 @@ struct intel_tv { > >> u32 save_TV_CTL; > >> }; > >> > >> +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; > >> @@ -873,32 +881,18 @@ 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; > >> + int format = to_intel_tv_connector_state(conn_state)->format; > >> > >> - for (i = 0; i < ARRAY_SIZE(tv_modes); i++) { > >> - const struct tv_mode *tv_mode = &tv_modes[i]; > >> - > >> - if (!strcmp(tv_format, tv_mode->name)) > >> - return tv_mode; > >> - } > >> - return NULL; > >> -} > >> - > >> -static const struct tv_mode * > >> -intel_tv_mode_find(struct intel_tv *intel_tv) > >> -{ > >> - return intel_tv_mode_lookup(intel_tv->tv_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(connector->state); > > It feels a bit fishy to use the state here. Generally that's a no-no. > > But in this case I wonder if it's the right choice after all. > > > > Not sure if some kind of "automatic" enum value might also work. It > > would at least avoid the self modifying property problem. Although I > > wonder if the user would still like to know what was actually used > > if they chose they automatic mode, so we might need a self modifying > > RO property for the current mode anyway. > > > > But that still leaves the problem of how the user would know which modes > > they should be able to use if .get_modes()/.mode_valid() doesn't respect > > the users choice of the tv format. Hmm, tricky. Might be the self > > modifying property is the only good choice. > > > > But if we would use the state here, what's the story with locking going > > to be? connection_mutex is what protects this stuff, but we're not > > holding that during mode enumeration. > Yeah locking is tricky, honestly I have no idea what would be the right thing to do here.. > > I don't really see a good solution, or at least one that would work correctly with atomic properties. Maybe we need to keep the format information in both intel_tv and the state. We'd use the intel_tv->format during detect, and during duplicate_state we'd do 'state->format = intel_tv->format' and during commit we'd do 'intel_tv->format = state->format' ? Still self modifying, and somewhat racy still, but at least we shouldn't explode on account of the connector->state dereference. -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic. 2017-03-13 9:29 ` Ville Syrjälä @ 2017-03-13 10:38 ` Maarten Lankhorst 2017-03-13 10:55 ` Ville Syrjälä 0 siblings, 1 reply; 25+ messages in thread From: Maarten Lankhorst @ 2017-03-13 10:38 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx Op 13-03-17 om 10:29 schreef Ville Syrjälä: > On Mon, Mar 13, 2017 at 10:22:51AM +0100, Maarten Lankhorst wrote: >> Op 09-03-17 om 18:36 schreef Ville Syrjälä: >>> On Thu, Mar 09, 2017 at 02:06:15PM +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. >>> Since it's so rare, if you want someone to actually test the code >>> it'll probably make sense to pick another connector ;) >> Yeah but the properties are among the most annoying, with the self modifying code and using state in mode_detect(). >>>> 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. >>> Hmm. I think it migth be really useful only if we have some >>> interactions between multiple properties that really need to be >>> checked. We might have those already I suppose but we don't seem >>> to check any of it currently. So as a first step I guess we can >>> just keep ignoring any such issues. >> Well it might be, for example not all properties may be set yet so you can only do a sane check in a separate step. >>>> To match the legacy behavior, format can be changed by probing just like >>>> in legacy mode. >>> Self modifying state irks me, but it's what we've been doing so I guess >>> we should keep it. >> Yeah, I hate it too. >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_tv.c | 238 +++++++++++++++++++++++----------------- >>>> 1 file changed, 136 insertions(+), 102 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c >>>> index 6ed1a3ce47b7..0fb1d8621fe8 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; >>>> @@ -85,6 +83,16 @@ struct intel_tv { >>>> u32 save_TV_CTL; >>>> }; >>>> >>>> +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; >>>> @@ -873,32 +881,18 @@ 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; >>>> + int format = to_intel_tv_connector_state(conn_state)->format; >>>> >>>> - for (i = 0; i < ARRAY_SIZE(tv_modes); i++) { >>>> - const struct tv_mode *tv_mode = &tv_modes[i]; >>>> - >>>> - if (!strcmp(tv_format, tv_mode->name)) >>>> - return tv_mode; >>>> - } >>>> - return NULL; >>>> -} >>>> - >>>> -static const struct tv_mode * >>>> -intel_tv_mode_find(struct intel_tv *intel_tv) >>>> -{ >>>> - return intel_tv_mode_lookup(intel_tv->tv_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(connector->state); >>> It feels a bit fishy to use the state here. Generally that's a no-no. >>> But in this case I wonder if it's the right choice after all. >>> >>> Not sure if some kind of "automatic" enum value might also work. It >>> would at least avoid the self modifying property problem. Although I >>> wonder if the user would still like to know what was actually used >>> if they chose they automatic mode, so we might need a self modifying >>> RO property for the current mode anyway. >>> >>> But that still leaves the problem of how the user would know which modes >>> they should be able to use if .get_modes()/.mode_valid() doesn't respect >>> the users choice of the tv format. Hmm, tricky. Might be the self >>> modifying property is the only good choice. >>> >>> But if we would use the state here, what's the story with locking going >>> to be? connection_mutex is what protects this stuff, but we're not >>> holding that during mode enumeration. >> Yeah locking is tricky, honestly I have no idea what would be the right thing to do here.. >> >> I don't really see a good solution, or at least one that would work correctly with atomic properties. > Maybe we need to keep the format information in both intel_tv and the > state. We'd use the intel_tv->format during detect, and during > duplicate_state we'd do 'state->format = intel_tv->format' and during > commit we'd do 'intel_tv->format = state->format' ? > > Still self modifying, and somewhat racy still, but at least we > shouldn't explode on account of the connector->state dereference. > I thought about that, but where do you want to update it in atomic commit? _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic. 2017-03-13 10:38 ` Maarten Lankhorst @ 2017-03-13 10:55 ` Ville Syrjälä 2017-03-13 12:19 ` Maarten Lankhorst 2017-03-13 16:10 ` [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2 Maarten Lankhorst 0 siblings, 2 replies; 25+ messages in thread From: Ville Syrjälä @ 2017-03-13 10:55 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx On Mon, Mar 13, 2017 at 11:38:33AM +0100, Maarten Lankhorst wrote: > Op 13-03-17 om 10:29 schreef Ville Syrjälä: > > On Mon, Mar 13, 2017 at 10:22:51AM +0100, Maarten Lankhorst wrote: > >> Op 09-03-17 om 18:36 schreef Ville Syrjälä: > >>> On Thu, Mar 09, 2017 at 02:06:15PM +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. > >>> Since it's so rare, if you want someone to actually test the code > >>> it'll probably make sense to pick another connector ;) > >> Yeah but the properties are among the most annoying, with the self modifying code and using state in mode_detect(). > >>>> 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. > >>> Hmm. I think it migth be really useful only if we have some > >>> interactions between multiple properties that really need to be > >>> checked. We might have those already I suppose but we don't seem > >>> to check any of it currently. So as a first step I guess we can > >>> just keep ignoring any such issues. > >> Well it might be, for example not all properties may be set yet so you can only do a sane check in a separate step. > >>>> To match the legacy behavior, format can be changed by probing just like > >>>> in legacy mode. > >>> Self modifying state irks me, but it's what we've been doing so I guess > >>> we should keep it. > >> Yeah, I hate it too. > >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>> --- > >>>> drivers/gpu/drm/i915/intel_tv.c | 238 +++++++++++++++++++++++----------------- > >>>> 1 file changed, 136 insertions(+), 102 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c > >>>> index 6ed1a3ce47b7..0fb1d8621fe8 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; > >>>> @@ -85,6 +83,16 @@ struct intel_tv { > >>>> u32 save_TV_CTL; > >>>> }; > >>>> > >>>> +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; > >>>> @@ -873,32 +881,18 @@ 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; > >>>> + int format = to_intel_tv_connector_state(conn_state)->format; > >>>> > >>>> - for (i = 0; i < ARRAY_SIZE(tv_modes); i++) { > >>>> - const struct tv_mode *tv_mode = &tv_modes[i]; > >>>> - > >>>> - if (!strcmp(tv_format, tv_mode->name)) > >>>> - return tv_mode; > >>>> - } > >>>> - return NULL; > >>>> -} > >>>> - > >>>> -static const struct tv_mode * > >>>> -intel_tv_mode_find(struct intel_tv *intel_tv) > >>>> -{ > >>>> - return intel_tv_mode_lookup(intel_tv->tv_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(connector->state); > >>> It feels a bit fishy to use the state here. Generally that's a no-no. > >>> But in this case I wonder if it's the right choice after all. > >>> > >>> Not sure if some kind of "automatic" enum value might also work. It > >>> would at least avoid the self modifying property problem. Although I > >>> wonder if the user would still like to know what was actually used > >>> if they chose they automatic mode, so we might need a self modifying > >>> RO property for the current mode anyway. > >>> > >>> But that still leaves the problem of how the user would know which modes > >>> they should be able to use if .get_modes()/.mode_valid() doesn't respect > >>> the users choice of the tv format. Hmm, tricky. Might be the self > >>> modifying property is the only good choice. > >>> > >>> But if we would use the state here, what's the story with locking going > >>> to be? connection_mutex is what protects this stuff, but we're not > >>> holding that during mode enumeration. > >> Yeah locking is tricky, honestly I have no idea what would be the right thing to do here.. > >> > >> I don't really see a good solution, or at least one that would work correctly with atomic properties. > > Maybe we need to keep the format information in both intel_tv and the > > state. We'd use the intel_tv->format during detect, and during > > duplicate_state we'd do 'state->format = intel_tv->format' and during > > commit we'd do 'intel_tv->format = state->format' ? > > > > Still self modifying, and somewhat racy still, but at least we > > shouldn't explode on account of the connector->state dereference. > > > I thought about that, but where do you want to update it in atomic commit? Does it matter? Somewhere around the swap_state I suppose? -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic. 2017-03-13 10:55 ` Ville Syrjälä @ 2017-03-13 12:19 ` Maarten Lankhorst 2017-03-13 16:10 ` [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2 Maarten Lankhorst 1 sibling, 0 replies; 25+ messages in thread From: Maarten Lankhorst @ 2017-03-13 12:19 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx Op 13-03-17 om 11:55 schreef Ville Syrjälä: > On Mon, Mar 13, 2017 at 11:38:33AM +0100, Maarten Lankhorst wrote: >> Op 13-03-17 om 10:29 schreef Ville Syrjälä: >>> On Mon, Mar 13, 2017 at 10:22:51AM +0100, Maarten Lankhorst wrote: >>>> Op 09-03-17 om 18:36 schreef Ville Syrjälä: >>>>> On Thu, Mar 09, 2017 at 02:06:15PM +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. >>>>> Since it's so rare, if you want someone to actually test the code >>>>> it'll probably make sense to pick another connector ;) >>>> Yeah but the properties are among the most annoying, with the self modifying code and using state in mode_detect(). >>>>>> 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. >>>>> Hmm. I think it migth be really useful only if we have some >>>>> interactions between multiple properties that really need to be >>>>> checked. We might have those already I suppose but we don't seem >>>>> to check any of it currently. So as a first step I guess we can >>>>> just keep ignoring any such issues. >>>> Well it might be, for example not all properties may be set yet so you can only do a sane check in a separate step. >>>>>> To match the legacy behavior, format can be changed by probing just like >>>>>> in legacy mode. >>>>> Self modifying state irks me, but it's what we've been doing so I guess >>>>> we should keep it. >>>> Yeah, I hate it too. >>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>>>> --- >>>>>> drivers/gpu/drm/i915/intel_tv.c | 238 +++++++++++++++++++++++----------------- >>>>>> 1 file changed, 136 insertions(+), 102 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c >>>>>> index 6ed1a3ce47b7..0fb1d8621fe8 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; >>>>>> @@ -85,6 +83,16 @@ struct intel_tv { >>>>>> u32 save_TV_CTL; >>>>>> }; >>>>>> >>>>>> +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; >>>>>> @@ -873,32 +881,18 @@ 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; >>>>>> + int format = to_intel_tv_connector_state(conn_state)->format; >>>>>> >>>>>> - for (i = 0; i < ARRAY_SIZE(tv_modes); i++) { >>>>>> - const struct tv_mode *tv_mode = &tv_modes[i]; >>>>>> - >>>>>> - if (!strcmp(tv_format, tv_mode->name)) >>>>>> - return tv_mode; >>>>>> - } >>>>>> - return NULL; >>>>>> -} >>>>>> - >>>>>> -static const struct tv_mode * >>>>>> -intel_tv_mode_find(struct intel_tv *intel_tv) >>>>>> -{ >>>>>> - return intel_tv_mode_lookup(intel_tv->tv_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(connector->state); >>>>> It feels a bit fishy to use the state here. Generally that's a no-no. >>>>> But in this case I wonder if it's the right choice after all. >>>>> >>>>> Not sure if some kind of "automatic" enum value might also work. It >>>>> would at least avoid the self modifying property problem. Although I >>>>> wonder if the user would still like to know what was actually used >>>>> if they chose they automatic mode, so we might need a self modifying >>>>> RO property for the current mode anyway. >>>>> >>>>> But that still leaves the problem of how the user would know which modes >>>>> they should be able to use if .get_modes()/.mode_valid() doesn't respect >>>>> the users choice of the tv format. Hmm, tricky. Might be the self >>>>> modifying property is the only good choice. >>>>> >>>>> But if we would use the state here, what's the story with locking going >>>>> to be? connection_mutex is what protects this stuff, but we're not >>>>> holding that during mode enumeration. >>>> Yeah locking is tricky, honestly I have no idea what would be the right thing to do here.. >>>> >>>> I don't really see a good solution, or at least one that would work correctly with atomic properties. >>> Maybe we need to keep the format information in both intel_tv and the >>> state. We'd use the intel_tv->format during detect, and during >>> duplicate_state we'd do 'state->format = intel_tv->format' and during >>> commit we'd do 'intel_tv->format = state->format' ? >>> >>> Still self modifying, and somewhat racy still, but at least we >>> shouldn't explode on account of the connector->state dereference. >>> >> I thought about that, but where do you want to update it in atomic commit? > Does it matter? Somewhere around the swap_state I suppose? > Hmm, I guess I'll add a connector->set_state hook for this, lets see how far I can get with a less racy v2.. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2. 2017-03-13 10:55 ` Ville Syrjälä 2017-03-13 12:19 ` Maarten Lankhorst @ 2017-03-13 16:10 ` Maarten Lankhorst 2017-03-13 16:22 ` Ville Syrjälä 2017-03-14 18:36 ` Ville Syrjälä 1 sibling, 2 replies; 25+ messages in thread From: Maarten Lankhorst @ 2017-03-13 16:10 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx 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@linux.intel.com> --- 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); 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; + intel_attached_tv(connector)->format = i; } /** @@ -1344,18 +1358,17 @@ intel_tv_detect(struct drm_connector *connector, bool force) } else status = connector_status_unknown; + if (status == connector_status_connected) { + intel_tv->type = type; + intel_tv_find_better_format(connector); + } + drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx); - } else - return connector->status; - if (status != connector_status_connected) return status; - - intel_tv->type = type; - intel_tv_find_better_format(connector); - - return connector_status_connected; + } else + return connector->status; } static const struct input_res { @@ -1375,12 +1388,9 @@ static const struct input_res { * Chose preferred mode according to line number of TV format */ static void -intel_tv_chose_preferred_modes(struct drm_connector *connector, +intel_tv_chose_preferred_modes(const struct tv_mode *tv_mode, struct drm_display_mode *mode_ptr) { - struct intel_tv *intel_tv = intel_attached_tv(connector); - const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv); - if (tv_mode->nbr_end < 480 && mode_ptr->vdisplay == 480) mode_ptr->type |= DRM_MODE_TYPE_PREFERRED; else if (tv_mode->nbr_end > 480) { @@ -1403,8 +1413,7 @@ static int intel_tv_get_modes(struct drm_connector *connector) { struct drm_display_mode *mode_ptr; - 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 j, count = 0; u64 tmp; @@ -1447,7 +1456,7 @@ intel_tv_get_modes(struct drm_connector *connector) mode_ptr->clock = (int) tmp; mode_ptr->type = DRM_MODE_TYPE_DRIVER; - intel_tv_chose_preferred_modes(connector, mode_ptr); + intel_tv_chose_preferred_modes(tv_mode, mode_ptr); drm_mode_probed_add(connector, mode_ptr); count++; } @@ -1462,56 +1471,97 @@ intel_tv_destroy(struct drm_connector *connector) kfree(connector); } - static int -intel_tv_set_property(struct drm_connector *connector, struct drm_property *property, - uint64_t val) +intel_tv_atomic_set_property(struct drm_connector *connector, + struct drm_connector_state *conn_state, + struct drm_property *property, + uint64_t val) { struct drm_device *dev = connector->dev; - struct intel_tv *intel_tv = intel_attached_tv(connector); - struct drm_crtc *crtc = intel_tv->base.base.crtc; - int ret = 0; + + struct intel_tv_connector_state *tv_state = + to_intel_tv_connector_state(conn_state); bool changed = false; - ret = drm_object_property_set_value(&connector->base, property, val); - if (ret < 0) - goto out; - - if (property == dev->mode_config.tv_left_margin_property && - intel_tv->margin[TV_MARGIN_LEFT] != val) { - intel_tv->margin[TV_MARGIN_LEFT] = val; - changed = true; - } else if (property == dev->mode_config.tv_right_margin_property && - intel_tv->margin[TV_MARGIN_RIGHT] != val) { - intel_tv->margin[TV_MARGIN_RIGHT] = val; - changed = true; - } else if (property == dev->mode_config.tv_top_margin_property && - intel_tv->margin[TV_MARGIN_TOP] != val) { - intel_tv->margin[TV_MARGIN_TOP] = val; - changed = true; - } else if (property == dev->mode_config.tv_bottom_margin_property && - intel_tv->margin[TV_MARGIN_BOTTOM] != val) { - intel_tv->margin[TV_MARGIN_BOTTOM] = val; - changed = true; + if (property == dev->mode_config.tv_left_margin_property) { + changed = tv_state->margin[TV_MARGIN_LEFT] != val; + + tv_state->margin[TV_MARGIN_LEFT] = val; + } else if (property == dev->mode_config.tv_right_margin_property) { + changed = tv_state->margin[TV_MARGIN_RIGHT] != val; + + tv_state->margin[TV_MARGIN_RIGHT] = val; + } else if (property == dev->mode_config.tv_top_margin_property) { + changed = tv_state->margin[TV_MARGIN_TOP] != val; + + tv_state->margin[TV_MARGIN_TOP] = val; + } else if (property == dev->mode_config.tv_bottom_margin_property) { + changed = tv_state->margin[TV_MARGIN_BOTTOM] != val; + + tv_state->margin[TV_MARGIN_BOTTOM] = val; } else if (property == dev->mode_config.tv_mode_property) { if (val >= ARRAY_SIZE(tv_modes)) { - ret = -EINVAL; - goto out; + DRM_DEBUG_ATOMIC("value %llu out of tv_modes array bounds\n", val); + + return -EINVAL; } - if (!strcmp(intel_tv->tv_format, tv_modes[val].name)) - goto out; - intel_tv->tv_format = tv_modes[val].name; - changed = true; + changed = tv_state->format != val; + tv_state->format = val; + } else { + DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name); + return -EINVAL; + } + + /* Trigger a modeset when connector properties are changed. */ + if (changed && conn_state->crtc) { + struct drm_crtc_state *crtc_state = + drm_atomic_get_existing_crtc_state(conn_state->state, + conn_state->crtc); + + crtc_state->connectors_changed = true; + } + return 0; +} + +static int +intel_tv_atomic_get_property(struct drm_connector *connector, + const struct drm_connector_state *state, + struct drm_property *property, + uint64_t *val) +{ + struct drm_device *dev = connector->dev; + struct intel_tv_connector_state *tv_state = + to_intel_tv_connector_state(state); + + if (property == dev->mode_config.tv_left_margin_property) + *val = tv_state->margin[TV_MARGIN_LEFT]; + else if (property == dev->mode_config.tv_right_margin_property) + *val = tv_state->margin[TV_MARGIN_RIGHT]; + else if (property == dev->mode_config.tv_top_margin_property) + *val = tv_state->margin[TV_MARGIN_TOP]; + else if (property == dev->mode_config.tv_bottom_margin_property) + *val = tv_state->margin[TV_MARGIN_BOTTOM]; + else if (property == dev->mode_config.tv_mode_property) { + *val = tv_state->format; } else { - ret = -EINVAL; - goto out; + DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name); + return -EINVAL; } - if (changed && crtc) - intel_crtc_restore_mode(crtc); -out: - return ret; + return 0; +} + +static struct drm_connector_state * +intel_tv_duplicate_state(struct drm_connector *connector) +{ + struct intel_tv_connector_state *state; + + state = kmemdup(&connector->state, sizeof(*state), GFP_KERNEL); + if (state) + __drm_atomic_helper_connector_duplicate_state(connector, &state->base); + + return &state->base; } static const struct drm_connector_funcs intel_tv_connector_funcs = { @@ -1520,11 +1570,12 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = { .late_register = intel_connector_register, .early_unregister = intel_connector_unregister, .destroy = intel_tv_destroy, - .set_property = intel_tv_set_property, - .atomic_get_property = intel_connector_atomic_get_property, + .set_property = drm_atomic_helper_connector_set_property, + .atomic_set_property = intel_tv_atomic_set_property, + .atomic_get_property = intel_tv_atomic_get_property, .fill_modes = drm_helper_probe_single_connector_modes, .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, + .atomic_duplicate_state = intel_tv_duplicate_state, }; static const struct drm_connector_helper_funcs intel_tv_connector_helper_funcs = { @@ -1547,6 +1598,7 @@ intel_tv_init(struct drm_i915_private *dev_priv) u32 tv_dac_on, tv_dac_off, save_tv_dac; const char *tv_format_names[ARRAY_SIZE(tv_modes)]; int i, initial_mode = 0; + struct intel_tv_connector_state *state; if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED) return; @@ -1580,16 +1632,17 @@ intel_tv_init(struct drm_i915_private *dev_priv) return; intel_tv = kzalloc(sizeof(*intel_tv), GFP_KERNEL); - if (!intel_tv) { - return; - } - - intel_connector = intel_connector_alloc(); - if (!intel_connector) { + intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL); + state = kzalloc(sizeof(*state), GFP_KERNEL); + if (!intel_tv || !intel_connector || !state) { + kfree(intel_connector); + kfree(state); kfree(intel_tv); return; } + __drm_atomic_helper_connector_reset(&intel_connector->base, &state->base); + intel_encoder = &intel_tv->base; connector = &intel_connector->base; @@ -1617,6 +1670,7 @@ intel_tv_init(struct drm_i915_private *dev_priv) intel_encoder->disable = intel_disable_tv; intel_encoder->get_hw_state = intel_tv_get_hw_state; intel_connector->get_hw_state = intel_connector_get_hw_state; + intel_connector->swap_state = intel_tv_swap_state; intel_connector_attach_encoder(intel_connector, intel_encoder); @@ -1629,12 +1683,13 @@ intel_tv_init(struct drm_i915_private *dev_priv) intel_tv->type = DRM_MODE_CONNECTOR_Unknown; /* BIOS margin values */ - intel_tv->margin[TV_MARGIN_LEFT] = 54; - intel_tv->margin[TV_MARGIN_TOP] = 36; - intel_tv->margin[TV_MARGIN_RIGHT] = 46; - intel_tv->margin[TV_MARGIN_BOTTOM] = 37; + state->margin[TV_MARGIN_LEFT] = 54; + state->margin[TV_MARGIN_TOP] = 36; + state->margin[TV_MARGIN_RIGHT] = 46; + state->margin[TV_MARGIN_BOTTOM] = 37; - intel_tv->tv_format = tv_modes[initial_mode].name; + state->format = initial_mode; + intel_tv->format = initial_mode; drm_connector_helper_add(connector, &intel_tv_connector_helper_funcs); connector->interlace_allowed = false; @@ -1648,17 +1703,17 @@ intel_tv_init(struct drm_i915_private *dev_priv) tv_format_names); drm_object_attach_property(&connector->base, dev->mode_config.tv_mode_property, - initial_mode); + state->format); drm_object_attach_property(&connector->base, dev->mode_config.tv_left_margin_property, - intel_tv->margin[TV_MARGIN_LEFT]); + state->margin[TV_MARGIN_LEFT]); drm_object_attach_property(&connector->base, dev->mode_config.tv_top_margin_property, - intel_tv->margin[TV_MARGIN_TOP]); + state->margin[TV_MARGIN_TOP]); drm_object_attach_property(&connector->base, dev->mode_config.tv_right_margin_property, - intel_tv->margin[TV_MARGIN_RIGHT]); + state->margin[TV_MARGIN_RIGHT]); drm_object_attach_property(&connector->base, dev->mode_config.tv_bottom_margin_property, - intel_tv->margin[TV_MARGIN_BOTTOM]); + state->margin[TV_MARGIN_BOTTOM]); } -- 2.7.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2. 2017-03-13 16:10 ` [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2 Maarten Lankhorst @ 2017-03-13 16:22 ` Ville Syrjälä 2017-03-13 16:35 ` Maarten Lankhorst 2017-03-13 16:44 ` Maarten Lankhorst 2017-03-14 18:36 ` Ville Syrjälä 1 sibling, 2 replies; 25+ messages in thread From: Ville Syrjälä @ 2017-03-13 16:22 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx 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@linux.intel.com> > --- > 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() ? > 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. With these two changes we shouldn't have to access ->state in detect() AFAICS. > + intel_attached_tv(connector)->format = i; > } > > /** > @@ -1344,18 +1358,17 @@ intel_tv_detect(struct drm_connector *connector, bool force) > } else > status = connector_status_unknown; > > + if (status == connector_status_connected) { > + intel_tv->type = type; > + intel_tv_find_better_format(connector); > + } > + > drm_modeset_drop_locks(&ctx); > drm_modeset_acquire_fini(&ctx); > - } else > - return connector->status; > > - if (status != connector_status_connected) > return status; > - > - intel_tv->type = type; > - intel_tv_find_better_format(connector); > - > - return connector_status_connected; > + } else > + return connector->status; > } > > static const struct input_res { > @@ -1375,12 +1388,9 @@ static const struct input_res { > * Chose preferred mode according to line number of TV format > */ > static void > -intel_tv_chose_preferred_modes(struct drm_connector *connector, > +intel_tv_chose_preferred_modes(const struct tv_mode *tv_mode, > struct drm_display_mode *mode_ptr) > { > - struct intel_tv *intel_tv = intel_attached_tv(connector); > - const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv); > - > if (tv_mode->nbr_end < 480 && mode_ptr->vdisplay == 480) > mode_ptr->type |= DRM_MODE_TYPE_PREFERRED; > else if (tv_mode->nbr_end > 480) { > @@ -1403,8 +1413,7 @@ static int > intel_tv_get_modes(struct drm_connector *connector) > { > struct drm_display_mode *mode_ptr; > - 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 j, count = 0; > u64 tmp; > > @@ -1447,7 +1456,7 @@ intel_tv_get_modes(struct drm_connector *connector) > mode_ptr->clock = (int) tmp; > > mode_ptr->type = DRM_MODE_TYPE_DRIVER; > - intel_tv_chose_preferred_modes(connector, mode_ptr); > + intel_tv_chose_preferred_modes(tv_mode, mode_ptr); ^^^^^ Pre-existing typo :) Might want to fix that up too. > drm_mode_probed_add(connector, mode_ptr); > count++; > } > @@ -1462,56 +1471,97 @@ intel_tv_destroy(struct drm_connector *connector) > kfree(connector); > } > > - > static int > -intel_tv_set_property(struct drm_connector *connector, struct drm_property *property, > - uint64_t val) > +intel_tv_atomic_set_property(struct drm_connector *connector, > + struct drm_connector_state *conn_state, > + struct drm_property *property, > + uint64_t val) > { > struct drm_device *dev = connector->dev; > - struct intel_tv *intel_tv = intel_attached_tv(connector); > - struct drm_crtc *crtc = intel_tv->base.base.crtc; > - int ret = 0; > + > + struct intel_tv_connector_state *tv_state = > + to_intel_tv_connector_state(conn_state); > bool changed = false; > > - ret = drm_object_property_set_value(&connector->base, property, val); > - if (ret < 0) > - goto out; > - > - if (property == dev->mode_config.tv_left_margin_property && > - intel_tv->margin[TV_MARGIN_LEFT] != val) { > - intel_tv->margin[TV_MARGIN_LEFT] = val; > - changed = true; > - } else if (property == dev->mode_config.tv_right_margin_property && > - intel_tv->margin[TV_MARGIN_RIGHT] != val) { > - intel_tv->margin[TV_MARGIN_RIGHT] = val; > - changed = true; > - } else if (property == dev->mode_config.tv_top_margin_property && > - intel_tv->margin[TV_MARGIN_TOP] != val) { > - intel_tv->margin[TV_MARGIN_TOP] = val; > - changed = true; > - } else if (property == dev->mode_config.tv_bottom_margin_property && > - intel_tv->margin[TV_MARGIN_BOTTOM] != val) { > - intel_tv->margin[TV_MARGIN_BOTTOM] = val; > - changed = true; > + if (property == dev->mode_config.tv_left_margin_property) { > + changed = tv_state->margin[TV_MARGIN_LEFT] != val; > + > + tv_state->margin[TV_MARGIN_LEFT] = val; > + } else if (property == dev->mode_config.tv_right_margin_property) { > + changed = tv_state->margin[TV_MARGIN_RIGHT] != val; > + > + tv_state->margin[TV_MARGIN_RIGHT] = val; > + } else if (property == dev->mode_config.tv_top_margin_property) { > + changed = tv_state->margin[TV_MARGIN_TOP] != val; > + > + tv_state->margin[TV_MARGIN_TOP] = val; > + } else if (property == dev->mode_config.tv_bottom_margin_property) { > + changed = tv_state->margin[TV_MARGIN_BOTTOM] != val; > + > + tv_state->margin[TV_MARGIN_BOTTOM] = val; > } else if (property == dev->mode_config.tv_mode_property) { > if (val >= ARRAY_SIZE(tv_modes)) { > - ret = -EINVAL; > - goto out; > + DRM_DEBUG_ATOMIC("value %llu out of tv_modes array bounds\n", val); > + > + return -EINVAL; > } > - if (!strcmp(intel_tv->tv_format, tv_modes[val].name)) > - goto out; > > - intel_tv->tv_format = tv_modes[val].name; > - changed = true; > + changed = tv_state->format != val; > + tv_state->format = val; > + } else { > + DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name); > + return -EINVAL; > + } > + > + /* Trigger a modeset when connector properties are changed. */ > + if (changed && conn_state->crtc) { > + struct drm_crtc_state *crtc_state = > + drm_atomic_get_existing_crtc_state(conn_state->state, > + conn_state->crtc); > + > + crtc_state->connectors_changed = true; > + } > + return 0; > +} > + > +static int > +intel_tv_atomic_get_property(struct drm_connector *connector, > + const struct drm_connector_state *state, > + struct drm_property *property, > + uint64_t *val) > +{ > + struct drm_device *dev = connector->dev; > + struct intel_tv_connector_state *tv_state = > + to_intel_tv_connector_state(state); > + > + if (property == dev->mode_config.tv_left_margin_property) > + *val = tv_state->margin[TV_MARGIN_LEFT]; > + else if (property == dev->mode_config.tv_right_margin_property) > + *val = tv_state->margin[TV_MARGIN_RIGHT]; > + else if (property == dev->mode_config.tv_top_margin_property) > + *val = tv_state->margin[TV_MARGIN_TOP]; > + else if (property == dev->mode_config.tv_bottom_margin_property) > + *val = tv_state->margin[TV_MARGIN_BOTTOM]; > + else if (property == dev->mode_config.tv_mode_property) { > + *val = tv_state->format; > } else { > - ret = -EINVAL; > - goto out; > + DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name); > + return -EINVAL; > } > > - if (changed && crtc) > - intel_crtc_restore_mode(crtc); > -out: > - return ret; > + return 0; > +} > + > +static struct drm_connector_state * > +intel_tv_duplicate_state(struct drm_connector *connector) > +{ > + struct intel_tv_connector_state *state; > + > + state = kmemdup(&connector->state, sizeof(*state), GFP_KERNEL); > + if (state) > + __drm_atomic_helper_connector_duplicate_state(connector, &state->base); > + > + return &state->base; > } > > static const struct drm_connector_funcs intel_tv_connector_funcs = { > @@ -1520,11 +1570,12 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = { > .late_register = intel_connector_register, > .early_unregister = intel_connector_unregister, > .destroy = intel_tv_destroy, > - .set_property = intel_tv_set_property, > - .atomic_get_property = intel_connector_atomic_get_property, > + .set_property = drm_atomic_helper_connector_set_property, > + .atomic_set_property = intel_tv_atomic_set_property, > + .atomic_get_property = intel_tv_atomic_get_property, > .fill_modes = drm_helper_probe_single_connector_modes, > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > + .atomic_duplicate_state = intel_tv_duplicate_state, > }; > > static const struct drm_connector_helper_funcs intel_tv_connector_helper_funcs = { > @@ -1547,6 +1598,7 @@ intel_tv_init(struct drm_i915_private *dev_priv) > u32 tv_dac_on, tv_dac_off, save_tv_dac; > const char *tv_format_names[ARRAY_SIZE(tv_modes)]; > int i, initial_mode = 0; > + struct intel_tv_connector_state *state; > > if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED) > return; > @@ -1580,16 +1632,17 @@ intel_tv_init(struct drm_i915_private *dev_priv) > return; > > intel_tv = kzalloc(sizeof(*intel_tv), GFP_KERNEL); > - if (!intel_tv) { > - return; > - } > - > - intel_connector = intel_connector_alloc(); > - if (!intel_connector) { > + intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL); > + state = kzalloc(sizeof(*state), GFP_KERNEL); > + if (!intel_tv || !intel_connector || !state) { > + kfree(intel_connector); > + kfree(state); > kfree(intel_tv); > return; > } > > + __drm_atomic_helper_connector_reset(&intel_connector->base, &state->base); > + > intel_encoder = &intel_tv->base; > connector = &intel_connector->base; > > @@ -1617,6 +1670,7 @@ intel_tv_init(struct drm_i915_private *dev_priv) > intel_encoder->disable = intel_disable_tv; > intel_encoder->get_hw_state = intel_tv_get_hw_state; > intel_connector->get_hw_state = intel_connector_get_hw_state; > + intel_connector->swap_state = intel_tv_swap_state; > > intel_connector_attach_encoder(intel_connector, intel_encoder); > > @@ -1629,12 +1683,13 @@ intel_tv_init(struct drm_i915_private *dev_priv) > intel_tv->type = DRM_MODE_CONNECTOR_Unknown; > > /* BIOS margin values */ > - intel_tv->margin[TV_MARGIN_LEFT] = 54; > - intel_tv->margin[TV_MARGIN_TOP] = 36; > - intel_tv->margin[TV_MARGIN_RIGHT] = 46; > - intel_tv->margin[TV_MARGIN_BOTTOM] = 37; > + state->margin[TV_MARGIN_LEFT] = 54; > + state->margin[TV_MARGIN_TOP] = 36; > + state->margin[TV_MARGIN_RIGHT] = 46; > + state->margin[TV_MARGIN_BOTTOM] = 37; > > - intel_tv->tv_format = tv_modes[initial_mode].name; > + state->format = initial_mode; > + intel_tv->format = initial_mode; > > drm_connector_helper_add(connector, &intel_tv_connector_helper_funcs); > connector->interlace_allowed = false; > @@ -1648,17 +1703,17 @@ intel_tv_init(struct drm_i915_private *dev_priv) > tv_format_names); > > drm_object_attach_property(&connector->base, dev->mode_config.tv_mode_property, > - initial_mode); > + state->format); > drm_object_attach_property(&connector->base, > dev->mode_config.tv_left_margin_property, > - intel_tv->margin[TV_MARGIN_LEFT]); > + state->margin[TV_MARGIN_LEFT]); > drm_object_attach_property(&connector->base, > dev->mode_config.tv_top_margin_property, > - intel_tv->margin[TV_MARGIN_TOP]); > + state->margin[TV_MARGIN_TOP]); > drm_object_attach_property(&connector->base, > dev->mode_config.tv_right_margin_property, > - intel_tv->margin[TV_MARGIN_RIGHT]); > + state->margin[TV_MARGIN_RIGHT]); > drm_object_attach_property(&connector->base, > dev->mode_config.tv_bottom_margin_property, > - intel_tv->margin[TV_MARGIN_BOTTOM]); > + state->margin[TV_MARGIN_BOTTOM]); > } > -- > 2.7.4 > -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2. 2017-03-13 16:22 ` Ville Syrjälä @ 2017-03-13 16:35 ` Maarten Lankhorst 2017-03-13 16:44 ` Ville Syrjälä 2017-03-13 16:44 ` Maarten Lankhorst 1 sibling, 1 reply; 25+ messages in thread From: Maarten Lankhorst @ 2017-03-13 16:35 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx 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@linux.intel.com> >> --- >> 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. > >> 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. > With these two changes we shouldn't have to access ->state in detect() > AFAICS. I think it's the only thing that makes sense. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2. 2017-03-13 16:35 ` Maarten Lankhorst @ 2017-03-13 16:44 ` Ville Syrjälä 2017-03-13 17:14 ` Maarten Lankhorst 0 siblings, 1 reply; 25+ messages in thread From: Ville Syrjälä @ 2017-03-13 16:44 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx 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@linux.intel.com> > >> --- > >> 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2. 2017-03-13 16:44 ` Ville Syrjälä @ 2017-03-13 17:14 ` Maarten Lankhorst 0 siblings, 0 replies; 25+ messages in thread From: Maarten Lankhorst @ 2017-03-13 17:14 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx Op 13-03-17 om 17:44 schreef Ville Syrjälä: > 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@linux.intel.com> >>>> --- >>>> 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. It's definitely a difference with the old code. So prone to regression if we introduce a behavioral change, best keep it the way it is. > 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. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2. 2017-03-13 16:22 ` Ville Syrjälä 2017-03-13 16:35 ` Maarten Lankhorst @ 2017-03-13 16:44 ` Maarten Lankhorst 1 sibling, 0 replies; 25+ messages in thread From: Maarten Lankhorst @ 2017-03-13 16:44 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx 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@linux.intel.com> >> --- >> 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 is fine, but yeah latter is probably more correct. > >> 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. > > With these two changes we shouldn't have to access ->state in detect() > AFAICS. Hm I think I might need to keep it here. That would return old values when userspace calls get_property, we don't want that. :) > >> + intel_attached_tv(connector)->format = i; >> } >> >> /** >> @@ -1344,18 +1358,17 @@ intel_tv_detect(struct drm_connector *connector, bool force) >> } else >> status = connector_status_unknown; >> >> + if (status == connector_status_connected) { >> + intel_tv->type = type; >> + intel_tv_find_better_format(connector); >> + } >> + >> drm_modeset_drop_locks(&ctx); >> drm_modeset_acquire_fini(&ctx); >> - } else >> - return connector->status; >> >> - if (status != connector_status_connected) >> return status; >> - >> - intel_tv->type = type; >> - intel_tv_find_better_format(connector); >> - >> - return connector_status_connected; >> + } else >> + return connector->status; >> } >> >> static const struct input_res { >> @@ -1375,12 +1388,9 @@ static const struct input_res { >> * Chose preferred mode according to line number of TV format >> */ >> static void >> -intel_tv_chose_preferred_modes(struct drm_connector *connector, >> +intel_tv_chose_preferred_modes(const struct tv_mode *tv_mode, >> struct drm_display_mode *mode_ptr) >> { >> - struct intel_tv *intel_tv = intel_attached_tv(connector); >> - const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv); >> - >> if (tv_mode->nbr_end < 480 && mode_ptr->vdisplay == 480) >> mode_ptr->type |= DRM_MODE_TYPE_PREFERRED; >> else if (tv_mode->nbr_end > 480) { >> @@ -1403,8 +1413,7 @@ static int >> intel_tv_get_modes(struct drm_connector *connector) >> { >> struct drm_display_mode *mode_ptr; >> - 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 j, count = 0; >> u64 tmp; >> >> @@ -1447,7 +1456,7 @@ intel_tv_get_modes(struct drm_connector *connector) >> mode_ptr->clock = (int) tmp; >> >> mode_ptr->type = DRM_MODE_TYPE_DRIVER; >> - intel_tv_chose_preferred_modes(connector, mode_ptr); >> + intel_tv_chose_preferred_modes(tv_mode, mode_ptr); > ^^^^^ > > Pre-existing typo :) Might want to fix that up too. Indeed. > > >> drm_mode_probed_add(connector, mode_ptr); >> count++; >> } >> @@ -1462,56 +1471,97 @@ intel_tv_destroy(struct drm_connector *connector) >> kfree(connector); >> } >> >> - >> static int >> -intel_tv_set_property(struct drm_connector *connector, struct drm_property *property, >> - uint64_t val) >> +intel_tv_atomic_set_property(struct drm_connector *connector, >> + struct drm_connector_state *conn_state, >> + struct drm_property *property, >> + uint64_t val) >> { >> struct drm_device *dev = connector->dev; >> - struct intel_tv *intel_tv = intel_attached_tv(connector); >> - struct drm_crtc *crtc = intel_tv->base.base.crtc; >> - int ret = 0; >> + >> + struct intel_tv_connector_state *tv_state = >> + to_intel_tv_connector_state(conn_state); >> bool changed = false; >> >> - ret = drm_object_property_set_value(&connector->base, property, val); >> - if (ret < 0) >> - goto out; >> - >> - if (property == dev->mode_config.tv_left_margin_property && >> - intel_tv->margin[TV_MARGIN_LEFT] != val) { >> - intel_tv->margin[TV_MARGIN_LEFT] = val; >> - changed = true; >> - } else if (property == dev->mode_config.tv_right_margin_property && >> - intel_tv->margin[TV_MARGIN_RIGHT] != val) { >> - intel_tv->margin[TV_MARGIN_RIGHT] = val; >> - changed = true; >> - } else if (property == dev->mode_config.tv_top_margin_property && >> - intel_tv->margin[TV_MARGIN_TOP] != val) { >> - intel_tv->margin[TV_MARGIN_TOP] = val; >> - changed = true; >> - } else if (property == dev->mode_config.tv_bottom_margin_property && >> - intel_tv->margin[TV_MARGIN_BOTTOM] != val) { >> - intel_tv->margin[TV_MARGIN_BOTTOM] = val; >> - changed = true; >> + if (property == dev->mode_config.tv_left_margin_property) { >> + changed = tv_state->margin[TV_MARGIN_LEFT] != val; >> + >> + tv_state->margin[TV_MARGIN_LEFT] = val; >> + } else if (property == dev->mode_config.tv_right_margin_property) { >> + changed = tv_state->margin[TV_MARGIN_RIGHT] != val; >> + >> + tv_state->margin[TV_MARGIN_RIGHT] = val; >> + } else if (property == dev->mode_config.tv_top_margin_property) { >> + changed = tv_state->margin[TV_MARGIN_TOP] != val; >> + >> + tv_state->margin[TV_MARGIN_TOP] = val; >> + } else if (property == dev->mode_config.tv_bottom_margin_property) { >> + changed = tv_state->margin[TV_MARGIN_BOTTOM] != val; >> + >> + tv_state->margin[TV_MARGIN_BOTTOM] = val; >> } else if (property == dev->mode_config.tv_mode_property) { >> if (val >= ARRAY_SIZE(tv_modes)) { >> - ret = -EINVAL; >> - goto out; >> + DRM_DEBUG_ATOMIC("value %llu out of tv_modes array bounds\n", val); >> + >> + return -EINVAL; >> } >> - if (!strcmp(intel_tv->tv_format, tv_modes[val].name)) >> - goto out; >> >> - intel_tv->tv_format = tv_modes[val].name; >> - changed = true; >> + changed = tv_state->format != val; >> + tv_state->format = val; >> + } else { >> + DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name); >> + return -EINVAL; >> + } >> + >> + /* Trigger a modeset when connector properties are changed. */ >> + if (changed && conn_state->crtc) { >> + struct drm_crtc_state *crtc_state = >> + drm_atomic_get_existing_crtc_state(conn_state->state, >> + conn_state->crtc); >> + >> + crtc_state->connectors_changed = true; >> + } >> + return 0; >> +} >> + >> +static int >> +intel_tv_atomic_get_property(struct drm_connector *connector, >> + const struct drm_connector_state *state, >> + struct drm_property *property, >> + uint64_t *val) >> +{ >> + struct drm_device *dev = connector->dev; >> + struct intel_tv_connector_state *tv_state = >> + to_intel_tv_connector_state(state); >> + >> + if (property == dev->mode_config.tv_left_margin_property) >> + *val = tv_state->margin[TV_MARGIN_LEFT]; >> + else if (property == dev->mode_config.tv_right_margin_property) >> + *val = tv_state->margin[TV_MARGIN_RIGHT]; >> + else if (property == dev->mode_config.tv_top_margin_property) >> + *val = tv_state->margin[TV_MARGIN_TOP]; >> + else if (property == dev->mode_config.tv_bottom_margin_property) >> + *val = tv_state->margin[TV_MARGIN_BOTTOM]; >> + else if (property == dev->mode_config.tv_mode_property) { >> + *val = tv_state->format; >> } else { >> - ret = -EINVAL; >> - goto out; >> + DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name); >> + return -EINVAL; >> } >> >> - if (changed && crtc) >> - intel_crtc_restore_mode(crtc); >> -out: >> - return ret; >> + return 0; >> +} >> + >> +static struct drm_connector_state * >> +intel_tv_duplicate_state(struct drm_connector *connector) >> +{ >> + struct intel_tv_connector_state *state; >> + >> + state = kmemdup(&connector->state, sizeof(*state), GFP_KERNEL); >> + if (state) >> + __drm_atomic_helper_connector_duplicate_state(connector, &state->base); >> + >> + return &state->base; >> } >> >> static const struct drm_connector_funcs intel_tv_connector_funcs = { >> @@ -1520,11 +1570,12 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = { >> .late_register = intel_connector_register, >> .early_unregister = intel_connector_unregister, >> .destroy = intel_tv_destroy, >> - .set_property = intel_tv_set_property, >> - .atomic_get_property = intel_connector_atomic_get_property, >> + .set_property = drm_atomic_helper_connector_set_property, >> + .atomic_set_property = intel_tv_atomic_set_property, >> + .atomic_get_property = intel_tv_atomic_get_property, >> .fill_modes = drm_helper_probe_single_connector_modes, >> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >> - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, >> + .atomic_duplicate_state = intel_tv_duplicate_state, >> }; >> >> static const struct drm_connector_helper_funcs intel_tv_connector_helper_funcs = { >> @@ -1547,6 +1598,7 @@ intel_tv_init(struct drm_i915_private *dev_priv) >> u32 tv_dac_on, tv_dac_off, save_tv_dac; >> const char *tv_format_names[ARRAY_SIZE(tv_modes)]; >> int i, initial_mode = 0; >> + struct intel_tv_connector_state *state; >> >> if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED) >> return; >> @@ -1580,16 +1632,17 @@ intel_tv_init(struct drm_i915_private *dev_priv) >> return; >> >> intel_tv = kzalloc(sizeof(*intel_tv), GFP_KERNEL); >> - if (!intel_tv) { >> - return; >> - } >> - >> - intel_connector = intel_connector_alloc(); >> - if (!intel_connector) { >> + intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL); >> + state = kzalloc(sizeof(*state), GFP_KERNEL); >> + if (!intel_tv || !intel_connector || !state) { >> + kfree(intel_connector); >> + kfree(state); >> kfree(intel_tv); >> return; >> } >> >> + __drm_atomic_helper_connector_reset(&intel_connector->base, &state->base); >> + >> intel_encoder = &intel_tv->base; >> connector = &intel_connector->base; >> >> @@ -1617,6 +1670,7 @@ intel_tv_init(struct drm_i915_private *dev_priv) >> intel_encoder->disable = intel_disable_tv; >> intel_encoder->get_hw_state = intel_tv_get_hw_state; >> intel_connector->get_hw_state = intel_connector_get_hw_state; >> + intel_connector->swap_state = intel_tv_swap_state; >> >> intel_connector_attach_encoder(intel_connector, intel_encoder); >> >> @@ -1629,12 +1683,13 @@ intel_tv_init(struct drm_i915_private *dev_priv) >> intel_tv->type = DRM_MODE_CONNECTOR_Unknown; >> >> /* BIOS margin values */ >> - intel_tv->margin[TV_MARGIN_LEFT] = 54; >> - intel_tv->margin[TV_MARGIN_TOP] = 36; >> - intel_tv->margin[TV_MARGIN_RIGHT] = 46; >> - intel_tv->margin[TV_MARGIN_BOTTOM] = 37; >> + state->margin[TV_MARGIN_LEFT] = 54; >> + state->margin[TV_MARGIN_TOP] = 36; >> + state->margin[TV_MARGIN_RIGHT] = 46; >> + state->margin[TV_MARGIN_BOTTOM] = 37; >> >> - intel_tv->tv_format = tv_modes[initial_mode].name; >> + state->format = initial_mode; >> + intel_tv->format = initial_mode; >> >> drm_connector_helper_add(connector, &intel_tv_connector_helper_funcs); >> connector->interlace_allowed = false; >> @@ -1648,17 +1703,17 @@ intel_tv_init(struct drm_i915_private *dev_priv) >> tv_format_names); >> >> drm_object_attach_property(&connector->base, dev->mode_config.tv_mode_property, >> - initial_mode); >> + state->format); >> drm_object_attach_property(&connector->base, >> dev->mode_config.tv_left_margin_property, >> - intel_tv->margin[TV_MARGIN_LEFT]); >> + state->margin[TV_MARGIN_LEFT]); >> drm_object_attach_property(&connector->base, >> dev->mode_config.tv_top_margin_property, >> - intel_tv->margin[TV_MARGIN_TOP]); >> + state->margin[TV_MARGIN_TOP]); >> drm_object_attach_property(&connector->base, >> dev->mode_config.tv_right_margin_property, >> - intel_tv->margin[TV_MARGIN_RIGHT]); >> + state->margin[TV_MARGIN_RIGHT]); >> drm_object_attach_property(&connector->base, >> dev->mode_config.tv_bottom_margin_property, >> - intel_tv->margin[TV_MARGIN_BOTTOM]); >> + state->margin[TV_MARGIN_BOTTOM]); >> } >> -- >> 2.7.4 >> _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2. 2017-03-13 16:10 ` [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2 Maarten Lankhorst 2017-03-13 16:22 ` Ville Syrjälä @ 2017-03-14 18:36 ` Ville Syrjälä 2017-03-15 8:32 ` Maarten Lankhorst 2017-03-15 10:08 ` [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v3 Maarten Lankhorst 1 sibling, 2 replies; 25+ messages in thread From: Ville Syrjälä @ 2017-03-14 18:36 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx 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@linux.intel.com> > --- > 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 <snip> > + > +static int > +intel_tv_atomic_get_property(struct drm_connector *connector, > + const struct drm_connector_state *state, > + struct drm_property *property, > + uint64_t *val) > +{ > + struct drm_device *dev = connector->dev; > + struct intel_tv_connector_state *tv_state = > + to_intel_tv_connector_state(state); > + > + if (property == dev->mode_config.tv_left_margin_property) > + *val = tv_state->margin[TV_MARGIN_LEFT]; > + else if (property == dev->mode_config.tv_right_margin_property) > + *val = tv_state->margin[TV_MARGIN_RIGHT]; > + else if (property == dev->mode_config.tv_top_margin_property) > + *val = tv_state->margin[TV_MARGIN_TOP]; > + else if (property == dev->mode_config.tv_bottom_margin_property) > + *val = tv_state->margin[TV_MARGIN_BOTTOM]; > + else if (property == dev->mode_config.tv_mode_property) { > + *val = tv_state->format; Actually, aren't all of these handled by drm_atomic_connector_set_property() already? And same deal with drm_atomic_connector_get_property() AFAICS. > } else { > - ret = -EINVAL; > - goto out; > + DRM_DEBUG_ATOMIC("Unknown atomic property %s\n", property->name); > + return -EINVAL; > } > > - if (changed && crtc) > - intel_crtc_restore_mode(crtc); > -out: > - return ret; > + return 0; > +} > + > +static struct drm_connector_state * > +intel_tv_duplicate_state(struct drm_connector *connector) > +{ > + struct intel_tv_connector_state *state; > + > + state = kmemdup(&connector->state, sizeof(*state), GFP_KERNEL); > + if (state) > + __drm_atomic_helper_connector_duplicate_state(connector, &state->base); > + > + return &state->base; > } > > static const struct drm_connector_funcs intel_tv_connector_funcs = { > @@ -1520,11 +1570,12 @@ static const struct drm_connector_funcs intel_tv_connector_funcs = { > .late_register = intel_connector_register, > .early_unregister = intel_connector_unregister, > .destroy = intel_tv_destroy, > - .set_property = intel_tv_set_property, > - .atomic_get_property = intel_connector_atomic_get_property, > + .set_property = drm_atomic_helper_connector_set_property, > + .atomic_set_property = intel_tv_atomic_set_property, > + .atomic_get_property = intel_tv_atomic_get_property, > .fill_modes = drm_helper_probe_single_connector_modes, > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > + .atomic_duplicate_state = intel_tv_duplicate_state, > }; > > static const struct drm_connector_helper_funcs intel_tv_connector_helper_funcs = { > @@ -1547,6 +1598,7 @@ intel_tv_init(struct drm_i915_private *dev_priv) > u32 tv_dac_on, tv_dac_off, save_tv_dac; > const char *tv_format_names[ARRAY_SIZE(tv_modes)]; > int i, initial_mode = 0; > + struct intel_tv_connector_state *state; > > if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED) > return; > @@ -1580,16 +1632,17 @@ intel_tv_init(struct drm_i915_private *dev_priv) > return; > > intel_tv = kzalloc(sizeof(*intel_tv), GFP_KERNEL); > - if (!intel_tv) { > - return; > - } > - > - intel_connector = intel_connector_alloc(); > - if (!intel_connector) { > + intel_connector = kzalloc(sizeof(*intel_connector), GFP_KERNEL); > + state = kzalloc(sizeof(*state), GFP_KERNEL); > + if (!intel_tv || !intel_connector || !state) { > + kfree(intel_connector); > + kfree(state); > kfree(intel_tv); > return; > } > > + __drm_atomic_helper_connector_reset(&intel_connector->base, &state->base); > + > intel_encoder = &intel_tv->base; > connector = &intel_connector->base; > > @@ -1617,6 +1670,7 @@ intel_tv_init(struct drm_i915_private *dev_priv) > intel_encoder->disable = intel_disable_tv; > intel_encoder->get_hw_state = intel_tv_get_hw_state; > intel_connector->get_hw_state = intel_connector_get_hw_state; > + intel_connector->swap_state = intel_tv_swap_state; > > intel_connector_attach_encoder(intel_connector, intel_encoder); > > @@ -1629,12 +1683,13 @@ intel_tv_init(struct drm_i915_private *dev_priv) > intel_tv->type = DRM_MODE_CONNECTOR_Unknown; > > /* BIOS margin values */ > - intel_tv->margin[TV_MARGIN_LEFT] = 54; > - intel_tv->margin[TV_MARGIN_TOP] = 36; > - intel_tv->margin[TV_MARGIN_RIGHT] = 46; > - intel_tv->margin[TV_MARGIN_BOTTOM] = 37; > + state->margin[TV_MARGIN_LEFT] = 54; > + state->margin[TV_MARGIN_TOP] = 36; > + state->margin[TV_MARGIN_RIGHT] = 46; > + state->margin[TV_MARGIN_BOTTOM] = 37; > > - intel_tv->tv_format = tv_modes[initial_mode].name; > + state->format = initial_mode; > + intel_tv->format = initial_mode; > > drm_connector_helper_add(connector, &intel_tv_connector_helper_funcs); > connector->interlace_allowed = false; > @@ -1648,17 +1703,17 @@ intel_tv_init(struct drm_i915_private *dev_priv) > tv_format_names); > > drm_object_attach_property(&connector->base, dev->mode_config.tv_mode_property, > - initial_mode); > + state->format); > drm_object_attach_property(&connector->base, > dev->mode_config.tv_left_margin_property, > - intel_tv->margin[TV_MARGIN_LEFT]); > + state->margin[TV_MARGIN_LEFT]); > drm_object_attach_property(&connector->base, > dev->mode_config.tv_top_margin_property, > - intel_tv->margin[TV_MARGIN_TOP]); > + state->margin[TV_MARGIN_TOP]); > drm_object_attach_property(&connector->base, > dev->mode_config.tv_right_margin_property, > - intel_tv->margin[TV_MARGIN_RIGHT]); > + state->margin[TV_MARGIN_RIGHT]); > drm_object_attach_property(&connector->base, > dev->mode_config.tv_bottom_margin_property, > - intel_tv->margin[TV_MARGIN_BOTTOM]); > + state->margin[TV_MARGIN_BOTTOM]); > } > -- > 2.7.4 > -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2. 2017-03-14 18:36 ` Ville Syrjälä @ 2017-03-15 8:32 ` Maarten Lankhorst 2017-03-15 10:08 ` [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v3 Maarten Lankhorst 1 sibling, 0 replies; 25+ messages in thread From: Maarten Lankhorst @ 2017-03-15 8:32 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx Op 14-03-17 om 19:36 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@linux.intel.com> >> --- >> 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 > <snip> >> + >> +static int >> +intel_tv_atomic_get_property(struct drm_connector *connector, >> + const struct drm_connector_state *state, >> + struct drm_property *property, >> + uint64_t *val) >> +{ >> + struct drm_device *dev = connector->dev; >> + struct intel_tv_connector_state *tv_state = >> + to_intel_tv_connector_state(state); >> + >> + if (property == dev->mode_config.tv_left_margin_property) >> + *val = tv_state->margin[TV_MARGIN_LEFT]; >> + else if (property == dev->mode_config.tv_right_margin_property) >> + *val = tv_state->margin[TV_MARGIN_RIGHT]; >> + else if (property == dev->mode_config.tv_top_margin_property) >> + *val = tv_state->margin[TV_MARGIN_TOP]; >> + else if (property == dev->mode_config.tv_bottom_margin_property) >> + *val = tv_state->margin[TV_MARGIN_BOTTOM]; >> + else if (property == dev->mode_config.tv_mode_property) { >> + *val = tv_state->format; > Actually, aren't all of these handled by drm_atomic_connector_set_property() > already? > > And same deal with drm_atomic_connector_get_property() AFAICS. Argh, indeed. Thanks for noticing.. That should reduce the amount of boilerplate. Is there hw for doing actual testing on the tv connector? If I actually tested this on real hw I would have noticed this. ~Maarten _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v3. 2017-03-14 18:36 ` Ville Syrjälä 2017-03-15 8:32 ` Maarten Lankhorst @ 2017-03-15 10:08 ` Maarten Lankhorst 2017-03-16 10:48 ` Daniel Vetter 1 sibling, 1 reply; 25+ messages in thread From: Maarten Lankhorst @ 2017-03-15 10:08 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx 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. Changes since v2: - Fix typo in tv_choose_preferred modes function name. - Assignment of tv properties is done in core, so intel_tv only needs a atomic_check function. Thanks Ville! Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 15 +++ drivers/gpu/drm/i915/intel_drv.h | 4 + drivers/gpu/drm/i915/intel_tv.c | 200 ++++++++++++++++------------------- 3 files changed, 110 insertions(+), 109 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..eeed17e6741a 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,6 +81,8 @@ struct intel_tv { u32 save_TV_DAC; u32 save_TV_CTL; + + int format; }; struct video_levels { @@ -848,6 +848,15 @@ 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); + + intel_tv->format = conn_state->tv.mode; +} + +static void intel_enable_tv(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config, struct drm_connector_state *conn_state) @@ -873,32 +882,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 = conn_state->tv.mode; - 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 +927,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 +1033,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; @@ -1135,12 +1136,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 += conn_state->tv.margins.left; + ypos += conn_state->tv.margins.top; + xsize -= (conn_state->tv.margins.left + + conn_state->tv.margins.right); + ysize -= (conn_state->tv.margins.top + + conn_state->tv.margins.bottom); I915_WRITE(TV_WIN_POS, (xpos<<16)|ypos); I915_WRITE(TV_WIN_SIZE, (xsize<<16)|ysize); @@ -1288,7 +1289,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); int i; if ((intel_tv->type == DRM_MODE_CONNECTOR_Component) == @@ -1304,9 +1305,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); + connector->state->tv.mode = i; + intel_attached_tv(connector)->format = i; } /** @@ -1344,18 +1344,17 @@ intel_tv_detect(struct drm_connector *connector, bool force) } else status = connector_status_unknown; + if (status == connector_status_connected) { + intel_tv->type = type; + intel_tv_find_better_format(connector); + } + drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx); - } else - return connector->status; - if (status != connector_status_connected) return status; - - intel_tv->type = type; - intel_tv_find_better_format(connector); - - return connector_status_connected; + } else + return connector->status; } static const struct input_res { @@ -1375,12 +1374,9 @@ static const struct input_res { * Chose preferred mode according to line number of TV format */ static void -intel_tv_chose_preferred_modes(struct drm_connector *connector, +intel_tv_choose_preferred_modes(const struct tv_mode *tv_mode, struct drm_display_mode *mode_ptr) { - struct intel_tv *intel_tv = intel_attached_tv(connector); - const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv); - if (tv_mode->nbr_end < 480 && mode_ptr->vdisplay == 480) mode_ptr->type |= DRM_MODE_TYPE_PREFERRED; else if (tv_mode->nbr_end > 480) { @@ -1403,8 +1399,7 @@ static int intel_tv_get_modes(struct drm_connector *connector) { struct drm_display_mode *mode_ptr; - 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 j, count = 0; u64 tmp; @@ -1447,7 +1442,7 @@ intel_tv_get_modes(struct drm_connector *connector) mode_ptr->clock = (int) tmp; mode_ptr->type = DRM_MODE_TYPE_DRIVER; - intel_tv_chose_preferred_modes(connector, mode_ptr); + intel_tv_choose_preferred_modes(tv_mode, mode_ptr); drm_mode_probed_add(connector, mode_ptr); count++; } @@ -1462,66 +1457,13 @@ intel_tv_destroy(struct drm_connector *connector) kfree(connector); } - -static int -intel_tv_set_property(struct drm_connector *connector, struct drm_property *property, - uint64_t val) -{ - struct drm_device *dev = connector->dev; - struct intel_tv *intel_tv = intel_attached_tv(connector); - struct drm_crtc *crtc = intel_tv->base.base.crtc; - int ret = 0; - bool changed = false; - - ret = drm_object_property_set_value(&connector->base, property, val); - if (ret < 0) - goto out; - - if (property == dev->mode_config.tv_left_margin_property && - intel_tv->margin[TV_MARGIN_LEFT] != val) { - intel_tv->margin[TV_MARGIN_LEFT] = val; - changed = true; - } else if (property == dev->mode_config.tv_right_margin_property && - intel_tv->margin[TV_MARGIN_RIGHT] != val) { - intel_tv->margin[TV_MARGIN_RIGHT] = val; - changed = true; - } else if (property == dev->mode_config.tv_top_margin_property && - intel_tv->margin[TV_MARGIN_TOP] != val) { - intel_tv->margin[TV_MARGIN_TOP] = val; - changed = true; - } else if (property == dev->mode_config.tv_bottom_margin_property && - intel_tv->margin[TV_MARGIN_BOTTOM] != val) { - intel_tv->margin[TV_MARGIN_BOTTOM] = val; - changed = true; - } else if (property == dev->mode_config.tv_mode_property) { - if (val >= ARRAY_SIZE(tv_modes)) { - ret = -EINVAL; - goto out; - } - if (!strcmp(intel_tv->tv_format, tv_modes[val].name)) - goto out; - - intel_tv->tv_format = tv_modes[val].name; - changed = true; - } else { - ret = -EINVAL; - goto out; - } - - if (changed && crtc) - intel_crtc_restore_mode(crtc); -out: - return ret; -} - static const struct drm_connector_funcs intel_tv_connector_funcs = { .dpms = drm_atomic_helper_connector_dpms, .detect = intel_tv_detect, .late_register = intel_connector_register, .early_unregister = intel_connector_unregister, .destroy = intel_tv_destroy, - .set_property = intel_tv_set_property, - .atomic_get_property = intel_connector_atomic_get_property, + .set_property = drm_atomic_helper_connector_set_property, .fill_modes = drm_helper_probe_single_connector_modes, .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, @@ -1532,6 +1474,41 @@ static const struct drm_connector_helper_funcs intel_tv_connector_helper_funcs = .get_modes = intel_tv_get_modes, }; +static int intel_tv_atomic_check(struct drm_encoder *encoder, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *new_state) +{ + struct drm_connector_state *old_state = new_state->connector->state; + + if (old_state->tv.mode != new_state->tv.mode || + old_state->tv.margins.left != new_state->tv.margins.left || + old_state->tv.margins.right != new_state->tv.margins.right || + old_state->tv.margins.top != new_state->tv.margins.top || + old_state->tv.margins.bottom != new_state->tv.margins.bottom) { + int ret; + + crtc_state->connectors_changed = true; + + /* + * This is called after drm_atomic_helper_check_modeset adds + * affected planes/connectors, so we have to add them manually. + */ + ret = drm_atomic_add_affected_connectors(new_state->state, crtc_state->crtc); + if (ret) + return ret; + + ret = drm_atomic_add_affected_planes(new_state->state, crtc_state->crtc); + if (ret) + return ret; + } + + return 0; +} + +static const struct drm_encoder_helper_funcs intel_tv_enc_helper_funcs = { + .atomic_check = intel_tv_atomic_check, +}; + static const struct drm_encoder_funcs intel_tv_enc_funcs = { .destroy = intel_encoder_destroy, }; @@ -1547,6 +1524,7 @@ intel_tv_init(struct drm_i915_private *dev_priv) u32 tv_dac_on, tv_dac_off, save_tv_dac; const char *tv_format_names[ARRAY_SIZE(tv_modes)]; int i, initial_mode = 0; + struct drm_connector_state *state; if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED) return; @@ -1592,6 +1570,7 @@ intel_tv_init(struct drm_i915_private *dev_priv) intel_encoder = &intel_tv->base; connector = &intel_connector->base; + state = connector->state; /* The documentation, for the older chipsets at least, recommend * using a polling method rather than hotplug detection for TVs. @@ -1617,6 +1596,7 @@ intel_tv_init(struct drm_i915_private *dev_priv) intel_encoder->disable = intel_disable_tv; intel_encoder->get_hw_state = intel_tv_get_hw_state; intel_connector->get_hw_state = intel_connector_get_hw_state; + intel_connector->swap_state = intel_tv_swap_state; intel_connector_attach_encoder(intel_connector, intel_encoder); @@ -1627,14 +1607,16 @@ intel_tv_init(struct drm_i915_private *dev_priv) intel_encoder->cloneable = 0; intel_encoder->base.possible_crtcs = ((1 << 0) | (1 << 1)); intel_tv->type = DRM_MODE_CONNECTOR_Unknown; + drm_encoder_helper_add(&intel_encoder->base, &intel_tv_enc_helper_funcs); /* BIOS margin values */ - intel_tv->margin[TV_MARGIN_LEFT] = 54; - intel_tv->margin[TV_MARGIN_TOP] = 36; - intel_tv->margin[TV_MARGIN_RIGHT] = 46; - intel_tv->margin[TV_MARGIN_BOTTOM] = 37; + state->tv.margins.left = 54; + state->tv.margins.top = 36; + state->tv.margins.right = 46; + state->tv.margins.bottom = 37; - intel_tv->tv_format = tv_modes[initial_mode].name; + state->tv.mode = initial_mode; + intel_tv->format = initial_mode; drm_connector_helper_add(connector, &intel_tv_connector_helper_funcs); connector->interlace_allowed = false; @@ -1648,17 +1630,17 @@ intel_tv_init(struct drm_i915_private *dev_priv) tv_format_names); drm_object_attach_property(&connector->base, dev->mode_config.tv_mode_property, - initial_mode); + state->tv.mode); drm_object_attach_property(&connector->base, dev->mode_config.tv_left_margin_property, - intel_tv->margin[TV_MARGIN_LEFT]); + state->tv.margins.left); drm_object_attach_property(&connector->base, dev->mode_config.tv_top_margin_property, - intel_tv->margin[TV_MARGIN_TOP]); + state->tv.margins.top); drm_object_attach_property(&connector->base, dev->mode_config.tv_right_margin_property, - intel_tv->margin[TV_MARGIN_RIGHT]); + state->tv.margins.right); drm_object_attach_property(&connector->base, dev->mode_config.tv_bottom_margin_property, - intel_tv->margin[TV_MARGIN_BOTTOM]); + state->tv.margins.bottom); } -- 2.7.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v3. 2017-03-15 10:08 ` [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v3 Maarten Lankhorst @ 2017-03-16 10:48 ` Daniel Vetter 2017-03-16 12:05 ` Maarten Lankhorst 0 siblings, 1 reply; 25+ messages in thread From: Daniel Vetter @ 2017-03-16 10:48 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx On Wed, Mar 15, 2017 at 11:08:56AM +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. > Changes since v2: > - Fix typo in tv_choose_preferred modes function name. > - Assignment of tv properties is done in core, so intel_tv only needs > a atomic_check function. Thanks Ville! > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 15 +++ > drivers/gpu/drm/i915/intel_drv.h | 4 + > drivers/gpu/drm/i915/intel_tv.c | 200 ++++++++++++++++------------------- > 3 files changed, 110 insertions(+), 109 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); > + } > +} So yeah I'm late, but this is pretty much exactly what DK's private state object stuff was meant for. Have you looked into rebasing on top of that? I think hand-rolling private state stuff everywhere isn't really good. Afaics DK's patch series is ready for merging into drm-misc, so shouldn't end up blocking you. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v3. 2017-03-16 10:48 ` Daniel Vetter @ 2017-03-16 12:05 ` Maarten Lankhorst 2017-03-16 14:56 ` Daniel Vetter 0 siblings, 1 reply; 25+ messages in thread From: Maarten Lankhorst @ 2017-03-16 12:05 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx Op 16-03-17 om 11:48 schreef Daniel Vetter: > On Wed, Mar 15, 2017 at 11:08:56AM +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. >> Changes since v2: >> - Fix typo in tv_choose_preferred modes function name. >> - Assignment of tv properties is done in core, so intel_tv only needs >> a atomic_check function. Thanks Ville! >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 15 +++ >> drivers/gpu/drm/i915/intel_drv.h | 4 + >> drivers/gpu/drm/i915/intel_tv.c | 200 ++++++++++++++++------------------- >> 3 files changed, 110 insertions(+), 109 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); >> + } >> +} > So yeah I'm late, but this is pretty much exactly what DK's private state > object stuff was meant for. Have you looked into rebasing on top of that? > I think hand-rolling private state stuff everywhere isn't really good. > > Afaics DK's patch series is ready for merging into drm-misc, so shouldn't > end up blocking you. This swap_state hook is used for connector state. We could probably add a swap_state hook that drivers can use for this in the core, but the connector state is already part of the state, adding it twice is silly. Instead, would adding a hook to the core be useful? ~Maarten _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v3. 2017-03-16 12:05 ` Maarten Lankhorst @ 2017-03-16 14:56 ` Daniel Vetter 2017-03-16 15:27 ` Maarten Lankhorst 0 siblings, 1 reply; 25+ messages in thread From: Daniel Vetter @ 2017-03-16 14:56 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx On Thu, Mar 16, 2017 at 01:05:06PM +0100, Maarten Lankhorst wrote: > Op 16-03-17 om 11:48 schreef Daniel Vetter: > > On Wed, Mar 15, 2017 at 11:08:56AM +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. > >> Changes since v2: > >> - Fix typo in tv_choose_preferred modes function name. > >> - Assignment of tv properties is done in core, so intel_tv only needs > >> a atomic_check function. Thanks Ville! > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_display.c | 15 +++ > >> drivers/gpu/drm/i915/intel_drv.h | 4 + > >> drivers/gpu/drm/i915/intel_tv.c | 200 ++++++++++++++++------------------- > >> 3 files changed, 110 insertions(+), 109 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); > >> + } > >> +} > > So yeah I'm late, but this is pretty much exactly what DK's private state > > object stuff was meant for. Have you looked into rebasing on top of that? > > I think hand-rolling private state stuff everywhere isn't really good. > > > > Afaics DK's patch series is ready for merging into drm-misc, so shouldn't > > end up blocking you. > This swap_state hook is used for connector state. We could probably add a swap_state hook that drivers > can use for this in the core, but the connector state is already part of the state, adding it twice is silly. > > Instead, would adding a hook to the core be useful? Why do you need a hook to swap the state? The core already does that for you ... to_tv_state(intel_tv->base.base->state) is probably what you want, instead of deref'ing a new intel_tv->format. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v3. 2017-03-16 14:56 ` Daniel Vetter @ 2017-03-16 15:27 ` Maarten Lankhorst 2017-03-16 15:43 ` Daniel Vetter 0 siblings, 1 reply; 25+ messages in thread From: Maarten Lankhorst @ 2017-03-16 15:27 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx Op 16-03-17 om 15:56 schreef Daniel Vetter: > On Thu, Mar 16, 2017 at 01:05:06PM +0100, Maarten Lankhorst wrote: >> Op 16-03-17 om 11:48 schreef Daniel Vetter: >>> On Wed, Mar 15, 2017 at 11:08:56AM +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. >>>> Changes since v2: >>>> - Fix typo in tv_choose_preferred modes function name. >>>> - Assignment of tv properties is done in core, so intel_tv only needs >>>> a atomic_check function. Thanks Ville! >>>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_display.c | 15 +++ >>>> drivers/gpu/drm/i915/intel_drv.h | 4 + >>>> drivers/gpu/drm/i915/intel_tv.c | 200 ++++++++++++++++------------------- >>>> 3 files changed, 110 insertions(+), 109 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); >>>> + } >>>> +} >>> So yeah I'm late, but this is pretty much exactly what DK's private state >>> object stuff was meant for. Have you looked into rebasing on top of that? >>> I think hand-rolling private state stuff everywhere isn't really good. >>> >>> Afaics DK's patch series is ready for merging into drm-misc, so shouldn't >>> end up blocking you. >> This swap_state hook is used for connector state. We could probably add a swap_state hook that drivers >> can use for this in the core, but the connector state is already part of the state, adding it twice is silly. >> >> Instead, would adding a hook to the core be useful? > Why do you need a hook to swap the state? The core already does that for > you ... to_tv_state(intel_tv->base.base->state) is probably what you want, > instead of deref'ing a new intel_tv->format. > -Daniel Well in some way I need to access connector_state->tv.mode in mode_valid, which doesn't hold any connector state locks. I think setting it from the state is the easiest way to do so. But I'm open to other approaches. :) ~Maarten _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v3. 2017-03-16 15:27 ` Maarten Lankhorst @ 2017-03-16 15:43 ` Daniel Vetter 0 siblings, 0 replies; 25+ messages in thread From: Daniel Vetter @ 2017-03-16 15:43 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx On Thu, Mar 16, 2017 at 04:27:42PM +0100, Maarten Lankhorst wrote: > Op 16-03-17 om 15:56 schreef Daniel Vetter: > > On Thu, Mar 16, 2017 at 01:05:06PM +0100, Maarten Lankhorst wrote: > >> Op 16-03-17 om 11:48 schreef Daniel Vetter: > >>> On Wed, Mar 15, 2017 at 11:08:56AM +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. > >>>> Changes since v2: > >>>> - Fix typo in tv_choose_preferred modes function name. > >>>> - Assignment of tv properties is done in core, so intel_tv only needs > >>>> a atomic_check function. Thanks Ville! > >>>> > >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>> --- > >>>> drivers/gpu/drm/i915/intel_display.c | 15 +++ > >>>> drivers/gpu/drm/i915/intel_drv.h | 4 + > >>>> drivers/gpu/drm/i915/intel_tv.c | 200 ++++++++++++++++------------------- > >>>> 3 files changed, 110 insertions(+), 109 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); > >>>> + } > >>>> +} > >>> So yeah I'm late, but this is pretty much exactly what DK's private state > >>> object stuff was meant for. Have you looked into rebasing on top of that? > >>> I think hand-rolling private state stuff everywhere isn't really good. > >>> > >>> Afaics DK's patch series is ready for merging into drm-misc, so shouldn't > >>> end up blocking you. > >> This swap_state hook is used for connector state. We could probably add a swap_state hook that drivers > >> can use for this in the core, but the connector state is already part of the state, adding it twice is silly. > >> > >> Instead, would adding a hook to the core be useful? > > Why do you need a hook to swap the state? The core already does that for > > you ... to_tv_state(intel_tv->base.base->state) is probably what you want, > > instead of deref'ing a new intel_tv->format. > > -Daniel > > Well in some way I need to access connector_state->tv.mode in mode_valid, which doesn't hold any connector state locks. > I think setting it from the state is the easiest way to do so. But I'm open to other approaches. :) Well fundamentally that access is going to be racy, so you better put a READ_ONCE into mode_valid. But once we accept that it's racy, we might as well chase connector->state, the only thing we need is to rcu-protected the freeing of old atomic state. I've typed such a patch yesterday (for vblank access to crtc->state from vblank interrupts), but then thrown it away again. Because it didn't fix my vblank issue correctly. I'll resurrect and send out. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 2/3] drm/i915: Convert intel_dp_mst connector properties to atomic. 2017-03-09 13:06 [PATCH 0/3] drm/i915: Convert tv/dp_mst and crt connector properties to atomic Maarten Lankhorst 2017-03-09 13:06 ` [PATCH 1/3] drm/i915: Convert intel_tv " Maarten Lankhorst @ 2017-03-09 13:06 ` Maarten Lankhorst 2017-03-09 13:06 ` [PATCH 3/3] drm/i915: Convert intel_crt " Maarten Lankhorst 2017-03-09 16:23 ` ✓ Fi.CI.BAT: success for drm/i915: Convert tv/dp_mst and crt " Patchwork 3 siblings, 0 replies; 25+ messages in thread From: Maarten Lankhorst @ 2017-03-09 13:06 UTC (permalink / raw) To: intel-gfx MST doesn't support setting any properties, but it should still use the atomic helper for setting properties. Only path and tile properties are supported (read-only), so keep using the i915 helper for obtaining those properties. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_dp_mst.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index 094cbdcbcd6d..ab756672e2cf 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -294,14 +294,6 @@ intel_dp_mst_detect(struct drm_connector *connector, bool force) return drm_dp_mst_detect_port(connector, &intel_dp->mst_mgr, intel_connector->port); } -static int -intel_dp_mst_set_property(struct drm_connector *connector, - struct drm_property *property, - uint64_t val) -{ - return 0; -} - static void intel_dp_mst_connector_destroy(struct drm_connector *connector) { @@ -318,7 +310,7 @@ static const struct drm_connector_funcs intel_dp_mst_connector_funcs = { .dpms = drm_atomic_helper_connector_dpms, .detect = intel_dp_mst_detect, .fill_modes = drm_helper_probe_single_connector_modes, - .set_property = intel_dp_mst_set_property, + .set_property = drm_atomic_helper_connector_set_property, .atomic_get_property = intel_connector_atomic_get_property, .late_register = intel_connector_register, .early_unregister = intel_connector_unregister, -- 2.7.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 3/3] drm/i915: Convert intel_crt connector properties to atomic. 2017-03-09 13:06 [PATCH 0/3] drm/i915: Convert tv/dp_mst and crt connector properties to atomic Maarten Lankhorst 2017-03-09 13:06 ` [PATCH 1/3] drm/i915: Convert intel_tv " Maarten Lankhorst 2017-03-09 13:06 ` [PATCH 2/3] drm/i915: Convert intel_dp_mst connector properties to atomic Maarten Lankhorst @ 2017-03-09 13:06 ` Maarten Lankhorst 2017-03-09 16:23 ` ✓ Fi.CI.BAT: success for drm/i915: Convert tv/dp_mst and crt " Patchwork 3 siblings, 0 replies; 25+ messages in thread From: Maarten Lankhorst @ 2017-03-09 13:06 UTC (permalink / raw) To: intel-gfx No properties are supported, so just use the helper and reject everything. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_crt.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index 8c82607294c6..4ee4df37e683 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -778,13 +778,6 @@ static int intel_crt_get_modes(struct drm_connector *connector) return ret; } -static int intel_crt_set_property(struct drm_connector *connector, - struct drm_property *property, - uint64_t value) -{ - return 0; -} - void intel_crt_reset(struct drm_encoder *encoder) { struct drm_i915_private *dev_priv = to_i915(encoder->dev); @@ -816,7 +809,7 @@ static const struct drm_connector_funcs intel_crt_connector_funcs = { .late_register = intel_connector_register, .early_unregister = intel_connector_unregister, .destroy = intel_crt_destroy, - .set_property = intel_crt_set_property, + .set_property = drm_atomic_helper_connector_set_property, .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, .atomic_get_property = intel_connector_atomic_get_property, -- 2.7.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 25+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Convert tv/dp_mst and crt connector properties to atomic. 2017-03-09 13:06 [PATCH 0/3] drm/i915: Convert tv/dp_mst and crt connector properties to atomic Maarten Lankhorst ` (2 preceding siblings ...) 2017-03-09 13:06 ` [PATCH 3/3] drm/i915: Convert intel_crt " Maarten Lankhorst @ 2017-03-09 16:23 ` Patchwork 3 siblings, 0 replies; 25+ messages in thread From: Patchwork @ 2017-03-09 16:23 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx == Series Details == Series: drm/i915: Convert tv/dp_mst and crt connector properties to atomic. URL : https://patchwork.freedesktop.org/series/20990/ State : success == Summary == Series 20990v1 drm/i915: Convert tv/dp_mst and crt connector properties to atomic. https://patchwork.freedesktop.org/api/1.0/series/20990/revisions/1/mbox/ Test gem_ctx_create: Subgroup basic-files: pass -> INCOMPLETE (fi-skl-6700k) fdo#100130 Test gem_exec_flush: Subgroup basic-batch-kernel-default-uc: pass -> FAIL (fi-snb-2600) fdo#100007 Test gem_exec_suspend: Subgroup basic-s4-devices: pass -> DMESG-WARN (fi-bxt-t5700) fdo#100125 fdo#100130 https://bugs.freedesktop.org/show_bug.cgi?id=100130 fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007 fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125 fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time: 474s fi-bsw-n3050 total:278 pass:239 dwarn:0 dfail:0 fail:0 skip:39 time: 616s fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time: 543s fi-bxt-t5700 total:278 pass:257 dwarn:1 dfail:0 fail:0 skip:20 time: 610s fi-byt-j1900 total:278 pass:251 dwarn:0 dfail:0 fail:0 skip:27 time: 507s fi-byt-n2820 total:278 pass:247 dwarn:0 dfail:0 fail:0 skip:31 time: 501s fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 446s fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 435s fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time: 437s fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 507s fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 491s fi-kbl-7500u total:278 pass:259 dwarn:1 dfail:0 fail:0 skip:18 time: 480s fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 509s fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time: 601s fi-skl-6700k total:17 pass:16 dwarn:0 dfail:0 fail:0 skip:0 time: 0s fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 548s fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time: 554s fi-snb-2600 total:278 pass:248 dwarn:0 dfail:0 fail:1 skip:29 time: 421s 510c200742ced5a91d07e48220b669a3c9b30c0c drm-tip: 2017y-03m-09d-15h-21m-14s UTC integration manifest 0f77d01 drm/i915: Convert intel_crt connector properties to atomic. 465968e drm/i915: Convert intel_dp_mst connector properties to atomic. 423cf50 drm/i915: Convert intel_tv connector properties to atomic. == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4116/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2017-03-16 15:43 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-09 13:06 [PATCH 0/3] drm/i915: Convert tv/dp_mst and crt connector properties to atomic Maarten Lankhorst 2017-03-09 13:06 ` [PATCH 1/3] drm/i915: Convert intel_tv " Maarten Lankhorst 2017-03-09 17:36 ` Ville Syrjälä 2017-03-13 9:22 ` Maarten Lankhorst 2017-03-13 9:29 ` Ville Syrjälä 2017-03-13 10:38 ` Maarten Lankhorst 2017-03-13 10:55 ` Ville Syrjälä 2017-03-13 12:19 ` Maarten Lankhorst 2017-03-13 16:10 ` [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2 Maarten Lankhorst 2017-03-13 16:22 ` Ville Syrjälä 2017-03-13 16:35 ` Maarten Lankhorst 2017-03-13 16:44 ` Ville Syrjälä 2017-03-13 17:14 ` Maarten Lankhorst 2017-03-13 16:44 ` Maarten Lankhorst 2017-03-14 18:36 ` Ville Syrjälä 2017-03-15 8:32 ` Maarten Lankhorst 2017-03-15 10:08 ` [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v3 Maarten Lankhorst 2017-03-16 10:48 ` Daniel Vetter 2017-03-16 12:05 ` Maarten Lankhorst 2017-03-16 14:56 ` Daniel Vetter 2017-03-16 15:27 ` Maarten Lankhorst 2017-03-16 15:43 ` Daniel Vetter 2017-03-09 13:06 ` [PATCH 2/3] drm/i915: Convert intel_dp_mst connector properties to atomic Maarten Lankhorst 2017-03-09 13:06 ` [PATCH 3/3] drm/i915: Convert intel_crt " Maarten Lankhorst 2017-03-09 16:23 ` ✓ Fi.CI.BAT: success for drm/i915: Convert tv/dp_mst and crt " Patchwork
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.