Re: [PATCH v7 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver
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 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.
> http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [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
> > > > > > > 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
Re: [PATCH v7 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver
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
> > > > > > 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
Maybe add a comment to the code summarizing the discussion. Definitely
needs to be covered in the commit message.
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH v7 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver
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 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
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver
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 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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver
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 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.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver
On Fri, Apr 24, 2020 at 08:12:04PM +0800, Nicolas Boichat wrote:
> On Fri, Apr 24, 2020 at 2:51 PM Xin Ji 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.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver
On Fri, Apr 24, 2020 at 2:51 PM Xin Ji 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.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver
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.
>
> On Tue, Feb 25, 2020 at 2:15 PM Xin Ji 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
> > ---
> > 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 = >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(>lock);
>
> Why do you need this lock?
Seems no need this lock, I'll remove it.
>
> > +
> > + hactive = mode->hdisplay;
>
> This is never used, drop it?
OK, I'll 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.
OK
>
> > + if (hfp & 0x1) {
> > + adj_hfp = hfp + 1;
>
> adj_hfp -= 1 for consistency?
OK
>
> > + adj_hblanking += 1;
> > + }
> > +
> > + /* minus 1 if hbp is odd */
> > + if (hbp & 0x1) {
> > + adj_hbp = hbp - 1;
>
> ditto, adj_hbp -= 1;
OK
>
> > + adj_hblanking -= 1;
> > + }
> > +
> > + /* plus 1 if hsync is odd */
> > + if (hsync & 0x1) {
> > + if (adj_hblanking < hblanking)
> > + adj_hsync = hsync + 1;
>
> ditto
OK
>
> > + else
> > + adj_hsync = hsync - 1;
>
> ditto
OK
>
> > + }
> > +
> > + /*
> > +* 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");
> > +
Re: [PATCH v7 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver
Hi,
Just commenting on the mode_fixup function that was added in v7.
On Tue, Feb 25, 2020 at 2:15 PM Xin Ji 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
> ---
> 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 = >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(>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
