Received-SPF: pass (google.com: domain of linux-kernel+bounces-14316-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: Jani Nikula <jani.nikula@linux.intel.com>
To: Javier Martinez Canillas <javierm@redhat.com>, linux-kernel@vger.kernel.org
Cc: Thomas Zimmermann <tzimmermann@suse.de>, Javier Martinez Canillas
 <javierm@redhat.com>, Maxime Ripard <mripard@kernel.org>,
 dri-devel@lists.freedesktop.org, David Airlie <airlied@gmail.com>
Subject: Re: [PATCH] drm: Move drm_set_preferred_mode() helper from drm_edid
 to drm_modes
In-Reply-To: <20240102111921.3057255-1-javierm@redhat.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
References: <20240102111921.3057255-1-javierm@redhat.com>
Date: Tue, 02 Jan 2024 13:54:22 +0200
Message-ID: <87y1d80x3l.fsf@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On Tue, 02 Jan 2024, Javier Martinez Canillas <javierm@redhat.com> wrote:
> The helper is generic and doesn't use the opaque EDID type struct drm_edid
> and is also used by drivers that only support non-probeable displays, such
> as fixed panels.
>
> These drivers add a list of modes using drm_mode_probed_add() and then set
> a preferred mode using the drm_set_preferred_mode() helper.
>
> It seems more logical to have the helper definition in drm_modes.o instead
> of drm_edid.o, since the former contains modes helper while the latter has
> helpers to manage the EDID information.
>
> Since both drm_edid.o and drm_modes.o object files are built-in the drm.o
> object, there are no functional changes. But besides being a more logical
> place for this helper, it could also allow to eventually make drm_edid.o
> optional and not included in drm.o if only fixed panels must be supported
> in a given system.
>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
>  drivers/gpu/drm/drm_edid.c  | 23 +----------------------
>  drivers/gpu/drm/drm_modes.c | 22 ++++++++++++++++++++++
>  include/drm/drm_edid.h      |  2 --
>  include/drm/drm_modes.h     |  2 ++
>  4 files changed, 25 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index cb4031d5dcbb..48dd2a0a0395 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -43,6 +43,7 @@
>  #include <drm/drm_edid.h>
>  #include <drm/drm_eld.h>
>  #include <drm/drm_encoder.h>
> +#include <drm/drm_modes.h>

Unnecessary.

Other than that,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>  #include <drm/drm_print.h>
>  
>  #include "drm_crtc_internal.h"
> @@ -6989,28 +6990,6 @@ int drm_add_modes_noedid(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_add_modes_noedid);
>  
> -/**
> - * drm_set_preferred_mode - Sets the preferred mode of a connector
> - * @connector: connector whose mode list should be processed
> - * @hpref: horizontal resolution of preferred mode
> - * @vpref: vertical resolution of preferred mode
> - *
> - * Marks a mode as preferred if it matches the resolution specified by @hpref
> - * and @vpref.
> - */
> -void drm_set_preferred_mode(struct drm_connector *connector,
> -			   int hpref, int vpref)
> -{
> -	struct drm_display_mode *mode;
> -
> -	list_for_each_entry(mode, &connector->probed_modes, head) {
> -		if (mode->hdisplay == hpref &&
> -		    mode->vdisplay == vpref)
> -			mode->type |= DRM_MODE_TYPE_PREFERRED;
> -	}
> -}
> -EXPORT_SYMBOL(drm_set_preferred_mode);
> -
>  static bool is_hdmi2_sink(const struct drm_connector *connector)
>  {
>  	/*
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index ac9a406250c5..01aa44e87534 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -2754,3 +2754,25 @@ bool drm_mode_is_420(const struct drm_display_info *display,
>  		drm_mode_is_420_also(display, mode);
>  }
>  EXPORT_SYMBOL(drm_mode_is_420);
> +
> +/**
> + * drm_set_preferred_mode - Sets the preferred mode of a connector
> + * @connector: connector whose mode list should be processed
> + * @hpref: horizontal resolution of preferred mode
> + * @vpref: vertical resolution of preferred mode
> + *
> + * Marks a mode as preferred if it matches the resolution specified by @hpref
> + * and @vpref.
> + */
> +void drm_set_preferred_mode(struct drm_connector *connector,
> +			   int hpref, int vpref)
> +{
> +	struct drm_display_mode *mode;
> +
> +	list_for_each_entry(mode, &connector->probed_modes, head) {
> +		if (mode->hdisplay == hpref &&
> +		    mode->vdisplay == vpref)
> +			mode->type |= DRM_MODE_TYPE_PREFERRED;
> +	}
> +}
> +EXPORT_SYMBOL(drm_set_preferred_mode);
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 54cc6f04a708..5bd6b6eb6c57 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -426,8 +426,6 @@ enum hdmi_quantization_range
>  drm_default_rgb_quant_range(const struct drm_display_mode *mode);
>  int drm_add_modes_noedid(struct drm_connector *connector,
>  			 int hdisplay, int vdisplay);
> -void drm_set_preferred_mode(struct drm_connector *connector,
> -			    int hpref, int vpref);
>  
>  int drm_edid_header_is_valid(const void *edid);
>  bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> index c613f0abe9dc..b9bb92e4b029 100644
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -467,6 +467,8 @@ bool drm_mode_is_420_also(const struct drm_display_info *display,
>  			  const struct drm_display_mode *mode);
>  bool drm_mode_is_420(const struct drm_display_info *display,
>  		     const struct drm_display_mode *mode);
> +void drm_set_preferred_mode(struct drm_connector *connector,
> +			    int hpref, int vpref);
>  
>  struct drm_display_mode *drm_analog_tv_mode(struct drm_device *dev,
>  					    enum drm_connector_tv_mode mode,

-- 
Jani Nikula, Intel