Re: [PATCH v7 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver

[Daniel Vetter]

On Thu, Apr 30, 2020 at 11:36:14AM +0800, Xin Ji wrote:
> On Tue, Apr 28, 2020 at 12:05:08PM +0200, Daniel Vetter wrote:
> > On Fri, Apr 24, 2020 at 08:12:04PM +0800, Nicolas Boichat wrote:
> > > On Fri, Apr 24, 2020 at 2:51 PM Xin Ji <xji@xxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Thu, Apr 23, 2020 at 07:55:15PM +0800, Nicolas Boichat wrote:
> > > > > Hi,
> > > > >
> > > > > Just commenting on the mode_fixup function that was added in v7.
> > > > >
> > > > [snip]
> > > > > > +       /*
> > > > > > +        * 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?
> > > > NO, need check original HFP and HBP, if they are not legal, driver need
> > > > set default value to adj_hsync, 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?
> > > > We want to set the default minimal Hblancking value, then it may display,
> > > > otherwise. If we just return false, there is no display for sure.
> > > 
> > > Right, understand your argument. I'm pondering if it's not just better
> > > to reject the mode rather than trying a timing that is definitely
> > > quite different from what the monitor was asking for. No super strong
> > > opinion, I'll let other people on the list weigh in.
> > 
> > Yeah mode_fixup is supposed to be used to adjust the mode in intermediate
> > stages (e.g. if you go from progressive to interlaced only at the end of
> > your pipeline or something like that). It's not meant for adjusting the
> > mode yout actually put out through a hdmi or dp connector. For fixed
> > panels adjusting modes to fit the panel is also fairly common, but not for
> > external outputs.
> > 
> > Since this is a DP bridge I'd say no adjusting, just reject what doesn't
> > fit.
> We have found some panel which HBP less than 8, if we reject to adjust
> video timing, then there is no display. The customer does not accept it,
> they push us to fix it, the only resolve way is to adjust timing.

Are we talking about external DP screen here, or some built-in panel? For
the later case we do a lot of mode adjusting in many drivers ...

I haven't checked, by if our connector type is eDP then this should be all
fine.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch