Re: [PATCH v2 3/4] drm/i915: Add "panel orientation" property to the panel connector

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 02-10-17 10:06, Daniel Vetter wrote:
> On Sun, Oct 01, 2017 at 05:33:16PM +0200, Hans de Goede wrote:
> > Ideally we could use the VBT for this, that would be simple, in
> > intel_dsi_init() check dev_priv->vbt.dsi.config->rotation, set
> > connector->display_info.panel_orientation accordingly and call
> > drm_connector_init_panel_orientation_property(), done.
> >
> > Unfortunately vbt.dsi.config->rotation is always 0 even on tablets
> > with an upside down LCD and where the GOP is properly rotating the
> > EFI fb in hardware.
> >
> > So instead we end up reading the rotation from the primary plane,
> > which is a bit more complicated.
> >
> > The code to read back the rotation from the hardware is based on an
> > earlier attempt to fix fdo#94894 by Ville Syrjala.
> >
> > Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=94894
> > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> > Suggested-by: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>
> This patch looks a bit like it tries to maximize layering violations:
>
> - store panel state in plane_state
> - digg a hole from global code to set up panel information
>
> Can't we do this in a better way? I'm thinking of something mimicking the
> fixed mode readout, which is also not driven in a backwards way like this.
> I.e. a small helper in intel_panel.c which reads out the rotation of the
> primary panel and hopes for the best.

Ok, I've reworked this in a way which will hopefully be much more to
your liking. This is now pretty much all isolated to intel_dsi.c

> Plus ofc taking the quirk list into account.
>
> Also, how exactly does the GOP figure out we need to rotate?

I wish I knew, the logical answer would be the rotation field of
struct mipi_config (which is part of the VBT) but at least no the
tablet with upside-down LCD panel I've using that field is not the
answer, hence the code to read it back from the hw at boot.

Regards,

Hans

