Re: [Intel-gfx] [PATCH v6 2/3] drm/i915 : Changing intel_connector iterators

[Jani Nikula <jani.nikula@intel.com>]

On Mon, 19 Sep 2022, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote:
> From: Suraj Kandpal <suraj.kandpal@intel.com>
>
> Changing intel_connector iterators as with writeback introduction
> not all drm_connector will be embedded within intel_connector.
>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_acpi.c     |  7 ++++-
>  drivers/gpu/drm/i915/display/intel_display.h  |  7 ++---
>  .../drm/i915/display/intel_display_types.h    | 26 ++++++++++++++++++-
>  .../drm/i915/display/intel_modeset_setup.c    | 16 +++++++++---
>  4 files changed, 46 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c b/drivers/gpu/drm/i915/display/intel_acpi.c
> index 9df78e7caa2b..912fe5c2ffe5 100644
> --- a/drivers/gpu/drm/i915/display/intel_acpi.c
> +++ b/drivers/gpu/drm/i915/display/intel_acpi.c
> @@ -349,8 +349,13 @@ void intel_acpi_video_register(struct drm_i915_private *i915)
>  	 */
>  	drm_connector_list_iter_begin(&i915->drm, &conn_iter);
>  	drm_for_each_connector_iter(connector, &conn_iter) {
> -		struct intel_panel *panel = &to_intel_connector(connector)->panel;
> +		struct intel_panel *panel;
> +		struct intel_connector *intel_connector =
> +					to_intel_connector(connector);
>  
> +		if (!intel_connector)
> +			continue;
> +		panel = &intel_connector->panel;
>  		if (panel->backlight.funcs && !panel->backlight.device) {
>  			acpi_video_register_backlight();
>  			break;
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> index a1ed9c82e2ed..102bf7d47ccc 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -52,6 +52,7 @@ struct intel_crtc_state;
>  struct intel_digital_port;
>  struct intel_dp;
>  struct intel_encoder;
> +struct intel_connector;
>  struct intel_initial_plane_config;
>  struct intel_load_detect_pipe;
>  struct intel_plane;
> @@ -469,16 +470,12 @@ enum hpd_pin {
>  		for_each_if(intel_encoder_can_psr(intel_encoder))
>  
>  #define for_each_intel_connector_iter(intel_connector, iter) \
> -	while ((intel_connector = to_intel_connector(drm_connector_list_iter_next(iter))))
> +	while ((intel_connector = intel_connector_list_iter_next(iter)))
>  
>  #define for_each_encoder_on_crtc(dev, __crtc, intel_encoder) \
>  	list_for_each_entry((intel_encoder), &(dev)->mode_config.encoder_list, base.head) \
>  		for_each_if((intel_encoder)->base.crtc == (__crtc))
>  
> -#define for_each_connector_on_encoder(dev, __encoder, intel_connector) \
> -	list_for_each_entry((intel_connector), &(dev)->mode_config.connector_list, base.head) \
> -		for_each_if((intel_connector)->base.encoder == (__encoder))
> -

Getting rid of this macro should be a separate change. As the first
patch, could've been merged already.

>  #define for_each_old_intel_plane_in_state(__state, plane, old_plane_state, __i) \
>  	for ((__i) = 0; \
>  	     (__i) < (__state)->base.dev->mode_config.num_total_plane && \
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 633cacd79074..a2d294929a64 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1498,12 +1498,14 @@ struct cxsr_latency {
>  #define to_intel_atomic_state(x) container_of(x, struct intel_atomic_state, base)
>  #define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
>  #define to_intel_crtc_state(x) container_of(x, struct intel_crtc_state, uapi)
> -#define to_intel_connector(x) container_of(x, struct intel_connector, base)
> +#define to_intel_wb_connector(x) container_of(x, struct intel_wb_connector, base)

Note that all of the macros here are sorted alphabetically.

The wb/wd difference is a pretty bad eyesore. Maybe we should use one or
the other, not mix them. (Except if we go with writeback, leave the
register macros WD because that's what they are.)

>  #define to_intel_encoder(x) container_of(x, struct intel_encoder, base)
>  #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
>  #define to_intel_plane(x) container_of(x, struct intel_plane, base)
>  #define to_intel_plane_state(x) container_of(x, struct intel_plane_state, uapi)
>  #define intel_fb_obj(x) ((x) ? to_intel_bo((x)->obj[0]) : NULL)
> +#define to_intel_connector(x) (((x->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)) ?	\
> +				NULL : container_of(x, struct intel_connector, base))

Would need to have (x)->connector_type, with parenthesis.

The problem with this is that we currently have 131 uses of
to_intel_connector() and practically all of them expect to get non-NULL
result.

You're going to get 131 static analyzer reports that you don't check for
NULL. You can't check for NULL in most places, because they're in the
middle of local parameter initialization.

>  
>  struct intel_hdmi {
>  	i915_reg_t hdmi_reg;
> @@ -2069,4 +2071,26 @@ to_intel_frontbuffer(struct drm_framebuffer *fb)
>  	return fb ? to_intel_framebuffer(fb)->frontbuffer : NULL;
>  }
>  
> +static inline struct intel_connector *
> +intel_connector_list_iter_next(struct drm_connector_list_iter *iter)
> +{
> +	struct drm_connector *connector;
> +	bool flag = true;
> +	/*
> +	 * Skipping connector that are Writeback connector as they will
> +	 * not be embedded in intel connector
> +	 */
> +	while (flag) {
> +		connector = drm_connector_list_iter_next(iter);
> +		if (connector && !to_intel_connector(connector))
> +			continue;
> +
> +		flag = false;
> +
> +		if (connector)
> +			return to_intel_connector(connector);
> +
> +	}
> +	return NULL;
> +}

This gets pretty ugly. I've got an idea, I'll send patches later
today.

Code is worth a thousand words, it's easier to explain that way.

BR,
Jani.

>  #endif /*  __INTEL_DISPLAY_TYPES_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> index cbfabd58b75a..e1a90331c230 100644
> --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> @@ -205,12 +205,22 @@ static bool intel_crtc_has_encoders(struct intel_crtc *crtc)
>  
>  static struct intel_connector *intel_encoder_find_connector(struct intel_encoder *encoder)
>  {
> -	struct drm_device *dev = encoder->base.dev;
> +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>  	struct intel_connector *connector;
> +	struct drm_connector_list_iter conn_iter;
> +	bool found_connector = false;
>  
> -	for_each_connector_on_encoder(dev, &encoder->base, connector)
> -		return connector;
> +	drm_connector_list_iter_begin(&i915->drm, &conn_iter);
> +	for_each_intel_connector_iter(connector, &conn_iter) {
> +		if (&encoder->base == connector->base.encoder) {
> +			found_connector = true;
> +			break;
> +		}
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
>  
> +	if (found_connector)
> +		return connector;
>  	return NULL;
>  }

-- 
Jani Nikula, Intel Open Source Graphics Center