Re: [PATCH 09/13] drm/i915: Setup dummy atomic state for connectors

[Ander Conselvan de Oliveira <conselvan2@gmail.com>]

On 01/20/2015 05:57 AM, Matt Roper wrote:
> We want to enable/test plane updates via the atomic interface, but as
> soon as we flip DRIVER_ATOMIC on, the DRM core will take some atomic
> codepaths to lookup properties during drmModeGetConnector() and some of
> those codepaths unconditionally dereference connector->state
> (specifically when looking up the CRTC ID property in
> drm_atomic_connector_get_property()).  Create a dummy connector state
> for each connector at init time to ensure the DRM core doesn't try to
> dereference a NULL connector->state.  The actual connector properties
> will never be updated or contain useful information, but since we're
> doing this specifically for testing/debug of the plane operations (and
> only when a specific kernel module option is given), that shouldn't
> really matter.
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0e18879..82defd3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12368,6 +12368,7 @@ static void intel_setup_outputs(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_encoder *encoder;
> +	struct drm_connector *connector;
>   	bool dpd_is_edp = false;
>
>   	intel_lvds_init(dev);
> @@ -12498,6 +12499,33 @@ static void intel_setup_outputs(struct drm_device *dev)
>   	if (SUPPORTS_TV(dev))
>   		intel_tv_init(dev);
>
> +	/*
> +	 * FIXME:  We don't have full atomic support yet, but we want to be
> +	 * able to enable/test plane updates via the atomic interface in the
> +	 * meantime.  However as soon as we flip DRIVER_ATOMIC on, the DRM core
> +	 * will take some atomic codepaths to lookup properties during
> +	 * drmModeGetConnector() that unconditionally dereference
> +	 * connector->state.
> +	 *
> +	 * We create a dummy connector state here for each connector to ensure
> +	 * the DRM core doesn't try to dereference a NULL connector->state.
> +	 * The actual connector properties will never be updated or contain
> +	 * useful information, but since we're doing this specifically for
> +	 * testing/debug of the plane operations (and only when a specific
> +	 * kernel module option is given), that shouldn't really matter.
> +	 *
> +	 * Once atomic support for crtc's + connectors lands, this loop should
> +	 * be removed since we'll be setting up real connector state, which
> +	 * will contain Intel-specific properties.
> +	 */
> +	list_for_each_entry(connector,
> +			    &dev->mode_config.connector_list,
> +			    head) {
> +		if (!WARN_ON(connector->state))
> +			connector->state = kzalloc(sizeof(*connector->state),
> +						   GFP_KERNEL);
> +	}
> +

I think this will trigger a WARN in drm_connector_cleanup() on driver 
unload, and we also leak the dummy state.

Also, I'm not sure if this changes the behaviour of 
drm_mode_getconnector(), since now the call to 
drm_connector_get_encoder() will always return NULL.

Ander

>   	intel_psr_init(dev);
>
>   	for_each_intel_encoder(dev, encoder) {
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx