Hi, On 2023-03-27 at 22:20:45 +0200, Maxime Ripard <maxime@xxxxxxxxxx> wrote: > Hi, > > On Sat, Mar 25, 2023 at 12:40:04PM +0100, Frank Oltmanns wrote: [...] >> Actually, I had the following third patch prepared that adjusted the dotclock rate so that the >> required PLL rate is set. But again, this seems very indirect, so that’s why I refrained from >> submitting it and I submitted the linked patch above instead. >> >> Anyway, here is the third proposal: >> >> — a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c >> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c >> @@ -819,6 +819,34 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder) >> regulator_disable(dsi->regulator); >> } >> >> +static bool sun6i_dsi_encoder_mode_fixup( >> ⁃ struct drm_encoder *encoder, >> ⁃ const struct drm_display_mode *mode, >> ⁃ struct drm_display_mode *adjusted_mode) >> +{ >> ⁃ if (encoder->encoder_type == DRM_MODE_ENCODER_DSI) { >> ⁃ /* >> ⁃ * For DSI the PLL rate has to respect the bits per pixel and >> ⁃ * number of lanes. >> ⁃ * >> ⁃ * According to the BSP code: >> ⁃ * PLL rate = DOTCLOCK * bpp / lanes >> ⁃ * >> ⁃ * Therefore, the clock has to be adjusted in order to set the >> ⁃ * correct PLL rate when actually setting the clock. >> ⁃ */ >> ⁃ struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder); >> ⁃ struct mipi_dsi_device *device = dsi->device; >> ⁃ u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format); >> ⁃ u8 lanes = device->lanes; >> ⁃ >> >> ⁃ adjusted_mode->crtc_clock = mode->crtc_clock >> ⁃ * bpp / (lanes * SUN6I_DSI_TCON_DIV); >> ⁃ } >> ⁃ >> >> ⁃ return true; >> +} >> ⁃ static int sun6i_dsi_get_modes(struct drm_connector *connector) >> { >> struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector); >> @@ -851,6 +879,7 @@ static const struct drm_connector_funcs sun6i_dsi_connector_funcs = { >> static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = { >> .disable = sun6i_dsi_encoder_disable, >> .enable = sun6i_dsi_encoder_enable, >> ⁃ .mode_fixup = sun6i_dsi_encoder_mode_fixup, >> }; > > It's not clear to me what this patch is supposed to be doing, there's no mode_fixup implementation > upstream? > Sorry, my mail client tried some fancy formatting. :( This is the patch again. --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -819,6 +819,34 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder) regulator_disable(dsi->regulator); } +static bool sun6i_dsi_encoder_mode_fixup( + struct drm_encoder *encoder, + const struct drm_display_mode *mode, + struct drm_display_mode *adjusted_mode) +{ + if (encoder->encoder_type == DRM_MODE_ENCODER_DSI) { + /* + * For DSI the PLL rate has to respect the bits per pixel and + * number of lanes. + * + * According to the BSP code: + * PLL rate = DOTCLOCK * bpp / lanes + * + * Therefore, the clock has to be adjusted in order to set the + * correct PLL rate when actually setting the clock. + */ + struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder); + struct mipi_dsi_device *device = dsi->device; + u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format); + u8 lanes = device->lanes; + + adjusted_mode->crtc_clock = mode->crtc_clock + * bpp / (lanes * SUN6I_DSI_TCON_DIV); + } + + return true; +} + static int sun6i_dsi_get_modes(struct drm_connector *connector) { struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector); @@ -851,6 +879,7 @@ static const struct drm_connector_funcs sun6i_dsi_connector_funcs = { static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = { .disable = sun6i_dsi_encoder_disable, .enable = sun6i_dsi_encoder_enable, + .mode_fixup = sun6i_dsi_encoder_mode_fixup, }; static u32 sun6i_dsi_dcs_build_pkt_hdr(struct sun6i_dsi *dsi, I still like the original patch better, but I'd be happy to submit this as a proper patch, if this is more to your liking. Thanks, Frank > Maxime > --