Re: [Intel-gfx] [PATCH resend 1/2] drm/connector: Split out orientation quirk detection (v2)

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Jan 02, 2020 at 10:35:28PM +0100, Hans de Goede wrote:
> Hi Rodrigo,
> 
> Thank you for the review.
> 
> On 02-01-2020 19:17, Rodrigo Vivi wrote:
> > On Mon, Dec 16, 2019 at 12:51:57PM +0100, Hans de Goede wrote:
> > > From: Derek Basehore <dbasehore@chromium.org>
> > > 
> > > Not every platform needs quirk detection for panel orientation, so
> > > split the drm_connector_init_panel_orientation_property into two
> > > functions. One for platforms without the need for quirks, and the
> > > other for platforms that need quirks.
> > > 
> > > Hans de Goede (changes in v2):
> > > 
> > > Rename the function from drm_connector_init_panel_orientation_property
> > > to drm_connector_set_panel_orientation[_with_quirk] and pass in the
> > > panel-orientation to set.
> > > 
> > > Beside the rename, also make the function set the passed in value
> > > only once, if the value was set before (to a value other then
> > > DRM_MODE_PANEL_ORIENTATION_UNKNOWN) make any further set calls a no-op.
> > > 
> > > This change is preparation for allowing the user to override the
> > > panel-orientation for any connector from the kernel commandline.
> > > When the panel-orientation is overridden this way, then we must ignore
> > > the panel-orientation detection done by the driver.
> > > 
> > > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > >   drivers/gpu/drm/drm_connector.c         | 74 ++++++++++++++++++-------
> > >   drivers/gpu/drm/i915/display/icl_dsi.c  |  5 +-
> > >   drivers/gpu/drm/i915/display/intel_dp.c |  9 ++-
> > >   drivers/gpu/drm/i915/display/vlv_dsi.c  |  5 +-
> > >   include/drm/drm_connector.h             |  9 ++-
> > >   5 files changed, 71 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > index 0965632008a9..f4fa5c59717d 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -1139,7 +1139,8 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
> > >    *	coordinates, so if userspace rotates the picture to adjust for
> > >    *	the orientation it must also apply the same transformation to the
> > >    *	touchscreen input coordinates. This property is initialized by calling
> > > - *	drm_connector_init_panel_orientation_property().
> > > + *	drm_connector_set_panel_orientation() or
> > > + *	drm_connector_set_panel_orientation_with_quirk()
> > 
> > do we have a better name than quirks for these dsi modes?
> 
> The difference between the 2 functions is that the second one calls
> drm_get_panel_orientation_quirk() and that if that returns a valid
> orientation it overwrites the passed orientation with the return value
> from drm_get_panel_orientation_quirk(), so the name seems correct.
> 
> As for drm_get_panel_orientation_quirk() itself that currently is only
> defined on x86 (it is a static inline no-op elsewhere) and it used
> DMI string matching to check for a model specific quirk. So again the
> name seems correct.
> 
> > >    *
> > >    * scaling mode:
> > >    *	This property defines how a non-native mode is upscaled to the native
> > > @@ -2046,38 +2047,41 @@ void drm_connector_set_vrr_capable_property(
> > >   EXPORT_SYMBOL(drm_connector_set_vrr_capable_property);
> > >   /**
> > > - * drm_connector_init_panel_orientation_property -
> > > - *	initialize the connecters panel_orientation property
> > > - * @connector: connector for which to init the panel-orientation property.
> > > - * @width: width in pixels of the panel, used for panel quirk detection
> > > - * @height: height in pixels of the panel, used for panel quirk detection
> > > + * drm_connector_set_panel_orientation - sets the connecter's panel_orientation
> > > + * @connector: connector for which to set the panel-orientation property.
> > > + * @panel_orientation: drm_panel_orientation value to set
> > > + *
> > > + * This function sets the connector's panel_orientation and attaches
> > > + * a "panel orientation" property to the connector.
> > >    *
> > > - * This function should only be called for built-in panels, after setting
> > > - * connector->display_info.panel_orientation first (if known).
> > > + * Calling this function on a connector where the panel_orientation has
> > > + * already been set is a no-op (e.g. the orientation has been overridden with
> > > + * a kernel commandline option).
> > >    *
> > > - * This function will check for platform specific (e.g. DMI based) quirks
> > > - * overriding display_info.panel_orientation first, then if panel_orientation
> > > - * is not DRM_MODE_PANEL_ORIENTATION_UNKNOWN it will attach the
> > > - * "panel orientation" property to the connector.
> > > + * It is allowed to call this function with a panel_orientation of
> > > + * DRM_MODE_PANEL_ORIENTATION_UNKNOWN, in which case it is a no-op.
> > >    *
> > >    * Returns:
> > >    * Zero on success, negative errno on failure.
> > >    */
> > > -int drm_connector_init_panel_orientation_property(
> > > -	struct drm_connector *connector, int width, int height)
> > > +int drm_connector_set_panel_orientation(
> > > +	struct drm_connector *connector,
> > > +	enum drm_panel_orientation panel_orientation)
> > >   {
> > >   	struct drm_device *dev = connector->dev;
> > >   	struct drm_display_info *info = &connector->display_info;
> > >   	struct drm_property *prop;
> > > -	int orientation_quirk;
> > > -	orientation_quirk = drm_get_panel_orientation_quirk(width, height);
> > > -	if (orientation_quirk != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
> > > -		info->panel_orientation = orientation_quirk;
> > > +	/* Already set? */
> > > +	if (info->panel_orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
> > > +		return 0;
> > 
> > What happens on the scenario of ICL DSI here?
> > In case we had something badly set we just respect the bad choices?
> 
> The only path where the value gets set twice is when it has been set
> from the kernel commandline (which gets hooked up in the second patch),
> and yes in that case we honor what the user passed in as value, honering
> the user passed value (which gets processed first) is the whole reason
> for the "Already set?" check.
> 
> > Any way to at least have some kind of warn when we tried the dsi mode but
> > it had already been set?
> 
> The 2nd patch prints a message when a value is passed on the kernel commandline:
> 
> +	if (mode->panel_orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN) {
> +		DRM_INFO("setting connector %s panel_orientation to %d\n",
> +			 connector->name, mode->panel_orientation);

Maybe we could be more explicit there that we are "forcing" and orientation?

But anyway, that's for the other patch, for this one

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> 
> Regards,
> 
> Hans
> 
> 
> 
> > > -	if (info->panel_orientation == DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
> > > +	/* Don't attach the property if the orientation is unknown */
> > > +	if (panel_orientation == DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
> > >   		return 0;
> > > +	info->panel_orientation = panel_orientation;
> > > +
> > >   	prop = dev->mode_config.panel_orientation_property;
> > >   	if (!prop) {
> > >   		prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE,
> > > @@ -2094,7 +2098,37 @@ int drm_connector_init_panel_orientation_property(
> > >   				   info->panel_orientation);
> > >   	return 0;
> > >   }
> > > -EXPORT_SYMBOL(drm_connector_init_panel_orientation_property);
> > > +EXPORT_SYMBOL(drm_connector_set_panel_orientation);
> > > +
> > > +/**
> > > + * drm_connector_set_panel_orientation_with_quirk -
> > > + *	set the connecter's panel_orientation after checking for quirks
> > > + * @connector: connector for which to init the panel-orientation property.
> > > + * @panel_orientation: drm_panel_orientation value to set
> > > + * @width: width in pixels of the panel, used for panel quirk detection
> > > + * @height: height in pixels of the panel, used for panel quirk detection
> > > + *
> > > + * Like drm_connector_set_panel_orientation(), but with a check for platform
> > > + * specific (e.g. DMI based) quirks overriding the passed in panel_orientation.
> > > + *
> > > + * Returns:
> > > + * Zero on success, negative errno on failure.
> > > + */
> > > +int drm_connector_set_panel_orientation_with_quirk(
> > > +	struct drm_connector *connector,
> > > +	enum drm_panel_orientation panel_orientation,
> > > +	int width, int height)
> > > +{
> > > +	int orientation_quirk;
> > > +
> > > +	orientation_quirk = drm_get_panel_orientation_quirk(width, height);
> > > +	if (orientation_quirk != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
> > > +		panel_orientation = orientation_quirk;
> > > +
> > > +	return drm_connector_set_panel_orientation(connector,
> > > +						   panel_orientation);
> > > +}
> > > +EXPORT_SYMBOL(drm_connector_set_panel_orientation_with_quirk);
> > >   int drm_connector_set_obj_prop(struct drm_mode_object *obj,
> > >   				    struct drm_property *property,
> > > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c
> > > index 6e398c33a524..8cd51cf67d02 100644
> > > --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> > > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> > > @@ -1536,9 +1536,8 @@ static void icl_dsi_add_properties(struct intel_connector *connector)
> > >   	connector->base.state->scaling_mode = DRM_MODE_SCALE_ASPECT;
> > > -	connector->base.display_info.panel_orientation =
> > > -			intel_dsi_get_panel_orientation(connector);
> > > -	drm_connector_init_panel_orientation_property(&connector->base,
> > > +	drm_connector_set_panel_orientation_with_quirk(&connector->base,
> > > +				intel_dsi_get_panel_orientation(connector),
> > >   				connector->panel.fixed_mode->hdisplay,
> > >   				connector->panel.fixed_mode->vdisplay);
> > >   }
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index aa515261cb9f..9f4425f8d0ac 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -7340,9 +7340,12 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> > >   	intel_connector->panel.backlight.power = intel_edp_backlight_power;
> > >   	intel_panel_setup_backlight(connector, pipe);
> > > -	if (fixed_mode)
> > > -		drm_connector_init_panel_orientation_property(
> > > -			connector, fixed_mode->hdisplay, fixed_mode->vdisplay);
> > > +	if (fixed_mode) {
> > > +		/* We do not know the orientation, but their might be a quirk */
> > > +		drm_connector_set_panel_orientation_with_quirk(connector,
> > > +				DRM_MODE_PANEL_ORIENTATION_UNKNOWN,
> > > +				fixed_mode->hdisplay, fixed_mode->vdisplay);
> > > +	}
> > >   	return true;
> > > diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c
> > > index 50064cde0724..a0de8e70e426 100644
> > > --- a/drivers/gpu/drm/i915/display/vlv_dsi.c
> > > +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
> > > @@ -1632,10 +1632,9 @@ static void vlv_dsi_add_properties(struct intel_connector *connector)
> > >   		connector->base.state->scaling_mode = DRM_MODE_SCALE_ASPECT;
> > > -		connector->base.display_info.panel_orientation =
> > > -			vlv_dsi_get_panel_orientation(connector);
> > > -		drm_connector_init_panel_orientation_property(
> > > +		drm_connector_set_panel_orientation_with_quirk(
> > >   				&connector->base,
> > > +				vlv_dsi_get_panel_orientation(connector),
> > >   				connector->panel.fixed_mode->hdisplay,
> > >   				connector->panel.fixed_mode->vdisplay);
> > >   	}
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index 221910948b37..2113500b4075 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -1552,8 +1552,13 @@ void drm_connector_set_link_status_property(struct drm_connector *connector,
> > >   					    uint64_t link_status);
> > >   void drm_connector_set_vrr_capable_property(
> > >   		struct drm_connector *connector, bool capable);
> > > -int drm_connector_init_panel_orientation_property(
> > > -	struct drm_connector *connector, int width, int height);
> > > +int drm_connector_set_panel_orientation(
> > > +	struct drm_connector *connector,
> > > +	enum drm_panel_orientation panel_orientation);
> > > +int drm_connector_set_panel_orientation_with_quirk(
> > > +	struct drm_connector *connector,
> > > +	enum drm_panel_orientation panel_orientation,
> > > +	int width, int height);
> > >   int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
> > >   					  int min, int max);
> > > -- 
> > > 2.23.0
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel