| Message ID | 1499719682-31750-1-git-send-email-john.stultz@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 10, 2017 at 01:48:02PM -0700, John Stultz wrote: > Currently the hikey dsi logic cannot generate accurate byte > clocks values for all pixel clock values. Thus if a mode clock > is selected that cannot match the calculated byte clock, the > device will boot with a blank screen. > > This patch uses the new mode_valid callback (many thanks to > Jose Abreu for upstreaming it!) to enforces known good mode > clocks for well known resolutions, which should allow the > display to work from given EDID options, and ensures for a given > resolution & refresh, the right mode clock is selected. > > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Sean Paul <seanpaul@chromium.org> > Cc: David Airlie <airlied@linux.ie> > Cc: Rob Clark <robdclark@gmail.com> > Cc: Xinliang Liu <xinliang.liu@linaro.org> > Cc: Xinliang Liu <z.liuxinliang@hisilicon.com> > Cc: Rongrong Zou <zourongrong@gmail.com> > Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com> > Cc: Chen Feng <puck.chen@hisilicon.com> > Cc: Jose Abreu <Jose.Abreu@synopsys.com> > Cc: Archit Taneja <architt@codeaurora.org> > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 37 ++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c > index f77dcfa..a84f4bb 100644 > --- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c > +++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c > @@ -603,6 +603,42 @@ static void dsi_encoder_enable(struct drm_encoder *encoder) > dsi->enable = true; > } > > +static enum drm_mode_status dsi_encoder_mode_valid(struct drm_encoder *crtc, > + const struct drm_display_mode *mode) > +{ > + /* > + * kirin cannot generate all modes, so use the whitelist below > + */ > + DRM_DEBUG("%s: Checking mode %ix%i@%i clock: %i...", __func__, > + mode->hdisplay, mode->vdisplay, drm_mode_vrefresh(mode), mode->clock); > + if ((mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 148500) || > + (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 80192) || > + (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 74250) || > + (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 61855) || > + (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 147116) || > + (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 146250) || > + (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 144589) || > + (mode->hdisplay == 1600 && mode->vdisplay == 1200 && mode->clock == 160961) || > + (mode->hdisplay == 1600 && mode->vdisplay == 900 && mode->clock == 118963) || > + (mode->hdisplay == 1440 && mode->vdisplay == 900 && mode->clock == 126991) || > + (mode->hdisplay == 1280 && mode->vdisplay == 1024 && mode->clock == 128946) || > + (mode->hdisplay == 1280 && mode->vdisplay == 1024 && mode->clock == 98619) || > + (mode->hdisplay == 1280 && mode->vdisplay == 960 && mode->clock == 102081) || > + (mode->hdisplay == 1280 && mode->vdisplay == 800 && mode->clock == 83496) || > + (mode->hdisplay == 1280 && mode->vdisplay == 720 && mode->clock == 74440) || > + (mode->hdisplay == 1280 && mode->vdisplay == 720 && mode->clock == 74250) || > + (mode->hdisplay == 1024 && mode->vdisplay == 768 && mode->clock == 78800) || > + (mode->hdisplay == 1024 && mode->vdisplay == 768 && mode->clock == 75000) || > + (mode->hdisplay == 1024 && mode->vdisplay == 768 && mode->clock == 81833) || > + (mode->hdisplay == 800 && mode->vdisplay == 600 && mode->clock == 48907) || > + (mode->hdisplay == 800 && mode->vdisplay == 600 && mode->clock == 40000)) { Bikeshed incoming: Can you break this out into a lookup table? It's kind of long-winded as-is. It'd be even better if you could calculate whether the mode is valid, but I didn't spend enough time to figure out if this is possible. Sean > + DRM_DEBUG("OK\n"); > + return MODE_OK; > + } > + DRM_DEBUG("BAD\n"); > + return MODE_BAD; > +} > + > static void dsi_encoder_mode_set(struct drm_encoder *encoder, > struct drm_display_mode *mode, > struct drm_display_mode *adj_mode) > @@ -622,6 +658,7 @@ static int dsi_encoder_atomic_check(struct drm_encoder *encoder, > > static const struct drm_encoder_helper_funcs dw_encoder_helper_funcs = { > .atomic_check = dsi_encoder_atomic_check, > + .mode_valid = dsi_encoder_mode_valid, > .mode_set = dsi_encoder_mode_set, > .enable = dsi_encoder_enable, > .disable = dsi_encoder_disable > -- > 2.7.4 >
On Mon, Jul 10, 2017 at 2:18 PM, Sean Paul <seanpaul@chromium.org> wrote: > On Mon, Jul 10, 2017 at 01:48:02PM -0700, John Stultz wrote: >> Currently the hikey dsi logic cannot generate accurate byte >> clocks values for all pixel clock values. Thus if a mode clock >> is selected that cannot match the calculated byte clock, the >> device will boot with a blank screen. >> >> This patch uses the new mode_valid callback (many thanks to >> Jose Abreu for upstreaming it!) to enforces known good mode >> clocks for well known resolutions, which should allow the >> display to work from given EDID options, and ensures for a given >> resolution & refresh, the right mode clock is selected. >> >> Cc: Daniel Vetter <daniel.vetter@intel.com> >> Cc: Jani Nikula <jani.nikula@linux.intel.com> >> Cc: Sean Paul <seanpaul@chromium.org> >> Cc: David Airlie <airlied@linux.ie> >> Cc: Rob Clark <robdclark@gmail.com> >> Cc: Xinliang Liu <xinliang.liu@linaro.org> >> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com> >> Cc: Rongrong Zou <zourongrong@gmail.com> >> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com> >> Cc: Chen Feng <puck.chen@hisilicon.com> >> Cc: Jose Abreu <Jose.Abreu@synopsys.com> >> Cc: Archit Taneja <architt@codeaurora.org> >> Cc: dri-devel@lists.freedesktop.org >> Signed-off-by: John Stultz <john.stultz@linaro.org> >> --- >> drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 37 ++++++++++++++++++++++++++++ >> 1 file changed, 37 insertions(+) >> >> diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c >> index f77dcfa..a84f4bb 100644 >> --- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c >> +++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c >> @@ -603,6 +603,42 @@ static void dsi_encoder_enable(struct drm_encoder *encoder) >> dsi->enable = true; >> } >> >> +static enum drm_mode_status dsi_encoder_mode_valid(struct drm_encoder *crtc, >> + const struct drm_display_mode *mode) >> +{ >> + /* >> + * kirin cannot generate all modes, so use the whitelist below >> + */ >> + DRM_DEBUG("%s: Checking mode %ix%i@%i clock: %i...", __func__, >> + mode->hdisplay, mode->vdisplay, drm_mode_vrefresh(mode), mode->clock); >> + if ((mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 148500) || >> + (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 80192) || >> + (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 74250) || >> + (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 61855) || >> + (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 147116) || >> + (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 146250) || >> + (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 144589) || >> + (mode->hdisplay == 1600 && mode->vdisplay == 1200 && mode->clock == 160961) || >> + (mode->hdisplay == 1600 && mode->vdisplay == 900 && mode->clock == 118963) || >> + (mode->hdisplay == 1440 && mode->vdisplay == 900 && mode->clock == 126991) || >> + (mode->hdisplay == 1280 && mode->vdisplay == 1024 && mode->clock == 128946) || >> + (mode->hdisplay == 1280 && mode->vdisplay == 1024 && mode->clock == 98619) || >> + (mode->hdisplay == 1280 && mode->vdisplay == 960 && mode->clock == 102081) || >> + (mode->hdisplay == 1280 && mode->vdisplay == 800 && mode->clock == 83496) || >> + (mode->hdisplay == 1280 && mode->vdisplay == 720 && mode->clock == 74440) || >> + (mode->hdisplay == 1280 && mode->vdisplay == 720 && mode->clock == 74250) || >> + (mode->hdisplay == 1024 && mode->vdisplay == 768 && mode->clock == 78800) || >> + (mode->hdisplay == 1024 && mode->vdisplay == 768 && mode->clock == 75000) || >> + (mode->hdisplay == 1024 && mode->vdisplay == 768 && mode->clock == 81833) || >> + (mode->hdisplay == 800 && mode->vdisplay == 600 && mode->clock == 48907) || >> + (mode->hdisplay == 800 && mode->vdisplay == 600 && mode->clock == 40000)) { > > Bikeshed incoming: > > Can you break this out into a lookup table? It's kind of long-winded as-is. It'd That's fair. The list has slowly grown from 4 or so modes to what it is now, so it is a bit longer then it was originally. I'll spin the patches with that change. > be even better if you could calculate whether the mode is valid, but I didn't > spend enough time to figure out if this is possible. Theoretically that might be possible, checking if the requested freq matches the calculated freq, and I've tried that but so far I've not been able to get it to work, as in some cases the freq on the current whitelist don't exactly match but do work on the large majority of monitors tested (while other freq have a similar error but don't work on most monitors). I'd like to spend some more time to try to refine and tune this, but having the current whitelist works fairly well, so I'm not sure its worth sitting on (this is basically the last functional patch outstanding for HiKey to fully work upstream - except the mali gpu of course), while I try to tinker and tune it. Thanks so much for the feedback! -john
On Mon, Jul 10, 2017 at 03:47:54PM -0700, John Stultz wrote: > On Mon, Jul 10, 2017 at 2:18 PM, Sean Paul <seanpaul@chromium.org> wrote: > > On Mon, Jul 10, 2017 at 01:48:02PM -0700, John Stultz wrote: > >> Currently the hikey dsi logic cannot generate accurate byte > >> clocks values for all pixel clock values. Thus if a mode clock > >> is selected that cannot match the calculated byte clock, the > >> device will boot with a blank screen. > >> > >> This patch uses the new mode_valid callback (many thanks to > >> Jose Abreu for upstreaming it!) to enforces known good mode > >> clocks for well known resolutions, which should allow the > >> display to work from given EDID options, and ensures for a given > >> resolution & refresh, the right mode clock is selected. > >> > >> Cc: Daniel Vetter <daniel.vetter@intel.com> > >> Cc: Jani Nikula <jani.nikula@linux.intel.com> > >> Cc: Sean Paul <seanpaul@chromium.org> > >> Cc: David Airlie <airlied@linux.ie> > >> Cc: Rob Clark <robdclark@gmail.com> > >> Cc: Xinliang Liu <xinliang.liu@linaro.org> > >> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com> > >> Cc: Rongrong Zou <zourongrong@gmail.com> > >> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com> > >> Cc: Chen Feng <puck.chen@hisilicon.com> > >> Cc: Jose Abreu <Jose.Abreu@synopsys.com> > >> Cc: Archit Taneja <architt@codeaurora.org> > >> Cc: dri-devel@lists.freedesktop.org > >> Signed-off-by: John Stultz <john.stultz@linaro.org> > >> --- > >> drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 37 ++++++++++++++++++++++++++++ > >> 1 file changed, 37 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c > >> index f77dcfa..a84f4bb 100644 > >> --- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c > >> +++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c > >> @@ -603,6 +603,42 @@ static void dsi_encoder_enable(struct drm_encoder *encoder) > >> dsi->enable = true; > >> } > >> > >> +static enum drm_mode_status dsi_encoder_mode_valid(struct drm_encoder *crtc, > >> + const struct drm_display_mode *mode) > >> +{ > >> + /* > >> + * kirin cannot generate all modes, so use the whitelist below > >> + */ > >> + DRM_DEBUG("%s: Checking mode %ix%i@%i clock: %i...", __func__, > >> + mode->hdisplay, mode->vdisplay, drm_mode_vrefresh(mode), mode->clock); > >> + if ((mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 148500) || > >> + (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 80192) || > >> + (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 74250) || > >> + (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 61855) || > >> + (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 147116) || > >> + (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 146250) || > >> + (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 144589) || > >> + (mode->hdisplay == 1600 && mode->vdisplay == 1200 && mode->clock == 160961) || > >> + (mode->hdisplay == 1600 && mode->vdisplay == 900 && mode->clock == 118963) || > >> + (mode->hdisplay == 1440 && mode->vdisplay == 900 && mode->clock == 126991) || > >> + (mode->hdisplay == 1280 && mode->vdisplay == 1024 && mode->clock == 128946) || > >> + (mode->hdisplay == 1280 && mode->vdisplay == 1024 && mode->clock == 98619) || > >> + (mode->hdisplay == 1280 && mode->vdisplay == 960 && mode->clock == 102081) || > >> + (mode->hdisplay == 1280 && mode->vdisplay == 800 && mode->clock == 83496) || > >> + (mode->hdisplay == 1280 && mode->vdisplay == 720 && mode->clock == 74440) || > >> + (mode->hdisplay == 1280 && mode->vdisplay == 720 && mode->clock == 74250) || > >> + (mode->hdisplay == 1024 && mode->vdisplay == 768 && mode->clock == 78800) || > >> + (mode->hdisplay == 1024 && mode->vdisplay == 768 && mode->clock == 75000) || > >> + (mode->hdisplay == 1024 && mode->vdisplay == 768 && mode->clock == 81833) || > >> + (mode->hdisplay == 800 && mode->vdisplay == 600 && mode->clock == 48907) || > >> + (mode->hdisplay == 800 && mode->vdisplay == 600 && mode->clock == 40000)) { > > > > Bikeshed incoming: > > > > Can you break this out into a lookup table? It's kind of long-winded as-is. It'd > > That's fair. The list has slowly grown from 4 or so modes to what it > is now, so it is a bit longer then it was originally. > > I'll spin the patches with that change. > > > be even better if you could calculate whether the mode is valid, but I didn't > > spend enough time to figure out if this is possible. > > Theoretically that might be possible, checking if the requested freq > matches the calculated freq, and I've tried that but so far I've not > been able to get it to work, as in some cases the freq on the current > whitelist don't exactly match but do work on the large majority of > monitors tested (while other freq have a similar error but don't work > on most monitors). > > I'd like to spend some more time to try to refine and tune this, but > having the current whitelist works fairly well, so I'm not sure its > worth sitting on (this is basically the last functional patch > outstanding for HiKey to fully work upstream - except the mali gpu of > course), while I try to tinker and tune it. > > Thanks so much for the feedback! Yeah the proper approach is to compute your pll/clock settings and bail out if those don't work. That's generally a magic spreadsheet supplied by the hw validation engineers, and I honestly don't want to know how they create it. Explicit modelist in the kernel sounds like a very bad hack. -Daniel
On Tue, Jul 11, 2017 at 12:56 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Jul 10, 2017 at 03:47:54PM -0700, John Stultz wrote: >> On Mon, Jul 10, 2017 at 2:18 PM, Sean Paul <seanpaul@chromium.org> wrote: >> > On Mon, Jul 10, 2017 at 01:48:02PM -0700, John Stultz wrote: >> >> Currently the hikey dsi logic cannot generate accurate byte >> >> clocks values for all pixel clock values. Thus if a mode clock >> >> is selected that cannot match the calculated byte clock, the >> >> device will boot with a blank screen. >> >> >> >> This patch uses the new mode_valid callback (many thanks to >> >> Jose Abreu for upstreaming it!) to enforces known good mode >> >> clocks for well known resolutions, which should allow the >> >> display to work from given EDID options, and ensures for a given >> >> resolution & refresh, the right mode clock is selected. >> >> >> >> Cc: Daniel Vetter <daniel.vetter@intel.com> >> >> Cc: Jani Nikula <jani.nikula@linux.intel.com> >> >> Cc: Sean Paul <seanpaul@chromium.org> >> >> Cc: David Airlie <airlied@linux.ie> >> >> Cc: Rob Clark <robdclark@gmail.com> >> >> Cc: Xinliang Liu <xinliang.liu@linaro.org> >> >> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com> >> >> Cc: Rongrong Zou <zourongrong@gmail.com> >> >> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com> >> >> Cc: Chen Feng <puck.chen@hisilicon.com> >> >> Cc: Jose Abreu <Jose.Abreu@synopsys.com> >> >> Cc: Archit Taneja <architt@codeaurora.org> >> >> Cc: dri-devel@lists.freedesktop.org >> >> Signed-off-by: John Stultz <john.stultz@linaro.org> >> >> --- >> >> drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 37 ++++++++++++++++++++++++++++ >> >> 1 file changed, 37 insertions(+) >> >> >> >> diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c >> >> index f77dcfa..a84f4bb 100644 >> >> --- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c >> >> +++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c >> >> @@ -603,6 +603,42 @@ static void dsi_encoder_enable(struct drm_encoder *encoder) >> >> dsi->enable = true; >> >> } >> >> >> >> +static enum drm_mode_status dsi_encoder_mode_valid(struct drm_encoder *crtc, >> >> + const struct drm_display_mode *mode) >> >> +{ >> >> + /* >> >> + * kirin cannot generate all modes, so use the whitelist below >> >> + */ >> >> + DRM_DEBUG("%s: Checking mode %ix%i@%i clock: %i...", __func__, >> >> + mode->hdisplay, mode->vdisplay, drm_mode_vrefresh(mode), mode->clock); >> >> + if ((mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 148500) || >> >> + (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 80192) || >> >> + (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 74250) || >> >> + (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 61855) || >> >> + (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 147116) || >> >> + (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 146250) || >> >> + (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 144589) || >> >> + (mode->hdisplay == 1600 && mode->vdisplay == 1200 && mode->clock == 160961) || >> >> + (mode->hdisplay == 1600 && mode->vdisplay == 900 && mode->clock == 118963) || >> >> + (mode->hdisplay == 1440 && mode->vdisplay == 900 && mode->clock == 126991) || >> >> + (mode->hdisplay == 1280 && mode->vdisplay == 1024 && mode->clock == 128946) || >> >> + (mode->hdisplay == 1280 && mode->vdisplay == 1024 && mode->clock == 98619) || >> >> + (mode->hdisplay == 1280 && mode->vdisplay == 960 && mode->clock == 102081) || >> >> + (mode->hdisplay == 1280 && mode->vdisplay == 800 && mode->clock == 83496) || >> >> + (mode->hdisplay == 1280 && mode->vdisplay == 720 && mode->clock == 74440) || >> >> + (mode->hdisplay == 1280 && mode->vdisplay == 720 && mode->clock == 74250) || >> >> + (mode->hdisplay == 1024 && mode->vdisplay == 768 && mode->clock == 78800) || >> >> + (mode->hdisplay == 1024 && mode->vdisplay == 768 && mode->clock == 75000) || >> >> + (mode->hdisplay == 1024 && mode->vdisplay == 768 && mode->clock == 81833) || >> >> + (mode->hdisplay == 800 && mode->vdisplay == 600 && mode->clock == 48907) || >> >> + (mode->hdisplay == 800 && mode->vdisplay == 600 && mode->clock == 40000)) { >> > >> > Bikeshed incoming: >> > >> > Can you break this out into a lookup table? It's kind of long-winded as-is. It'd >> >> That's fair. The list has slowly grown from 4 or so modes to what it >> is now, so it is a bit longer then it was originally. >> >> I'll spin the patches with that change. >> >> > be even better if you could calculate whether the mode is valid, but I didn't >> > spend enough time to figure out if this is possible. >> >> Theoretically that might be possible, checking if the requested freq >> matches the calculated freq, and I've tried that but so far I've not >> been able to get it to work, as in some cases the freq on the current >> whitelist don't exactly match but do work on the large majority of >> monitors tested (while other freq have a similar error but don't work >> on most monitors). >> >> I'd like to spend some more time to try to refine and tune this, but >> having the current whitelist works fairly well, so I'm not sure its >> worth sitting on (this is basically the last functional patch >> outstanding for HiKey to fully work upstream - except the mali gpu of >> course), while I try to tinker and tune it. >> >> Thanks so much for the feedback! > > Yeah the proper approach is to compute your pll/clock settings and bail > out if those don't work. That's generally a magic spreadsheet supplied by > the hw validation engineers, and I honestly don't want to know how they > create it. Explicit modelist in the kernel sounds like a very bad hack. So without such a magic spreadsheet, how would you suggest I move this forward? Not having the whitelist hack and picking modes the device can't generate is a fairly major usability issue. thanks -john
On Tue, Jul 11, 2017 at 5:05 PM, John Stultz <john.stultz@linaro.org> wrote: >>> > be even better if you could calculate whether the mode is valid, but I didn't >>> > spend enough time to figure out if this is possible. >>> >>> Theoretically that might be possible, checking if the requested freq >>> matches the calculated freq, and I've tried that but so far I've not >>> been able to get it to work, as in some cases the freq on the current >>> whitelist don't exactly match but do work on the large majority of >>> monitors tested (while other freq have a similar error but don't work >>> on most monitors). >>> >>> I'd like to spend some more time to try to refine and tune this, but >>> having the current whitelist works fairly well, so I'm not sure its >>> worth sitting on (this is basically the last functional patch >>> outstanding for HiKey to fully work upstream - except the mali gpu of >>> course), while I try to tinker and tune it. >>> >>> Thanks so much for the feedback! >> >> Yeah the proper approach is to compute your pll/clock settings and bail >> out if those don't work. That's generally a magic spreadsheet supplied by >> the hw validation engineers, and I honestly don't want to know how they >> create it. Explicit modelist in the kernel sounds like a very bad hack. > > So without such a magic spreadsheet, how would you suggest I move this forward? > Not having the whitelist hack and picking modes the device can't > generate is a fairly major usability issue. I guess if the whitelist is the only thing I'd do 2 things differently: - Whitelist the clocks, not modes, since that's what seems to matter here. - Put it as close as possible to the code that comuptes the clock settings (yet if you use the clock subsystem that's a bit hard, but for an atomic driver this should be where this is done ...). Whitelist of modes looks super-hacky. -Daniel
On Tue, Jul 11, 2017 at 8:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Jul 11, 2017 at 5:05 PM, John Stultz <john.stultz@linaro.org> wrote: >>>> > be even better if you could calculate whether the mode is valid, but I didn't >>>> > spend enough time to figure out if this is possible. >>>> >>>> Theoretically that might be possible, checking if the requested freq >>>> matches the calculated freq, and I've tried that but so far I've not >>>> been able to get it to work, as in some cases the freq on the current >>>> whitelist don't exactly match but do work on the large majority of >>>> monitors tested (while other freq have a similar error but don't work >>>> on most monitors). >>>> >>>> I'd like to spend some more time to try to refine and tune this, but >>>> having the current whitelist works fairly well, so I'm not sure its >>>> worth sitting on (this is basically the last functional patch >>>> outstanding for HiKey to fully work upstream - except the mali gpu of >>>> course), while I try to tinker and tune it. >>>> >>>> Thanks so much for the feedback! >>> >>> Yeah the proper approach is to compute your pll/clock settings and bail >>> out if those don't work. That's generally a magic spreadsheet supplied by >>> the hw validation engineers, and I honestly don't want to know how they >>> create it. Explicit modelist in the kernel sounds like a very bad hack. >> >> So without such a magic spreadsheet, how would you suggest I move this forward? >> Not having the whitelist hack and picking modes the device can't >> generate is a fairly major usability issue. > > I guess if the whitelist is the only thing I'd do 2 things differently: > - Whitelist the clocks, not modes, since that's what seems to matter here. > - Put it as close as possible to the code that comuptes the clock > settings (yet if you use the clock subsystem that's a bit hard, but > for an atomic driver this should be where this is done ...). > > Whitelist of modes looks super-hacky. Sure. The whitelist modes were easiest to use initially dealing with problem reports since the EDID numbers were what users could report working or not. But this feedback sounds reasonable, as I can map those to the underlying pixel clocks and generate a whitelist of those. I really appreciate the feedback here! thanks -john
On Tue, Jul 11, 2017 at 5:44 PM, John Stultz <john.stultz@linaro.org> wrote: > On Tue, Jul 11, 2017 at 8:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Tue, Jul 11, 2017 at 5:05 PM, John Stultz <john.stultz@linaro.org> wrote: >>>>> > be even better if you could calculate whether the mode is valid, but I didn't >>>>> > spend enough time to figure out if this is possible. >>>>> >>>>> Theoretically that might be possible, checking if the requested freq >>>>> matches the calculated freq, and I've tried that but so far I've not >>>>> been able to get it to work, as in some cases the freq on the current >>>>> whitelist don't exactly match but do work on the large majority of >>>>> monitors tested (while other freq have a similar error but don't work >>>>> on most monitors). >>>>> >>>>> I'd like to spend some more time to try to refine and tune this, but >>>>> having the current whitelist works fairly well, so I'm not sure its >>>>> worth sitting on (this is basically the last functional patch >>>>> outstanding for HiKey to fully work upstream - except the mali gpu of >>>>> course), while I try to tinker and tune it. >>>>> >>>>> Thanks so much for the feedback! >>>> >>>> Yeah the proper approach is to compute your pll/clock settings and bail >>>> out if those don't work. That's generally a magic spreadsheet supplied by >>>> the hw validation engineers, and I honestly don't want to know how they >>>> create it. Explicit modelist in the kernel sounds like a very bad hack. >>> >>> So without such a magic spreadsheet, how would you suggest I move this forward? >>> Not having the whitelist hack and picking modes the device can't >>> generate is a fairly major usability issue. >> >> I guess if the whitelist is the only thing I'd do 2 things differently: >> - Whitelist the clocks, not modes, since that's what seems to matter here. >> - Put it as close as possible to the code that comuptes the clock >> settings (yet if you use the clock subsystem that's a bit hard, but >> for an atomic driver this should be where this is done ...). >> >> Whitelist of modes looks super-hacky. > > Sure. The whitelist modes were easiest to use initially dealing with > problem reports since the EDID numbers were what users could report > working or not. But this feedback sounds reasonable, as I can map > those to the underlying pixel clocks and generate a whitelist of > those. > > I really appreciate the feedback here! Another one: If you put this into the encoders ->mode_valid it will be enforced both when listing modes, and when trying to set a mode. Which means your users won't be able to see unsupported modes nor try them out. But it's not really a hard hw limit, just our current best guess, and so makes testing new modes unecessarily complicated. If you instead move this into the connectors ->mode_valid, then it's only used to validate the connectors mode list, but not at modeset time. Which means users could still test modes manually added to xrandr and then tell you what modes to add. We do that e.g. for sink mode limits, because some sinks have buggy EDIDs and report wrong limits. Users can then still set modes at their own risk, and be happy when they work. -Daniel
On Tue, Jul 11, 2017 at 9:27 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Jul 11, 2017 at 5:44 PM, John Stultz <john.stultz@linaro.org> wrote: >> On Tue, Jul 11, 2017 at 8:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >>> On Tue, Jul 11, 2017 at 5:05 PM, John Stultz <john.stultz@linaro.org> wrote: >>>>>> > be even better if you could calculate whether the mode is valid, but I didn't >>>>>> > spend enough time to figure out if this is possible. >>>>>> >>>>>> Theoretically that might be possible, checking if the requested freq >>>>>> matches the calculated freq, and I've tried that but so far I've not >>>>>> been able to get it to work, as in some cases the freq on the current >>>>>> whitelist don't exactly match but do work on the large majority of >>>>>> monitors tested (while other freq have a similar error but don't work >>>>>> on most monitors). >>>>>> >>>>>> I'd like to spend some more time to try to refine and tune this, but >>>>>> having the current whitelist works fairly well, so I'm not sure its >>>>>> worth sitting on (this is basically the last functional patch >>>>>> outstanding for HiKey to fully work upstream - except the mali gpu of >>>>>> course), while I try to tinker and tune it. >>>>>> >>>>>> Thanks so much for the feedback! >>>>> >>>>> Yeah the proper approach is to compute your pll/clock settings and bail >>>>> out if those don't work. That's generally a magic spreadsheet supplied by >>>>> the hw validation engineers, and I honestly don't want to know how they >>>>> create it. Explicit modelist in the kernel sounds like a very bad hack. >>>> >>>> So without such a magic spreadsheet, how would you suggest I move this forward? >>>> Not having the whitelist hack and picking modes the device can't >>>> generate is a fairly major usability issue. >>> >>> I guess if the whitelist is the only thing I'd do 2 things differently: >>> - Whitelist the clocks, not modes, since that's what seems to matter here. >>> - Put it as close as possible to the code that comuptes the clock >>> settings (yet if you use the clock subsystem that's a bit hard, but >>> for an atomic driver this should be where this is done ...). >>> >>> Whitelist of modes looks super-hacky. >> >> Sure. The whitelist modes were easiest to use initially dealing with >> problem reports since the EDID numbers were what users could report >> working or not. But this feedback sounds reasonable, as I can map >> those to the underlying pixel clocks and generate a whitelist of >> those. >> >> I really appreciate the feedback here! > > Another one: If you put this into the encoders ->mode_valid it will be > enforced both when listing modes, and when trying to set a mode. Which > means your users won't be able to see unsupported modes nor try them > out. > > But it's not really a hard hw limit, just our current best guess, and > so makes testing new modes unecessarily complicated. > > If you instead move this into the connectors ->mode_valid, then it's > only used to validate the connectors mode list, but not at modeset > time. Which means users could still test modes manually added to > xrandr and then tell you what modes to add. > > We do that e.g. for sink mode limits, because some sinks have buggy > EDIDs and report wrong limits. Users can then still set modes at their > own risk, and be happy when they work. So got some time to tinker here, and I've got two issues I'm not sure how to move on. 1) The kirin driver doesn't seem to have a connector (just encoder/crtc on the kirin side), the connector seems to be on the adv7511 bridge, which isn't the component that has the mode restrictions. So I'm not sure how to push the mode_valid check into the connector. 2) In trying to move away from the whitelist, the kirin encoder is where we calculate the phy byte clock which we want to match (depending on the lanes used, by a fraction of) the mode clock. However, the kirin crtc logic tweaks the adj_mode at fixup/set time. This means the mode->clock we check against in the encoder mode_valid ends up not being the mode we actually try to use at encoder mode_set time. Am I just missing something? Do we need to run the modes through the pipeline's mode_fixups before checking its mode_valids? Or should the encoder mode_valid be asking the crtc to fix up the modes before testing? thanks -john
On Mon, Jul 17, 2017 at 04:20:23PM -0700, John Stultz wrote: > On Tue, Jul 11, 2017 at 9:27 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Tue, Jul 11, 2017 at 5:44 PM, John Stultz <john.stultz@linaro.org> wrote: > >> On Tue, Jul 11, 2017 at 8:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > >>> On Tue, Jul 11, 2017 at 5:05 PM, John Stultz <john.stultz@linaro.org> wrote: > >>>>>> > be even better if you could calculate whether the mode is valid, but I didn't > >>>>>> > spend enough time to figure out if this is possible. > >>>>>> > >>>>>> Theoretically that might be possible, checking if the requested freq > >>>>>> matches the calculated freq, and I've tried that but so far I've not > >>>>>> been able to get it to work, as in some cases the freq on the current > >>>>>> whitelist don't exactly match but do work on the large majority of > >>>>>> monitors tested (while other freq have a similar error but don't work > >>>>>> on most monitors). > >>>>>> > >>>>>> I'd like to spend some more time to try to refine and tune this, but > >>>>>> having the current whitelist works fairly well, so I'm not sure its > >>>>>> worth sitting on (this is basically the last functional patch > >>>>>> outstanding for HiKey to fully work upstream - except the mali gpu of > >>>>>> course), while I try to tinker and tune it. > >>>>>> > >>>>>> Thanks so much for the feedback! > >>>>> > >>>>> Yeah the proper approach is to compute your pll/clock settings and bail > >>>>> out if those don't work. That's generally a magic spreadsheet supplied by > >>>>> the hw validation engineers, and I honestly don't want to know how they > >>>>> create it. Explicit modelist in the kernel sounds like a very bad hack. > >>>> > >>>> So without such a magic spreadsheet, how would you suggest I move this forward? > >>>> Not having the whitelist hack and picking modes the device can't > >>>> generate is a fairly major usability issue. > >>> > >>> I guess if the whitelist is the only thing I'd do 2 things differently: > >>> - Whitelist the clocks, not modes, since that's what seems to matter here. > >>> - Put it as close as possible to the code that comuptes the clock > >>> settings (yet if you use the clock subsystem that's a bit hard, but > >>> for an atomic driver this should be where this is done ...). > >>> > >>> Whitelist of modes looks super-hacky. > >> > >> Sure. The whitelist modes were easiest to use initially dealing with > >> problem reports since the EDID numbers were what users could report > >> working or not. But this feedback sounds reasonable, as I can map > >> those to the underlying pixel clocks and generate a whitelist of > >> those. > >> > >> I really appreciate the feedback here! > > > > Another one: If you put this into the encoders ->mode_valid it will be > > enforced both when listing modes, and when trying to set a mode. Which > > means your users won't be able to see unsupported modes nor try them > > out. > > > > But it's not really a hard hw limit, just our current best guess, and > > so makes testing new modes unecessarily complicated. > > > > If you instead move this into the connectors ->mode_valid, then it's > > only used to validate the connectors mode list, but not at modeset > > time. Which means users could still test modes manually added to > > xrandr and then tell you what modes to add. > > > > We do that e.g. for sink mode limits, because some sinks have buggy > > EDIDs and report wrong limits. Users can then still set modes at their > > own risk, and be happy when they work. > > So got some time to tinker here, and I've got two issues I'm not sure > how to move on. > > 1) The kirin driver doesn't seem to have a connector (just > encoder/crtc on the kirin side), the connector seems to be on the > adv7511 bridge, which isn't the component that has the mode > restrictions. So I'm not sure how to push the mode_valid check into > the connector. It was just an idea to make debugging easier. And avoid an endless stream of regression reports to keep you busy :-) > 2) In trying to move away from the whitelist, the kirin encoder is > where we calculate the phy byte clock which we want to match > (depending on the lanes used, by a fraction of) the mode clock. > However, the kirin crtc logic tweaks the adj_mode at fixup/set time. > This means the mode->clock we check against in the encoder mode_valid > ends up not being the mode we actually try to use at encoder mode_set > time. > > Am I just missing something? Do we need to run the modes through the > pipeline's mode_fixups before checking its mode_valids? Or should the > encoder mode_valid be asking the crtc to fix up the modes before > testing? mode_valid was kinda meant for simple limit stuff like max/min. If you first need to round stuff to something the hw can do, then whitelist it, it gets more tricky. You might just need to hand-roll stuff here, I don't think this can be reasonably resolved with just helper infrastructure. Hand-roll = create a dummy adjusted_mode and call the mode_fixup stuff directly. -Daniel
diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c index f77dcfa..a84f4bb 100644 --- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c +++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c @@ -603,6 +603,42 @@ static void dsi_encoder_enable(struct drm_encoder *encoder) dsi->enable = true; } +static enum drm_mode_status dsi_encoder_mode_valid(struct drm_encoder *crtc, + const struct drm_display_mode *mode) +{ + /* + * kirin cannot generate all modes, so use the whitelist below + */ + DRM_DEBUG("%s: Checking mode %ix%i@%i clock: %i...", __func__, + mode->hdisplay, mode->vdisplay, drm_mode_vrefresh(mode), mode->clock); + if ((mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 148500) || + (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 80192) || + (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 74250) || + (mode->hdisplay == 1920 && mode->vdisplay == 1080 && mode->clock == 61855) || + (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 147116) || + (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 146250) || + (mode->hdisplay == 1680 && mode->vdisplay == 1050 && mode->clock == 144589) || + (mode->hdisplay == 1600 && mode->vdisplay == 1200 && mode->clock == 160961) || + (mode->hdisplay == 1600 && mode->vdisplay == 900 && mode->clock == 118963) || + (mode->hdisplay == 1440 && mode->vdisplay == 900 && mode->clock == 126991) || + (mode->hdisplay == 1280 && mode->vdisplay == 1024 && mode->clock == 128946) || + (mode->hdisplay == 1280 && mode->vdisplay == 1024 && mode->clock == 98619) || + (mode->hdisplay == 1280 && mode->vdisplay == 960 && mode->clock == 102081) || + (mode->hdisplay == 1280 && mode->vdisplay == 800 && mode->clock == 83496) || + (mode->hdisplay == 1280 && mode->vdisplay == 720 && mode->clock == 74440) || + (mode->hdisplay == 1280 && mode->vdisplay == 720 && mode->clock == 74250) || + (mode->hdisplay == 1024 && mode->vdisplay == 768 && mode->clock == 78800) || + (mode->hdisplay == 1024 && mode->vdisplay == 768 && mode->clock == 75000) || + (mode->hdisplay == 1024 && mode->vdisplay == 768 && mode->clock == 81833) || + (mode->hdisplay == 800 && mode->vdisplay == 600 && mode->clock == 48907) || + (mode->hdisplay == 800 && mode->vdisplay == 600 && mode->clock == 40000)) { + DRM_DEBUG("OK\n"); + return MODE_OK; + } + DRM_DEBUG("BAD\n"); + return MODE_BAD; +} + static void dsi_encoder_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode, struct drm_display_mode *adj_mode) @@ -622,6 +658,7 @@ static int dsi_encoder_atomic_check(struct drm_encoder *encoder, static const struct drm_encoder_helper_funcs dw_encoder_helper_funcs = { .atomic_check = dsi_encoder_atomic_check, + .mode_valid = dsi_encoder_mode_valid, .mode_set = dsi_encoder_mode_set, .enable = dsi_encoder_enable, .disable = dsi_encoder_disable
Currently the hikey dsi logic cannot generate accurate byte clocks values for all pixel clock values. Thus if a mode clock is selected that cannot match the calculated byte clock, the device will boot with a blank screen. This patch uses the new mode_valid callback (many thanks to Jose Abreu for upstreaming it!) to enforces known good mode clocks for well known resolutions, which should allow the display to work from given EDID options, and ensures for a given resolution & refresh, the right mode clock is selected. Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Sean Paul <seanpaul@chromium.org> Cc: David Airlie <airlied@linux.ie> Cc: Rob Clark <robdclark@gmail.com> Cc: Xinliang Liu <xinliang.liu@linaro.org> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com> Cc: Rongrong Zou <zourongrong@gmail.com> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com> Cc: Chen Feng <puck.chen@hisilicon.com> Cc: Jose Abreu <Jose.Abreu@synopsys.com> Cc: Archit Taneja <architt@codeaurora.org> Cc: dri-devel@lists.freedesktop.org Signed-off-by: John Stultz <john.stultz@linaro.org> --- drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 37 ++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)