> > ---
> > Changes in v2:
> > -Rebased on 4.14-rc1
> > -Readback primary plane rotation from the hardware and use that to
> >   set the panel orientation
> > -This (readback) fixes fdo#94894, add Fixes: tag
> > ---
> >   drivers/gpu/drm/i915/i915_drv.h      |  3 +++
> >   drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++++++++++-
> >   drivers/gpu/drm/i915/intel_drv.h     |  5 +++++
> >   drivers/gpu/drm/i915/intel_panel.c   | 33 ++++++++++++++++++++++++++++++
> >   4 files changed, 79 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 18d9da53282b..c4c8590972b4 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2151,6 +2151,8 @@ struct intel_cdclk_state {
> >   	unsigned int cdclk, vco, ref;
> >   };
> >   
> > +struct intel_panel;
> > +
> >   struct drm_i915_private {
> >   	struct drm_device drm;
> >   
> > @@ -2200,6 +2202,7 @@ struct drm_i915_private {
> >   	struct i915_gem_context *kernel_context;
> >   	struct intel_engine_cs *engine[I915_NUM_ENGINES];
> >   	struct i915_vma *semaphore;
> > +	struct intel_panel *panel;
> >   
> >   	struct drm_dma_handle *status_page_dmah;
> >   	struct resource mch_res;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 00cd17c76fdc..fbd61f92b60d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2891,6 +2891,9 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> >   		return;
> >   	}
> >   
> > +	intel_panel_set_orientation_from_crtc(dev_priv->panel, intel_crtc,
> > +					      plane_config->panel_orientation);
> > +
> >   	plane_state->src_x = 0;
> >   	plane_state->src_y = 0;
> >   	plane_state->src_w = fb->width << 16;
> > @@ -7523,6 +7526,10 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
> >   			plane_config->tiling = I915_TILING_X;
> >   			fb->modifier = I915_FORMAT_MOD_X_TILED;
> >   		}
> > +
> > +		if (val & DISPPLANE_ROTATE_180)
> > +			plane_config->panel_orientation =
> > +				DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP;
> >   	}
> >   
> >   	pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
> > @@ -8576,6 +8583,24 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
> >   		goto error;
> >   	}
> >   
> > +	/* The rotation is used to *correct* for the panel orientation */
> > +	switch (val & PLANE_CTL_ROTATE_MASK) {
> > +	case PLANE_CTL_ROTATE_0:
> > +		break;
> > +	case PLANE_CTL_ROTATE_90:
> > +		plane_config->panel_orientation =
> > +			DRM_MODE_PANEL_ORIENTATION_RIGHT_UP;
> > +		break;
> > +	case PLANE_CTL_ROTATE_180:
> > +		plane_config->panel_orientation =
> > +			DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP;
> > +		break;
> > +	case PLANE_CTL_ROTATE_270:
> > +		plane_config->panel_orientation =
> > +			DRM_MODE_PANEL_ORIENTATION_LEFT_UP;
> > +		break;
> > +	}
> > +
> >   	base = I915_READ(PLANE_SURF(pipe, 0)) & 0xfffff000;
> >   	plane_config->base = base;
> >   
> > @@ -8661,6 +8686,10 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
> >   			plane_config->tiling = I915_TILING_X;
> >   			fb->modifier = I915_FORMAT_MOD_X_TILED;
> >   		}
> > +
> > +		if (val & DISPPLANE_ROTATE_180)
> > +			plane_config->panel_orientation =
> > +				DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP;
> >   	}
> >   
> >   	pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
> > @@ -14578,7 +14607,9 @@ int intel_modeset_init(struct drm_device *dev)
> >   	drm_modeset_unlock_all(dev);
> >   
> >   	for_each_intel_crtc(dev, crtc) {
> > -		struct intel_initial_plane_config plane_config = {};
> > +		struct intel_initial_plane_config plane_config = {
> > +			.panel_orientation = DRM_MODE_PANEL_ORIENTATION_NORMAL,
> > +		};
> >   
> >   		if (!crtc->active)
> >   			continue;
> > @@ -14600,6 +14631,12 @@ int intel_modeset_init(struct drm_device *dev)
> >   		intel_find_initial_plane_obj(crtc, &plane_config);
> >   	}
> >   
> > +	/*
> > +	 * Init panel orientation prop now that intel_find_initial_plane_obj()
> > +	 * has had a chance to set panel orientation.
> > +	 */
> > +	intel_panel_init_orientation_prop(dev_priv->panel);
> > +
> >   	/*
> >   	 * Make sure hardware watermarks really match the state we read out.
> >   	 * Note that we need to do this after reconstructing the BIOS fb's
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index fa47285918f4..50fa0eeca655 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -447,6 +447,7 @@ struct intel_initial_plane_config {
> >   	unsigned int tiling;
> >   	int size;
> >   	u32 base;
> > +	int panel_orientation; /* DRM_MODE_PANEL_ORIENTATION_* */
> >   };
> >   
> >   #define SKL_MIN_SRC_W 8
> > @@ -1703,6 +1704,10 @@ extern struct drm_display_mode *intel_find_panel_downclock(
> >   				struct drm_i915_private *dev_priv,
> >   				struct drm_display_mode *fixed_mode,
> >   				struct drm_connector *connector);
> > +void intel_panel_set_orientation_from_crtc(struct intel_panel *panel,
> > +					   struct intel_crtc *intel_crtc,
> > +					   int orientation);
> > +void intel_panel_init_orientation_prop(struct intel_panel *panel);
> >   
> >   #if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
> >   int intel_backlight_device_register(struct intel_connector *connector);
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index 0f7942a5dccf..fa7dfb9ac5f0 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -1925,11 +1925,44 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel)
> >   	}
> >   }
> >   
> > +void intel_panel_set_orientation_from_crtc(struct intel_panel *panel,
> > +					   struct intel_crtc *intel_crtc,
> > +					   int orientation)
> > +{
> > +	struct intel_connector *panel_conn;
> > +
> > +	if (!panel)
> > +		return;
> > +
> > +	panel_conn = container_of(panel, struct intel_connector, panel);
> > +	if (panel_conn->base.state->crtc == &intel_crtc->base)
> > +		panel_conn->base.display_info.panel_orientation = orientation;
> > +}
> > +
> > +void intel_panel_init_orientation_prop(struct intel_panel *panel)
> > +{
> > +	struct intel_connector *panel_conn;
> > +
> > +	if (!panel || !panel->fixed_mode)
> > +		return;
> > +
> > +	panel_conn = container_of(panel, struct intel_connector, panel);
> > +	drm_connector_init_panel_orientation_property(&panel_conn->base,
> > +		panel->fixed_mode->hdisplay, panel->fixed_mode->vdisplay);
> > +}
> > +
> >   int intel_panel_init(struct intel_panel *panel,
> >   		     struct drm_display_mode *fixed_mode,
> >   		     struct drm_display_mode *alt_fixed_mode,
> >   		     struct drm_display_mode *downclock_mode)
> >   {
> > +	struct intel_connector *intel_connector =
> > +		container_of(panel, struct intel_connector, panel);
> > +	struct drm_i915_private *dev_priv = to_i915(intel_connector->base.dev);
> > +
> > +	if (!dev_priv->panel)
> > +		dev_priv->panel = panel;
> > +
> >   	intel_panel_init_backlight_funcs(panel);
> >   
> >   	panel->fixed_mode = fixed_mode;
> > --
> > 2.14.2
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel