Hi, Just commenting on the mode_fixup function that was added in v7. On Tue, Feb 25, 2020 at 2:15 PM Xin Ji <xji@xxxxxxxxxxxxxxxx> wrote: > > The ANX7625 is an ultra-low power 4K Mobile HD Transmitter designed > for portable device. It converts MIPI DSI/DPI to DisplayPort 1.3 4K. > > The ANX7625 can support both USB Type-C PD feature and MIPI DSI/DPI > to DP feature. This driver only enabled MIPI DSI/DPI to DP feature. > > Signed-off-by: Xin Ji <xji@xxxxxxxxxxxxxxxx> > --- > drivers/gpu/drm/bridge/Makefile | 2 +- > drivers/gpu/drm/bridge/analogix/Kconfig | 6 + > drivers/gpu/drm/bridge/analogix/Makefile | 1 + > drivers/gpu/drm/bridge/analogix/anx7625.c | 2172 +++++++++++++++++++++++++++++ > drivers/gpu/drm/bridge/analogix/anx7625.h | 410 ++++++ > 5 files changed, 2590 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.c > create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.h > > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > index 4934fcf..bcd388a 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile [snip] > +static bool anx7625_bridge_mode_fixup(struct drm_bridge *bridge, > + const struct drm_display_mode *mode, > + struct drm_display_mode *adj) > +{ > + struct anx7625_data *ctx = bridge_to_anx7625(bridge); > + struct device *dev = &ctx->client->dev; > + u32 hsync, hfp, hbp, hactive, hblanking; > + u32 adj_hsync, adj_hfp, adj_hbp, adj_hblanking, delta_adj; > + u32 vref, adj_clock; > + > + DRM_DEV_DEBUG_DRIVER(dev, "drm mode fixup set\n"); > + > + mutex_lock(&ctx->lock); Why do you need this lock? > + > + hactive = mode->hdisplay; This is never used, drop it? > + hsync = mode->hsync_end - mode->hsync_start; > + hfp = mode->hsync_start - mode->hdisplay; > + hbp = mode->htotal - mode->hsync_end; > + hblanking = mode->htotal - mode->hdisplay; > + > + DRM_DEV_DEBUG_DRIVER(dev, "before mode fixup\n"); > + DRM_DEV_DEBUG_DRIVER(dev, "hsync(%d),hfp(%d),hbp(%d),clock(%d)\n", > + hsync, > + hfp, > + hbp, > + adj->clock); > + DRM_DEV_DEBUG_DRIVER(dev, "hsync_start(%d),hsync_end(%d),htotal(%d)\n", > + adj->hsync_start, > + adj->hsync_end, > + adj->htotal); > + > + adj_hfp = hfp; > + adj_hsync = hsync; > + adj_hbp = hbp; > + adj_hblanking = hblanking; > + > + /* plus 1 if hfp is odd */ A better way to word these comments is to say "hfp needs to be even", otherwise, you're just repeating what we can already see in the code. > + if (hfp & 0x1) { > + adj_hfp = hfp + 1; adj_hfp -= 1 for consistency? > + adj_hblanking += 1; > + } > + > + /* minus 1 if hbp is odd */ > + if (hbp & 0x1) { > + adj_hbp = hbp - 1; ditto, adj_hbp -= 1; > + adj_hblanking -= 1; > + } > + > + /* plus 1 if hsync is odd */ > + if (hsync & 0x1) { > + if (adj_hblanking < hblanking) > + adj_hsync = hsync + 1; ditto > + else > + adj_hsync = hsync - 1; ditto > + } > + > + /* > + * once illegal timing detected, use default HFP, HSYNC, HBP > + */ > + if (hblanking < HBLANKING_MIN || (hfp < HP_MIN && hbp < HP_MIN)) { should this be adj_hblanking/adj_hfp/adj_hbp? > + adj_hsync = SYNC_LEN_DEF; > + adj_hfp = HFP_HBP_DEF; > + adj_hbp = HFP_HBP_DEF; > + vref = adj->clock * 1000 / (adj->htotal * adj->vtotal); > + if (hblanking < HBLANKING_MIN) { > + delta_adj = HBLANKING_MIN - hblanking; > + adj_clock = vref * delta_adj * adj->vtotal; > + adj->clock += DIV_ROUND_UP(adj_clock, 1000); > + } else { > + delta_adj = hblanking - HBLANKING_MIN; > + adj_clock = vref * delta_adj * adj->vtotal; > + adj->clock -= DIV_ROUND_UP(adj_clock, 1000); > + } > + > + DRM_WARN("illegal hblanking timing, use default.\n"); > + DRM_WARN("hfp(%d),hbp(%d),hsync(%d).\n", hfp, hbp, hsync); How likely is it that this mode is going to work? Can you just return false here to reject the mode? > + } else if (adj_hfp < HP_MIN) { > + /* adjust hfp if hfp less than HP_MIN */ > + delta_adj = HP_MIN - adj_hfp; > + adj_hfp = HP_MIN; > + > + /* > + * balance total HBlanking pixel, if HBP hasn't enough space, "does not have enough space" > + * adjust HSYNC length, otherwize adjust HBP otherwise > + */ > + if ((adj_hbp - delta_adj) < HP_MIN) > + /* hbp not enough space */ > + adj_hsync -= delta_adj; > + else > + adj_hbp -= delta_adj; > + } else if (adj_hbp < HP_MIN) { > + delta_adj = HP_MIN - adj_hbp; > + adj_hbp = HP_MIN; > + > + /* > + * balance total HBlanking pixel, if HBP hasn't enough space, > + * adjust HSYNC length, otherwize adjust HBP > + */ > + if ((adj_hfp - delta_adj) < HP_MIN) > + /* hbp not enough space */ > + adj_hsync -= delta_adj; > + else > + adj_hfp -= delta_adj; > + } > + > + DRM_DEV_DEBUG_DRIVER(dev, "after mode fixup\n"); > + DRM_DEV_DEBUG_DRIVER(dev, "hsync(%d),hfp(%d),hbp(%d),clock(%d)\n", Add spaces after commas in your debug strings (same above and below). > + adj_hsync, > + adj_hfp, > + adj_hbp, > + adj->clock); Put these 4 on a single line. > + > + /* reconstruct timing */ > + adj->hsync_start = adj->hdisplay + adj_hfp; > + adj->hsync_end = adj->hsync_start + adj_hsync; > + adj->htotal = adj->hsync_end + adj_hbp; > + DRM_DEV_DEBUG_DRIVER(dev, "hsync_start(%d),hsync_end(%d),htotal(%d)\n", > + adj->hsync_start, > + adj->hsync_end, > + adj->htotal); > + > + mutex_unlock(&ctx->lock); > + > + return true; > +} > + > [snip] _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel