On Mon, Sep 17, 2018 at 08:34:36PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 17, 2018 at 10:16:05AM -0700, Rodrigo Vivi wrote:
> > On Mon, Sep 17, 2018 at 06:15:03PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > >
> > > SDVO encoders can have multiple different types of outputs hanging off
> > > them. Currently the code tries to muck around with various is_foo
> > > flags in the encoder to figure out which type its driving. That doesn't
> > > work with atomic and other stuff, so let's nuke those flags and just
> > > look at which type of connector we're actually dealing with.
> > >
> > > The is_hdmi we'll need as that's not discoverable via the output flags,
> > > but we'll just move it under the connector.
> > >
> > > We'll also move the sdvo fixed mode handling out from the .get_modes()
> > > hook into the sdvo lvds init function so that we can bail out properly
> > > if there is no fixed mode to be found.
> >
> > I wondered if this last part didn't deserved a separated patch. But not sure
> > how chicken-egg issue this could cause so maybe the same patch is better
> > indeed...
>
> There are both chickens and eggs in there for sure. I suppose it might
> have been possible to handle it by clearing the LVDS bit from
> output_flag if we fail to find the mode. But then we'd be left with
> an LVDS connector without the flag and I'm not quite sure where the
> code would lead if we ever tried to use it. Although one can argue
> that the current code already has that same issue, maybe.
Decided to keep this patch as is, and pushed it. Thanks for the review.
>
> >
> > Reviewed-by: Rodrigo Vivi <rodrigo.v...@intel.com>
> >
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > > ---
> > > drivers/gpu/drm/i915/intel_sdvo.c | 101
> > > +++++++++++++++++---------------------
> > > 1 file changed, 44 insertions(+), 57 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c
> > > b/drivers/gpu/drm/i915/intel_sdvo.c
> > > index 812fe7b06f87..701372e512a8 100644
> > > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > > @@ -99,31 +99,12 @@ struct intel_sdvo {
> > > */
> > > uint16_t hotplug_active;
> > >
> > > - /**
> > > - * This is set if we're going to treat the device as TV-out.
> > > - *
> > > - * While we have these nice friendly flags for output types that ought
> > > - * to decide this for us, the S-Video output on our HDMI+S-Video card
> > > - * shows up as RGB1 (VGA).
> > > - */
> > > - bool is_tv;
> > > -
> > > enum port port;
> > >
> > > - /**
> > > - * This is set if we treat the device as HDMI, instead of DVI.
> > > - */
> > > - bool is_hdmi;
> > > bool has_hdmi_monitor;
> > > bool has_hdmi_audio;
> > > bool rgb_quant_range_selectable;
> > >
> > > - /**
> > > - * This is set if we detect output of sdvo device as LVDS and
> > > - * have a valid fixed mode to use with the panel.
> > > - */
> > > - bool is_lvds;
> > > -
> > > /**
> > > * This is sdvo fixed pannel mode pointer
> > > */
> > > @@ -172,6 +153,11 @@ struct intel_sdvo_connector {
> > >
> > > /* this is to get the range of margin.*/
> > > u32 max_hscan, max_vscan;
> > > +
> > > + /**
> > > + * This is set if we treat the device as HDMI, instead of DVI.
> > > + */
> > > + bool is_hdmi;
> > > };
> > >
> > > struct intel_sdvo_connector_state {
> > > @@ -766,6 +752,7 @@ static bool intel_sdvo_get_input_timing(struct
> > > intel_sdvo *intel_sdvo,
> > >
> > > static bool
> > > intel_sdvo_create_preferred_input_timing(struct intel_sdvo *intel_sdvo,
> > > + struct intel_sdvo_connector
> > > *intel_sdvo_connector,
> > > uint16_t clock,
> > > uint16_t width,
> > > uint16_t height)
> > > @@ -778,7 +765,7 @@ intel_sdvo_create_preferred_input_timing(struct
> > > intel_sdvo *intel_sdvo,
> > > args.height = height;
> > > args.interlace = 0;
> > >
> > > - if (intel_sdvo->is_lvds &&
> > > + if (IS_LVDS(intel_sdvo_connector) &&
> > > (intel_sdvo->sdvo_lvds_fixed_mode->hdisplay != width ||
> > > intel_sdvo->sdvo_lvds_fixed_mode->vdisplay != height))
> > > args.scaled = 1;
> > > @@ -1067,6 +1054,7 @@ intel_sdvo_set_output_timings_from_mode(struct
> > > intel_sdvo *intel_sdvo,
> > > */
> > > static bool
> > > intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
> > > + struct intel_sdvo_connector
> > > *intel_sdvo_connector,
> > > const struct drm_display_mode *mode,
> > > struct drm_display_mode *adjusted_mode)
> > > {
> > > @@ -1077,6 +1065,7 @@ intel_sdvo_get_preferred_input_mode(struct
> > > intel_sdvo *intel_sdvo,
> > > return false;
> > >
> > > if (!intel_sdvo_create_preferred_input_timing(intel_sdvo,
> > > + intel_sdvo_connector,
> > > mode->clock / 10,
> > > mode->hdisplay,
> > > mode->vdisplay))
> > > @@ -1127,6 +1116,8 @@ static bool intel_sdvo_compute_config(struct
> > > intel_encoder *encoder,
> > > struct intel_sdvo *intel_sdvo = to_sdvo(encoder);
> > > struct intel_sdvo_connector_state *intel_sdvo_state =
> > > to_intel_sdvo_connector_state(conn_state);
> > > + struct intel_sdvo_connector *intel_sdvo_connector =
> > > + to_intel_sdvo_connector(conn_state->connector);
> > > struct drm_display_mode *adjusted_mode =
> > > &pipe_config->base.adjusted_mode;
> > > struct drm_display_mode *mode = &pipe_config->base.mode;
> > >
> > > @@ -1142,20 +1133,22 @@ static bool intel_sdvo_compute_config(struct
> > > intel_encoder *encoder,
> > > * timings, even though this isn't really the right place in
> > > * the sequence to do it. Oh well.
> > > */
> > > - if (intel_sdvo->is_tv) {
> > > + if (IS_TV(intel_sdvo_connector)) {
> > > if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo, mode))
> > > return false;
> > >
> > > (void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
> > > + intel_sdvo_connector,
> > > mode,
> > > adjusted_mode);
> > > pipe_config->sdvo_tv_clock = true;
> > > - } else if (intel_sdvo->is_lvds) {
> > > + } else if (IS_LVDS(intel_sdvo_connector)) {
> > > if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo,
> > >
> > > intel_sdvo->sdvo_lvds_fixed_mode))
> > > return false;
> > >
> > > (void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
> > > + intel_sdvo_connector,
> > > mode,
> > > adjusted_mode);
> > > }
> > > @@ -1194,11 +1187,11 @@ static bool intel_sdvo_compute_config(struct
> > > intel_encoder *encoder,
> > > }
> > >
> > > /* Clock computation needs to happen after pixel multiplier. */
> > > - if (intel_sdvo->is_tv)
> > > + if (IS_TV(intel_sdvo_connector))
> > > i9xx_adjust_sdvo_tv_clock(pipe_config);
> > >
> > > /* Set user selected PAR to incoming mode's member */
> > > - if (intel_sdvo->is_hdmi)
> > > + if (intel_sdvo_connector->is_hdmi)
> > > adjusted_mode->picture_aspect_ratio =
> > > conn_state->picture_aspect_ratio;
> > >
> > > return true;
> > > @@ -1275,6 +1268,8 @@ static void intel_sdvo_pre_enable(struct
> > > intel_encoder *intel_encoder,
> > > const struct drm_display_mode *adjusted_mode =
> > > &crtc_state->base.adjusted_mode;
> > > const struct intel_sdvo_connector_state *sdvo_state =
> > > to_intel_sdvo_connector_state(conn_state);
> > > + const struct intel_sdvo_connector *intel_sdvo_connector =
> > > + to_intel_sdvo_connector(conn_state->connector);
> > > const struct drm_display_mode *mode = &crtc_state->base.mode;
> > > struct intel_sdvo *intel_sdvo = to_sdvo(intel_encoder);
> > > u32 sdvox;
> > > @@ -1304,7 +1299,7 @@ static void intel_sdvo_pre_enable(struct
> > > intel_encoder *intel_encoder,
> > > return;
> > >
> > > /* lvds has a special fixed output timing. */
> > > - if (intel_sdvo->is_lvds)
> > > + if (IS_LVDS(intel_sdvo_connector))
> > > intel_sdvo_get_dtd_from_mode(&output_dtd,
> > > intel_sdvo->sdvo_lvds_fixed_mode);
> > > else
> > > @@ -1325,13 +1320,13 @@ static void intel_sdvo_pre_enable(struct
> > > intel_encoder *intel_encoder,
> > > } else
> > > intel_sdvo_set_encode(intel_sdvo, SDVO_ENCODE_DVI);
> > >
> > > - if (intel_sdvo->is_tv &&
> > > + if (IS_TV(intel_sdvo_connector) &&
> > > !intel_sdvo_set_tv_format(intel_sdvo, conn_state))
> > > return;
> > >
> > > intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
> > >
> > > - if (intel_sdvo->is_tv || intel_sdvo->is_lvds)
> > > + if (IS_TV(intel_sdvo_connector) || IS_LVDS(intel_sdvo_connector))
> > > input_dtd.part2.sdvo_flags = intel_sdvo->dtd_sdvo_flags;
> > > if (!intel_sdvo_set_input_timing(intel_sdvo, &input_dtd))
> > > DRM_INFO("Setting input timings on %s failed\n",
> > > @@ -1630,6 +1625,8 @@ intel_sdvo_mode_valid(struct drm_connector
> > > *connector,
> > > struct drm_display_mode *mode)
> > > {
> > > struct intel_sdvo *intel_sdvo = intel_attached_sdvo(connector);
> > > + struct intel_sdvo_connector *intel_sdvo_connector =
> > > + to_intel_sdvo_connector(connector);
> > > int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
> > >
> > > if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> > > @@ -1644,7 +1641,7 @@ intel_sdvo_mode_valid(struct drm_connector
> > > *connector,
> > > if (mode->clock > max_dotclk)
> > > return MODE_CLOCK_HIGH;
> > >
> > > - if (intel_sdvo->is_lvds) {
> > > + if (IS_LVDS(intel_sdvo_connector)) {
> > > if (mode->hdisplay > intel_sdvo->sdvo_lvds_fixed_mode->hdisplay)
> > > return MODE_PANEL;
> > >
> > > @@ -1759,6 +1756,8 @@ static enum drm_connector_status
> > > intel_sdvo_tmds_sink_detect(struct drm_connector *connector)
> > > {
> > > struct intel_sdvo *intel_sdvo = intel_attached_sdvo(connector);
> > > + struct intel_sdvo_connector *intel_sdvo_connector =
> > > + to_intel_sdvo_connector(connector);
> > > enum drm_connector_status status;
> > > struct edid *edid;
> > >
> > > @@ -1797,7 +1796,7 @@ intel_sdvo_tmds_sink_detect(struct drm_connector
> > > *connector)
> > > /* DDC bus is shared, match EDID to connector type */
> > > if (edid->input & DRM_EDID_INPUT_DIGITAL) {
> > > status = connector_status_connected;
> > > - if (intel_sdvo->is_hdmi) {
> > > + if (intel_sdvo_connector->is_hdmi) {
> > > intel_sdvo->has_hdmi_monitor =
> > > drm_detect_hdmi_monitor(edid);
> > > intel_sdvo->has_hdmi_audio =
> > > drm_detect_monitor_audio(edid);
> > > intel_sdvo->rgb_quant_range_selectable =
> > > @@ -1875,17 +1874,6 @@ intel_sdvo_detect(struct drm_connector *connector,
> > > bool force)
> > > ret = connector_status_connected;
> > > }
> > >
> > > - /* May update encoder flag for like clock for SDVO TV, etc.*/
> > > - if (ret == connector_status_connected) {
> > > - intel_sdvo->is_tv = false;
> > > - intel_sdvo->is_lvds = false;
> > > -
> > > - if (response & SDVO_TV_MASK)
> > > - intel_sdvo->is_tv = true;
> > > - if (response & SDVO_LVDS_MASK)
> > > - intel_sdvo->is_lvds = intel_sdvo->sdvo_lvds_fixed_mode
> > > != NULL;
> > > - }
> > > -
> > > return ret;
> > > }
> > >
> > > @@ -2054,16 +2042,6 @@ static void intel_sdvo_get_lvds_modes(struct
> > > drm_connector *connector)
> > > * arranged in priority order.
> > > */
> > > intel_ddc_get_modes(connector, &intel_sdvo->ddc);
> > > -
> > > - list_for_each_entry(newmode, &connector->probed_modes, head) {
> > > - if (newmode->type & DRM_MODE_TYPE_PREFERRED) {
> > > - intel_sdvo->sdvo_lvds_fixed_mode =
> > > - drm_mode_duplicate(connector->dev, newmode);
> > > -
> > > - intel_sdvo->is_lvds = true;
> > > - break;
> > > - }
> > > - }
> > > }
> > >
> > > static int intel_sdvo_get_modes(struct drm_connector *connector)
> > > @@ -2555,7 +2533,7 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo,
> > > int device)
> > > if (INTEL_GEN(dev_priv) >= 4 &&
> > > intel_sdvo_is_hdmi_connector(intel_sdvo, device)) {
> > > connector->connector_type = DRM_MODE_CONNECTOR_HDMIA;
> > > - intel_sdvo->is_hdmi = true;
> > > + intel_sdvo_connector->is_hdmi = true;
> > > }
> > >
> > > if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> > > @@ -2563,7 +2541,7 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo,
> > > int device)
> > > return false;
> > > }
> > >
> > > - if (intel_sdvo->is_hdmi)
> > > + if (intel_sdvo_connector->is_hdmi)
> > > intel_sdvo_add_hdmi_properties(intel_sdvo,
> > > intel_sdvo_connector);
> > >
> > > return true;
> > > @@ -2591,8 +2569,6 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo,
> > > int type)
> > > intel_sdvo->controlled_output |= type;
> > > intel_sdvo_connector->output_flag = type;
> > >
> > > - intel_sdvo->is_tv = true;
> > > -
> > > if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> > > kfree(intel_sdvo_connector);
> > > return false;
> > > @@ -2654,6 +2630,7 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo,
> > > int device)
> > > struct drm_connector *connector;
> > > struct intel_connector *intel_connector;
> > > struct intel_sdvo_connector *intel_sdvo_connector;
> > > + struct drm_display_mode *mode;
> > >
> > > DRM_DEBUG_KMS("initialising LVDS device %d\n", device);
> > >
> > > @@ -2682,6 +2659,19 @@ intel_sdvo_lvds_init(struct intel_sdvo
> > > *intel_sdvo, int device)
> > > if (!intel_sdvo_create_enhance_property(intel_sdvo,
> > > intel_sdvo_connector))
> > > goto err;
> > >
> > > + intel_sdvo_get_lvds_modes(connector);
> > > +
> > > + list_for_each_entry(mode, &connector->probed_modes, head) {
> > > + if (mode->type & DRM_MODE_TYPE_PREFERRED) {
> > > + intel_sdvo->sdvo_lvds_fixed_mode =
> > > + drm_mode_duplicate(connector->dev, mode);
> > > + break;
> > > + }
> > > + }
> > > +
> > > + if (!intel_sdvo->sdvo_lvds_fixed_mode)
> > > + goto err;
> > > +
> > > return true;
> > >
> > > err:
> > > @@ -2692,9 +2682,6 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo,
> > > int device)
> > > static bool
> > > intel_sdvo_output_setup(struct intel_sdvo *intel_sdvo, uint16_t flags)
> > > {
> > > - intel_sdvo->is_tv = false;
> > > - intel_sdvo->is_lvds = false;
> > > -
> > > /* SDVO requires XXX1 function may not exist unless it has XXX0
> > > function.*/
> > >
> > > if (flags & SDVO_OUTPUT_TMDS0)
> > > --
> > > 2.16.4
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
