On Wed, Jul 19, 2017 at 3:16 AM, Jose Abreu Jose.Abreu@synopsys.com wrote:
Hi John,
On 18-07-2017 18:59, John Stultz wrote:
Currently the hikey dsi logic cannot generate accurate byte clocks values for all pixel clock values. Thus if a mode clock is selected that cannot match the calculated byte clock, the device will boot with a blank screen.
This patch uses the new mode_valid callback (many thanks to Jose Abreu for upstreaming it!) to ensure we don't select modes we cannot generate.
Also, since the ade crtc code will adjust the mode in mode_set, this patch also adds a mode_fixup callback which we use to make sure we are validating the mode clock that will eventually be used.
Many thanks to Jose and Daniel for recent feedback. I think this version is looking much nicer. But I'd still welcome any feedback or suggestions!
Cc: Daniel Vetter daniel.vetter@intel.com Cc: Jani Nikula jani.nikula@linux.intel.com Cc: Sean Paul seanpaul@chromium.org Cc: David Airlie airlied@linux.ie Cc: Rob Clark robdclark@gmail.com Cc: Xinliang Liu xinliang.liu@linaro.org Cc: Xinliang Liu z.liuxinliang@hisilicon.com Cc: Rongrong Zou zourongrong@gmail.com Cc: Xinwei Kong kong.kongxinwei@hisilicon.com Cc: Chen Feng puck.chen@hisilicon.com Cc: Jose Abreu Jose.Abreu@synopsys.com Cc: Archit Taneja architt@codeaurora.org Cc: dri-devel@lists.freedesktop.org Signed-off-by: John Stultz john.stultz@linaro.org
v2: Reworked to calculate if modeclock matches the phy's byteclock, rather then using a whitelist of known modes.
v3: Reworked to check across all possible crtcs (even though for us there is only one), and use mode_fixup instead of a custom function, as suggested by Jose and Daniel.
drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 62 +++++++++++++++++++++++++ drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 14 ++++++ 2 files changed, 76 insertions(+)
diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c index f77dcfa..d7b5820 100644 --- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c +++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c @@ -603,6 +603,67 @@ static void dsi_encoder_enable(struct drm_encoder *encoder) dsi->enable = true; }
+static enum drm_mode_status dsi_encoder_phy_mode_valid(struct drm_encoder *encoder,
const struct drm_display_mode *mode)+{
struct dw_dsi *dsi = encoder_to_dsi(encoder);struct mipi_phy_params phy;u32 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);u32 req_kHz, act_kHz, lane_byte_clk_kHz;/* Calculate the lane byte clk using the adjusted mode clk */memset(&phy, 0, sizeof(phy));req_kHz = mode->clock * bpp / dsi->lanes;act_kHz = dsi_calc_phy_rate(req_kHz, &phy);lane_byte_clk_kHz = act_kHz / 8;DRM_DEBUG_DRIVER("Checking mode %ix%i-%i@%i clock: %i...",mode->hdisplay, mode->vdisplay, bpp,drm_mode_vrefresh(mode), mode->clock);/** Make sure the adjused mode clock and the lane byte clk* have a common denominator base frequency*/if (mode->clock/dsi->lanes == lane_byte_clk_kHz/3) {DRM_DEBUG_DRIVER("OK!\n");return MODE_OK;}DRM_DEBUG_DRIVER("BAD!\n");return MODE_BAD;+}
+static enum drm_mode_status dsi_encoder_mode_valid(struct drm_encoder *encoder,
const struct drm_display_mode *mode)+{
const struct drm_crtc_helper_funcs *crtc_funcs = NULL;struct drm_crtc *crtc = NULL;struct drm_display_mode adj_mode;int ret;This int should be an enum drm_mode_status ...
memcpy(&adj_mode, mode, sizeof(adj_mode));Maybe you should move this to the loop so that you pass a "clean" adjusted mode (i.e. adj_mode == mode) for each crtc. You could also use drm_mode_duplicate()/drm_mode_destroy() ...
Ah! Good catch! Thanks for that!
What is the benefit of drm_mode_duplicate over just using memcpy? I see there's some base.id and head pointers that are kept unique, but we're not touching those for this case. The extra allocating/freeing seems a bit needless.
/** The crtc might adjust the mode, so go through the* possible crtcs (technically just one) and call* mode_fixup to figure out the adjusted mode before we* validate it.*/drm_for_each_crtc(crtc, encoder->dev) {crtc_funcs = crtc->helper_private;if (crtc_funcs && crtc_funcs->mode_fixup)ret = crtc_funcs->mode_fixup(crtc, mode,&adj_mode);No return check?
Hrm. Actually, not sure what to do if mode_fixup failed there. I guess print a warning and validate the unadjusted mode? Other suggestions?
thanks -john