All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC][PATCH v2] drm: kirin: Add mode_valid logic to avoid mode clocks we can't generate
@ 2017-07-18  4:22 ` John Stultz
  0 siblings, 0 replies; 7+ messages in thread
From: John Stultz @ 2017-07-18  4:22 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Daniel Vetter, Jani Nikula, Sean Paul, David Airlie,
	Rob Clark, Xinliang Liu, Xinliang Liu, Rongrong Zou, Xinwei Kong,
	Chen Feng, Jose Abreu, Archit Taneja, dri-devel

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 ensure we don't select
modes we cannot generate.

NOTE: Stylistically I suspect there are better ways to do what
I'm trying to do here. The encoder -> crtc bit is terrible, and
getting the crtc adjusted mode from the encoder logic feels
less then ideal. So feedback would be greatly appreciated!

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>
---
v2: Reworked to calculate if modeclock matches the phy's byteclock,
    rather then using a whitelist of known modes.
---
 drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c    | 45 +++++++++++++++++++++++++
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c |  8 +++++
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h |  2 ++
 3 files changed, 55 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..9a553e7 100644
--- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
+++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
@@ -24,6 +24,7 @@
 #include <drm/drm_encoder_slave.h>
 #include <drm/drm_atomic_helper.h>
 
+#include "kirin_drm_drv.h"
 #include "dw_dsi_reg.h"
 
 #define MAX_TX_ESC_CLK		10
@@ -603,6 +604,49 @@ static void dsi_encoder_enable(struct drm_encoder *encoder)
 	dsi->enable = true;
 }
 
+static enum drm_mode_status dsi_encoder_mode_valid(struct drm_encoder *encoder,
+					const struct drm_display_mode *mode)
+{
+	struct dw_dsi *dsi = encoder_to_dsi(encoder);
+	struct drm_crtc *crtc = NULL;
+	struct mipi_phy_params phy;
+	u32 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
+	u32 adjusted_clock;
+	u32 req_kHz, act_kHz, lane_byte_clk_kHz;
+
+	/* Figure out what the adjusted modeclock will be */
+	/* XXX There's got to be a better way to go encoder->crtc */
+	drm_for_each_crtc(crtc, encoder->dev)
+		if (crtc)
+			break;
+	if (crtc)
+		adjusted_clock = kirin_ade_adj_mode_clk(crtc, mode->clock);
+	else
+		adjusted_clock = mode->clock;
+
+	/* Calculate the lane byte clk using the adjusted mode clk */
+	memset(&phy, 0, sizeof(phy));
+	req_kHz = adjusted_clock * bpp / dsi->lanes;
+	act_kHz = dsi_calc_phy_rate(req_kHz, &phy);
+	lane_byte_clk_kHz = act_kHz / 8;
+
+	DRM_DEBUG_DRIVER("Checking mode %ix%i-%i@%i clock: %i adj_clock: %i...",
+			mode->hdisplay, mode->vdisplay, bpp,
+			drm_mode_vrefresh(mode), mode->clock, adjusted_clock);
+
+	/*
+	 * Make sure the adjused mode clock and the lane byte clk
+	 * have a common denominator base frequency
+	 */
+	if (adjusted_clock/dsi->lanes == lane_byte_clk_kHz/3) {
+		DRM_DEBUG_DRIVER("OK!\n");
+		return MODE_OK;
+	}
+
+	DRM_DEBUG_DRIVER("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 +666,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
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
index 074b0af..dffcf76 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
@@ -178,6 +178,14 @@ static void ade_init(struct ade_hw_ctx *ctx)
 			FRM_END_START_MASK, REG_EFFECTIVE_IN_ADEEN_FRMEND);
 }
 
+u32 kirin_ade_adj_mode_clk(struct drm_crtc *crtc, u32 modeclk)
+{
+	struct ade_crtc *acrtc = to_ade_crtc(crtc);
+	struct ade_hw_ctx *ctx = acrtc->ctx;
+
+	return clk_round_rate(ctx->ade_pix_clk, modeclk * 1000) / 1000;
+}
+
 static void ade_set_pix_clk(struct ade_hw_ctx *ctx,
 			    struct drm_display_mode *mode,
 			    struct drm_display_mode *adj_mode)
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
index 7f60c649..85f69ee 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
@@ -27,4 +27,6 @@ struct kirin_drm_private {
 
 extern const struct kirin_dc_ops ade_dc_ops;
 
+u32 kirin_ade_adj_mode_clk(struct drm_crtc *crtc, u32 modeclk);
+
 #endif /* __KIRIN_DRM_DRV_H__ */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [RFC][PATCH v2] drm: kirin: Add mode_valid logic to avoid mode clocks we can't generate
@ 2017-07-18  4:22 ` John Stultz
  0 siblings, 0 replies; 7+ messages in thread
From: John Stultz @ 2017-07-18  4:22 UTC (permalink / raw)
  To: lkml
  Cc: Jose Abreu, Chen Feng, Xinwei Kong, Xinliang Liu, dri-devel,
	Rongrong Zou, Daniel Vetter

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 ensure we don't select
modes we cannot generate.

NOTE: Stylistically I suspect there are better ways to do what
I'm trying to do here. The encoder -> crtc bit is terrible, and
getting the crtc adjusted mode from the encoder logic feels
less then ideal. So feedback would be greatly appreciated!

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>
---
v2: Reworked to calculate if modeclock matches the phy's byteclock,
    rather then using a whitelist of known modes.
---
 drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c    | 45 +++++++++++++++++++++++++
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c |  8 +++++
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h |  2 ++
 3 files changed, 55 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..9a553e7 100644
--- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
+++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
@@ -24,6 +24,7 @@
 #include <drm/drm_encoder_slave.h>
 #include <drm/drm_atomic_helper.h>
 
+#include "kirin_drm_drv.h"
 #include "dw_dsi_reg.h"
 
 #define MAX_TX_ESC_CLK		10
@@ -603,6 +604,49 @@ static void dsi_encoder_enable(struct drm_encoder *encoder)
 	dsi->enable = true;
 }
 
+static enum drm_mode_status dsi_encoder_mode_valid(struct drm_encoder *encoder,
+					const struct drm_display_mode *mode)
+{
+	struct dw_dsi *dsi = encoder_to_dsi(encoder);
+	struct drm_crtc *crtc = NULL;
+	struct mipi_phy_params phy;
+	u32 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
+	u32 adjusted_clock;
+	u32 req_kHz, act_kHz, lane_byte_clk_kHz;
+
+	/* Figure out what the adjusted modeclock will be */
+	/* XXX There's got to be a better way to go encoder->crtc */
+	drm_for_each_crtc(crtc, encoder->dev)
+		if (crtc)
+			break;
+	if (crtc)
+		adjusted_clock = kirin_ade_adj_mode_clk(crtc, mode->clock);
+	else
+		adjusted_clock = mode->clock;
+
+	/* Calculate the lane byte clk using the adjusted mode clk */
+	memset(&phy, 0, sizeof(phy));
+	req_kHz = adjusted_clock * bpp / dsi->lanes;
+	act_kHz = dsi_calc_phy_rate(req_kHz, &phy);
+	lane_byte_clk_kHz = act_kHz / 8;
+
+	DRM_DEBUG_DRIVER("Checking mode %ix%i-%i@%i clock: %i adj_clock: %i...",
+			mode->hdisplay, mode->vdisplay, bpp,
+			drm_mode_vrefresh(mode), mode->clock, adjusted_clock);
+
+	/*
+	 * Make sure the adjused mode clock and the lane byte clk
+	 * have a common denominator base frequency
+	 */
+	if (adjusted_clock/dsi->lanes == lane_byte_clk_kHz/3) {
+		DRM_DEBUG_DRIVER("OK!\n");
+		return MODE_OK;
+	}
+
+	DRM_DEBUG_DRIVER("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 +666,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
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
index 074b0af..dffcf76 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
@@ -178,6 +178,14 @@ static void ade_init(struct ade_hw_ctx *ctx)
 			FRM_END_START_MASK, REG_EFFECTIVE_IN_ADEEN_FRMEND);
 }
 
