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 <[email protected]>
Cc: Jani Nikula <[email protected]>
Cc: Sean Paul <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Xinliang Liu <[email protected]>
Cc: Xinliang Liu <[email protected]>
Cc: Rongrong Zou <[email protected]>
Cc: Xinwei Kong <[email protected]>
Cc: Chen Feng <[email protected]>
Cc: Jose Abreu <[email protected]>
Cc: Archit Taneja <[email protected]>
Cc: [email protected]
Signed-off-by: John Stultz <[email protected]>
---
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)) {
+ 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 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 <[email protected]>
> Cc: Jani Nikula <[email protected]>
> Cc: Sean Paul <[email protected]>
> Cc: David Airlie <[email protected]>
> Cc: Rob Clark <[email protected]>
> Cc: Xinliang Liu <[email protected]>
> Cc: Xinliang Liu <[email protected]>
> Cc: Rongrong Zou <[email protected]>
> Cc: Xinwei Kong <[email protected]>
> Cc: Chen Feng <[email protected]>
> Cc: Jose Abreu <[email protected]>
> Cc: Archit Taneja <[email protected]>
> Cc: [email protected]
> Signed-off-by: John Stultz <[email protected]>
> ---
> 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
>
--
Sean Paul, Software Engineer, Google / Chromium OS
On Mon, Jul 10, 2017 at 2:18 PM, Sean Paul <[email protected]> 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 <[email protected]>
>> Cc: Jani Nikula <[email protected]>
>> Cc: Sean Paul <[email protected]>
>> Cc: David Airlie <[email protected]>
>> Cc: Rob Clark <[email protected]>
>> Cc: Xinliang Liu <[email protected]>
>> Cc: Xinliang Liu <[email protected]>
>> Cc: Rongrong Zou <[email protected]>
>> Cc: Xinwei Kong <[email protected]>
>> Cc: Chen Feng <[email protected]>
>> Cc: Jose Abreu <[email protected]>
>> Cc: Archit Taneja <[email protected]>
>> Cc: [email protected]
>> Signed-off-by: John Stultz <[email protected]>
>> ---
>> 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 <[email protected]> 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 <[email protected]>
> >> Cc: Jani Nikula <[email protected]>
> >> Cc: Sean Paul <[email protected]>
> >> Cc: David Airlie <[email protected]>
> >> Cc: Rob Clark <[email protected]>
> >> Cc: Xinliang Liu <[email protected]>
> >> Cc: Xinliang Liu <[email protected]>
> >> Cc: Rongrong Zou <[email protected]>
> >> Cc: Xinwei Kong <[email protected]>
> >> Cc: Chen Feng <[email protected]>
> >> Cc: Jose Abreu <[email protected]>
> >> Cc: Archit Taneja <[email protected]>
> >> Cc: [email protected]
> >> Signed-off-by: John Stultz <[email protected]>
> >> ---
> >> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Tue, Jul 11, 2017 at 12:56 AM, Daniel Vetter <[email protected]> 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 <[email protected]> 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 <[email protected]>
>> >> Cc: Jani Nikula <[email protected]>
>> >> Cc: Sean Paul <[email protected]>
>> >> Cc: David Airlie <[email protected]>
>> >> Cc: Rob Clark <[email protected]>
>> >> Cc: Xinliang Liu <[email protected]>
>> >> Cc: Xinliang Liu <[email protected]>
>> >> Cc: Rongrong Zou <[email protected]>
>> >> Cc: Xinwei Kong <[email protected]>
>> >> Cc: Chen Feng <[email protected]>
>> >> Cc: Jose Abreu <[email protected]>
>> >> Cc: Archit Taneja <[email protected]>
>> >> Cc: [email protected]
>> >> Signed-off-by: John Stultz <[email protected]>
>> >> ---
>> >> 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 <[email protected]> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Tue, Jul 11, 2017 at 8:12 AM, Daniel Vetter <[email protected]> wrote:
> On Tue, Jul 11, 2017 at 5:05 PM, John Stultz <[email protected]> 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 <[email protected]> wrote:
> On Tue, Jul 11, 2017 at 8:12 AM, Daniel Vetter <[email protected]> wrote:
>> On Tue, Jul 11, 2017 at 5:05 PM, John Stultz <[email protected]> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Tue, Jul 11, 2017 at 9:27 AM, Daniel Vetter <[email protected]> wrote:
> On Tue, Jul 11, 2017 at 5:44 PM, John Stultz <[email protected]> wrote:
>> On Tue, Jul 11, 2017 at 8:12 AM, Daniel Vetter <[email protected]> wrote:
>>> On Tue, Jul 11, 2017 at 5:05 PM, John Stultz <[email protected]> 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 <[email protected]> wrote:
> > On Tue, Jul 11, 2017 at 5:44 PM, John Stultz <[email protected]> wrote:
> >> On Tue, Jul 11, 2017 at 8:12 AM, Daniel Vetter <[email protected]> wrote:
> >>> On Tue, Jul 11, 2017 at 5:05 PM, John Stultz <[email protected]> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch