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@xxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxx>
> > > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxx>
>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