+u32 kirin_ade_adj_mode_clk(struct drm_crtc *crtc, u32 modeclk)
+{
+	struct ade_crtc *acrtc = to_ade_crtc(crtc);
+	struct ade_hw_ctx *ctx = acrtc->ctx;
+
+	return clk_round_rate(ctx->ade_pix_clk, modeclk * 1000) / 1000;
+}
+
 static void ade_set_pix_clk(struct ade_hw_ctx *ctx,
 			    struct drm_display_mode *mode,
 			    struct drm_display_mode *adj_mode)
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
index 7f60c649..85f69ee 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
@@ -27,4 +27,6 @@ struct kirin_drm_private {
 
 extern const struct kirin_dc_ops ade_dc_ops;
 
+u32 kirin_ade_adj_mode_clk(struct drm_crtc *crtc, u32 modeclk);
+
 #endif /* __KIRIN_DRM_DRV_H__ */
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC][PATCH v2] drm: kirin: Add mode_valid logic to avoid mode clocks we can't generate
  2017-07-18  4:22 ` John Stultz
@ 2017-07-18 11:10   ` Jose Abreu
  -1 siblings, 0 replies; 7+ messages in thread
From: Jose Abreu @ 2017-07-18 11:10 UTC (permalink / raw)
  To: John Stultz, lkml
  Cc: Daniel Vetter, Jani Nikula, Sean Paul, David Airlie, Rob Clark,
	Xinliang Liu, Xinliang Liu, Rongrong Zou, Xinwei Kong, Chen Feng,
	Jose Abreu, Archit Taneja, dri-devel

Hi John,


On 18-07-2017 05:22, 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 ensure we don't select
> modes we cannot generate.
>
> NOTE: Stylistically I suspect there are better ways to do what
> I'm trying to do here. The encoder -> crtc bit is terrible, and
> getting the crtc adjusted mode from the encoder logic feels
> less then ideal. So feedback would be greatly appreciated!
>
> 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>
> ---
> v2: Reworked to calculate if modeclock matches the phy's byteclock,
>     rather then using a whitelist of known modes.

Something like Daniel suggested would be simpler maybe:

encoder_mode_valid_aux()
{
    ...
}

dsi_encoder_mode_valid(...)
{
    drm_for_each_crtc(crtc, dev) {
        crtc->mode_fixup(crtc, mode, adjusted_mode);
        ret = encoder_mode_valid_aux(adjusted_mode);
        if (ret != MODE_OK)
            return ret;
    }
}

BTW, I think at commit stage you have encoder->crtc populated,
not sure though. But at probbing stage it will not be bound. And
also if you have more than one crtc this may be wrong. (How will
you know which crtc will be bound to the encoder so that you can
get the right clock?).

