Re: [PATCH] drm/i915: add encoder get_config function v3

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, May 07, 2013 at 03:38:41PM -0700, Jesse Barnes wrote:
> We can use this for fetching encoder specific pipe_config state, like
> mode flags, adjusted clock, etc.
> 
> Just used for mode flags atm, so we can check the pipe config state at
> mode set time.
> 
> v2: get_config when checking hw state too
> v3: fix DVO and LVDS mode flags (Ville)
>     get SDVO DTD for flag fetch (Ville)
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_crt.c     |   23 ++++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |   20 +++++++++++++---
>  drivers/gpu/drm/i915/intel_dp.c      |   23 ++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |    4 ++++
>  drivers/gpu/drm/i915/intel_dvo.c     |   21 +++++++++++++++++
>  drivers/gpu/drm/i915/intel_hdmi.c    |   23 ++++++++++++++++++
>  drivers/gpu/drm/i915/intel_lvds.c    |   26 ++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_sdvo.c    |   43 ++++++++++++++++++++++++++++++++++
>  8 files changed, 180 insertions(+), 3 deletions(-)
> 
<snip>
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -712,6 +712,13 @@ static bool intel_sdvo_set_timing(struct intel_sdvo *intel_sdvo, u8 cmd,
>  		intel_sdvo_set_value(intel_sdvo, cmd + 1, &dtd->part2, sizeof(dtd->part2));
>  }
>  
> +static bool intel_sdvo_get_timing(struct intel_sdvo *intel_sdvo, u8 cmd,
> +				  struct intel_sdvo_dtd *dtd)
> +{
> +	return intel_sdvo_get_value(intel_sdvo, cmd, &dtd->part1, sizeof(dtd->part1)) &&
> +		intel_sdvo_get_value(intel_sdvo, cmd + 1, &dtd->part2, sizeof(dtd->part2));
> +}
> +
>  static bool intel_sdvo_set_input_timing(struct intel_sdvo *intel_sdvo,
>  					 struct intel_sdvo_dtd *dtd)
>  {
> @@ -726,6 +733,13 @@ static bool intel_sdvo_set_output_timing(struct intel_sdvo *intel_sdvo,
>  				     SDVO_CMD_SET_OUTPUT_TIMINGS_PART1, dtd);
>  }
>  
> +static bool intel_sdvo_get_output_timing(struct intel_sdvo *intel_sdvo,
> +					 struct intel_sdvo_dtd *dtd)
> +{
> +	return intel_sdvo_get_timing(intel_sdvo,
> +				     SDVO_CMD_GET_OUTPUT_TIMINGS_PART2, dtd);

PART1

Oh and it looks like we actually feed adjusted_mode into the input timings,
not the output timings. So I'm thinking we should use the input timings in
get_config() too.

> +}
> +
>  static bool
>  intel_sdvo_create_preferred_input_timing(struct intel_sdvo *intel_sdvo,
>  					 uint16_t clock,
> @@ -1264,6 +1278,33 @@ static bool intel_sdvo_get_hw_state(struct intel_encoder *encoder,
>  	return true;
>  }
>  
> +static void intel_sdvo_get_config(struct intel_encoder *encoder,
> +				  struct intel_crtc_config *pipe_config)
> +{
> +	struct intel_sdvo *intel_sdvo = to_intel_sdvo(&encoder->base);
> +	struct intel_sdvo_dtd dtd;
> +	u32 flags = 0;
> +	bool ret;
> +
> +	ret = intel_sdvo_get_output_timing(intel_sdvo, &dtd);
> +	if (!ret) {
> +		DRM_DEBUG_DRIVER("failed to retrieve SDVO DTD\n");
> +		return;
> +	}
> +
> +	if (dtd.part2.dtd_flags & DTD_FLAG_HSYNC_POSITIVE)
> +		flags |= DRM_MODE_FLAG_PHSYNC;
> +	else
> +		flags |= DRM_MODE_FLAG_NHSYNC;
> +
> +	if (dtd.part2.dtd_flags & DTD_FLAG_VSYNC_POSITIVE)
> +		flags |= DRM_MODE_FLAG_PVSYNC;
> +	else
> +		flags |= DRM_MODE_FLAG_NVSYNC;
> +
> +	pipe_config->adjusted_mode.flags |= flags;
> +}
> +
>  static void intel_disable_sdvo(struct intel_encoder *encoder)
>  {
>  	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> @@ -2793,6 +2834,8 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
>  	intel_encoder->mode_set = intel_sdvo_mode_set;
>  	intel_encoder->enable = intel_enable_sdvo;
>  	intel_encoder->get_hw_state = intel_sdvo_get_hw_state;
> +	if (INTEL_INFO(dev)->gen >= 4)
> +		intel_encoder->get_config = intel_sdvo_get_config;

I'm assuming the gen4 check can be dropped now.

So, as we discussed on irc, the only issue left is that we don't sanitize
the adjusted_mode sync flags. I think they just get copied from the user
specified mode directly, so if the user specifies neither + or - flag,
or specifies both, intel_pipe_config_compare() will scream.

>  	/* In default case sdvo lvds is false */
>  	if (!intel_sdvo_get_capabilities(intel_sdvo, &intel_sdvo->caps))
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC