lkml.org 
[lkml]   [2020]   [Apr]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v7 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver
    On Thu, Apr 30, 2020 at 03:54:38PM +0200, Daniel Vetter wrote:
    > On Thu, Apr 30, 2020 at 09:47:46PM +0800, Xin Ji wrote:
    > > Hi Daniel,
    > >
    > > On Thu, Apr 30, 2020 at 03:38:39PM +0200, Daniel Vetter wrote:
    > > > On Thu, Apr 30, 2020 at 03:37:31PM +0200, Daniel Vetter wrote:
    > > > > 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@analogixsemi.com> 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.
    > > >
    > > > Ok I read the patch now, you seem to support both. Would it work if we
    > > > make this adjustement conditional on it being an internal panel only? I
    > > > think that would be perfect.
    > > > -Daniel
    > > > --
    > > > Daniel Vetter
    > > > Software Engineer, Intel Corporation
    > > Based on comments of V8, only keeped eDP built-in panel in V9 version,
    > > removed external DP screen support.
    >
    > Ah even better. Then the above adjusting has my:
    >
    > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
    >
    > Maybe add a comment to the code summarizing the discussion. Definitely
    > needs to be covered in the commit message.
    OK, I'll add it in the next serial.

    Thanks, Xin
    >
    > Thanks, Daniel
    > --
    > Daniel Vetter
    > Software Engineer, Intel Corporation
    > http://blog.ffwll.ch

    \
     
     \ /
      Last update: 2020-04-30 16:01    [W:3.011 / U:0.408 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site