Best regards,
Jose Miguel Abreu

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC][PATCH v2] drm: kirin: Add mode_valid logic to avoid mode clocks we can't generate
@ 2017-07-18 11:10   ` Jose Abreu
  0 siblings, 0 replies; 7+ messages in thread
From: Jose Abreu @ 2017-07-18 11:10 UTC (permalink / raw)
  To: John Stultz, lkml
  Cc: Daniel Vetter, Jani Nikula, Sean Paul, David Airlie, Rob Clark,
	Xinliang Liu, Xinliang Liu, Rongrong Zou, Xinwei Kong, Chen Feng,
	Jose Abreu, Archit Taneja, dri-devel

Hi John,


On 18-07-2017 05:22, 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 ensure we don't select
> modes we cannot generate.
>
> NOTE: Stylistically I suspect there are better ways to do what
> I'm trying to do here. The encoder -> crtc bit is terrible, and
> getting the crtc adjusted mode from the encoder logic feels
> less then ideal. So feedback would be greatly appreciated!
>
> 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>
> ---
> v2: Reworked to calculate if modeclock matches the phy's byteclock,
>     rather then using a whitelist of known modes.

Something like Daniel suggested would be simpler maybe:

encoder_mode_valid_aux()
{
    ...
}

dsi_encoder_mode_valid(...)
{
    drm_for_each_crtc(crtc, dev) {
        crtc->mode_fixup(crtc, mode, adjusted_mode);
        ret = encoder_mode_valid_aux(adjusted_mode);
        if (ret != MODE_OK)
            return ret;
    }
}

BTW, I think at commit stage you have encoder->crtc populated,
not sure though. But at probbing stage it will not be bound. And
also if you have more than one crtc this may be wrong. (How will
you know which crtc will be bound to the encoder so that you can
get the right clock?).

Best regards,
Jose Miguel Abreu

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC][PATCH v2] drm: kirin: Add mode_valid logic to avoid mode clocks we can't generate
  2017-07-18 11:10   ` Jose Abreu
@ 2017-07-18 16:56     ` John Stultz
  -1 siblings, 0 replies; 7+ messages in thread
From: John Stultz @ 2017-07-18 16:56 UTC (permalink / raw)
  To: Jose Abreu
  Cc: lkml, Daniel Vetter, Jani Nikula, Sean Paul, David Airlie,
	Rob Clark, Xinliang Liu, Xinliang Liu, Rongrong Zou, Xinwei Kong,
	Chen Feng, Archit Taneja, dri-devel

On Tue, Jul 18, 2017 at 4:10 AM, Jose Abreu <Jose.Abreu@synopsys.com> wrote:
> Hi John,
>
>
> On 18-07-2017 05:22, 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 ensure we don't select
>> modes we cannot generate.
>>
>> NOTE: Stylistically I suspect there are better ways to do what
>> I'm trying to do here. The encoder -> crtc bit is terrible, and
>> getting the crtc adjusted mode from the encoder logic feels
>> less then ideal. So feedback would be greatly appreciated!
>>
>> 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>
>> ---
>> v2: Reworked to calculate if modeclock matches the phy's byteclock,
>>     rather then using a whitelist of known modes.
>
> Something like Daniel suggested would be simpler maybe:
>
> encoder_mode_valid_aux()
> {
>     ...
> }
>
> dsi_encoder_mode_valid(...)
> {
>     drm_for_each_crtc(crtc, dev) {
>         crtc->mode_fixup(crtc, mode, adjusted_mode);
>         ret = encoder_mode_valid_aux(adjusted_mode);
>         if (ret != MODE_OK)
>             return ret;
>     }
> }
>
> BTW, I think at commit stage you have encoder->crtc populated,
> not sure though. But at probbing stage it will not be bound. And
> also if you have more than one crtc this may be wrong. (How will
> you know which crtc will be bound to the encoder so that you can
> get the right clock?).


Thanks so much for the suggestion above and the explanation. I only
have a rough sense of the components and not as much sense of an
overview on how they are all initialized, so this is very helpful!

And yes, my case is fairly limited since the encoder and crtc are used
together for this driver, but I know it can be more complex with
others, so I'll re-implement as you've suggested!

One thing that does worry me with trying to validate modes without
knowing which path is to be used, is that if there were two crtcs (and
again, on my hardware, thankfully there isn't), and for a given mode,
one adjusted the mode and one didn't.  So then for that given mode the
encoder could only generate a valid mode for one of the crtcs, its not
clear if that is MODE_OK or MODE_BAD.  We don't want to prune modes
that could possibly work with other crtcs, but we don't want a mode
that cannot be generated be selected either.  To me it seems we should
have the path figured out before we go pruning modes. Obviously there
needs to be a probe step that checks if any mode works with a given
pipeline (so that we can validate the pipeline), but it seems like
there should be a second step to ensure once we have a pipeline bound
we don't try to use modes it cannot support. Am I further
misunderstanding something here?

Thanks again!
-john

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC][PATCH v2] drm: kirin: Add mode_valid logic to avoid mode clocks we can't generate
@ 2017-07-18 16:56     ` John Stultz
  0 siblings, 0 replies; 7+ messages in thread
From: John Stultz @ 2017-07-18 16:56 UTC (permalink / raw)
  To: Jose Abreu
  Cc: Chen Feng, lkml, Xinliang Liu, dri-devel, Rongrong Zou,
	Daniel Vetter, Xinwei Kong

On Tue, Jul 18, 2017 at 4:10 AM, Jose Abreu <Jose.Abreu@synopsys.com> wrote:
> Hi John,
>
>
> On 18-07-2017 05:22, 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 ensure we don't select
>> modes we cannot generate.
>>
>> NOTE: Stylistically I suspect there are better ways to do what
>> I'm trying to do here. The encoder -> crtc bit is terrible, and
>> getting the crtc adjusted mode from the encoder logic feels
>> less then ideal. So feedback would be greatly appreciated!
>>
>> 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>
>> ---
>> v2: Reworked to calculate if modeclock matches the phy's byteclock,
>>     rather then using a whitelist of known modes.
>
> Something like Daniel suggested would be simpler maybe:
>
> encoder_mode_valid_aux()
> {
>     ...
> }
>
> dsi_encoder_mode_valid(...)
> {
>     drm_for_each_crtc(crtc, dev) {
>         crtc->mode_fixup(crtc, mode, adjusted_mode);
>         ret = encoder_mode_valid_aux(adjusted_mode);
>         if (ret != MODE_OK)
>             return ret;
>     }
> }
>
> BTW, I think at commit stage you have encoder->crtc populated,
> not sure though. But at probbing stage it will not be bound. And
> also if you have more than one crtc this may be wrong. (How will
> you know which crtc will be bound to the encoder so that you can
> get the right clock?).


Thanks so much for the suggestion above and the explanation. I only
have a rough sense of the components and not as much sense of an
overview on how they are all initialized, so this is very helpful!

And yes, my case is fairly limited since the encoder and crtc are used
together for this driver, but I know it can be more complex with
others, so I'll re-implement as you've suggested!

One thing that does worry me with trying to validate modes without
knowing which path is to be used, is that if there were two crtcs (and
again, on my hardware, thankfully there isn't), and for a given mode,
one adjusted the mode and one didn't.  So then for that given mode the
encoder could only generate a valid mode for one of the crtcs, its not
clear if that is MODE_OK or MODE_BAD.  We don't want to prune modes
that could possibly work with other crtcs, but we don't want a mode
that cannot be generated be selected either.  To me it seems we should
have the path figured out before we go pruning modes. Obviously there
needs to be a probe step that checks if any mode works with a given
pipeline (so that we can validate the pipeline), but it seems like
there should be a second step to ensure once we have a pipeline bound
we don't try to use modes it cannot support. Am I further
misunderstanding something here?

Thanks again!
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC][PATCH v2] drm: kirin: Add mode_valid logic to avoid mode clocks we can't generate
  2017-07-18 16:56     ` John Stultz
  (?)
@ 2017-07-19 10:10     ` Jose Abreu
  -1 siblings, 0 replies; 7+ messages in thread
From: Jose Abreu @ 2017-07-19 10:10 UTC (permalink / raw)
  To: John Stultz, Jose Abreu
  Cc: lkml, Daniel Vetter, Jani Nikula, Sean Paul, David Airlie,
	Rob Clark, Xinliang Liu, Xinliang Liu, Rongrong Zou, Xinwei Kong,
	Chen Feng, Archit Taneja, dri-devel


On 18-07-2017 17:56, John Stultz wrote:
> On Tue, Jul 18, 2017 at 4:10 AM, Jose Abreu <Jose.Abreu@synopsys.com> wrote:
>> Hi John,
>>
>>
>> On 18-07-2017 05:22, 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 ensure we don't select
>>> modes we cannot generate.
>>>
>>> NOTE: Stylistically I suspect there are better ways to do what
>>> I'm trying to do here. The encoder -> crtc bit is terrible, and
>>> getting the crtc adjusted mode from the encoder logic feels
>>> less then ideal. So feedback would be greatly appreciated!
>>>
>>> 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>
>>> ---
>>> v2: Reworked to calculate if modeclock matches the phy's byteclock,
>>>     rather then using a whitelist of known modes.
>> Something like Daniel suggested would be simpler maybe:
>>
>> encoder_mode_valid_aux()
>> {
>>     ...
>> }
>>
>> dsi_encoder_mode_valid(...)
>> {
>>     drm_for_each_crtc(crtc, dev) {
>>         crtc->mode_fixup(crtc, mode, adjusted_mode);
>>         ret = encoder_mode_valid_aux(adjusted_mode);
>>         if (ret != MODE_OK)
>>             return ret;
>>     }
>> }
>>
>> BTW, I think at commit stage you have encoder->crtc populated,
>> not sure though. But at probbing stage it will not be bound. And
>> also if you have more than one crtc this may be wrong. (How will
>> you know which crtc will be bound to the encoder so that you can
>> get the right clock?).
>
> Thanks so much for the suggestion above and the explanation. I only
> have a rough sense of the components and not as much sense of an
> overview on how they are all initialized, so this is very helpful!
>
> And yes, my case is fairly limited since the encoder and crtc are used
> together for this driver, but I know it can be more complex with
> others, so I'll re-implement as you've suggested!
>
> One thing that does worry me with trying to validate modes without
> knowing which path is to be used, is that if there were two crtcs (and
> again, on my hardware, thankfully there isn't), and for a given mode,
> one adjusted the mode and one didn't.  So then for that given mode the
> encoder could only generate a valid mode for one of the crtcs, its not
> clear if that is MODE_OK or MODE_BAD.  We don't want to prune modes
> that could possibly work with other crtcs, but we don't want a mode
> that cannot be generated be selected either.  To me it seems we should
> have the path figured out before we go pruning modes. Obviously there
> needs to be a probe step that checks if any mode works with a given
> pipeline (so that we can validate the pipeline), but it seems like
> there should be a second step to ensure once we have a pipeline bound
> we don't try to use modes it cannot support. Am I further
> misunderstanding something here?

Well, we can't know at probbing stage the path that will be
chosen. If you check drm_mode_validate_pipeline() at
drm_probe_helper.c you will see that we start from the connector,
then check which encoders can belong to the connector and which
crtc's can belong to the encoder. So, for any possible pipeline
we validate the mode and if at least one is accepted then the
mode is probbed. I only tested this in arcpgu, which only has 1
pipeline. Maybe when there is a more complex driver using this we
can find if this is the right approach :)

Best regards,
Jose Miguel Abreu

>
> Thanks again!
> -john

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2017-07-19 10:11 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-18  4:22 [RFC][PATCH v2] drm: kirin: Add mode_valid logic to avoid mode clocks we can't generate John Stultz
2017-07-18  4:22 ` John Stultz
2017-07-18 11:10 ` Jose Abreu
2017-07-18 11:10   ` Jose Abreu
2017-07-18 16:56   ` John Stultz
2017-07-18 16:56     ` John Stultz
2017-07-19 10:10     ` Jose Abreu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.