LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "H. Nikolaus Schaller" <hns@goldelico.com>
To: Paul Cercueil <paul@crapouillou.net>
Cc: Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Kees Cook <keescook@chromium.org>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	Andrzej Hajda <a.hajda@samsung.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Robert Foss <robert.foss@linaro.org>,
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
	Jernej Skrabec <jernej.skrabec@gmail.com>,
	Ezequiel Garcia <ezequiel@collabora.com>,
	Harry Wentland <harry.wentland@amd.com>,
	Sam Ravnborg <sam@ravnborg.org>,
	Maxime Ripard <maxime@cerno.tech>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	devicetree@vger.kernel.org, linux-mips@vger.kernel.org,
	linux-kernel@vger.kernel.org, letux-kernel@openphoenux.org,
	Paul Boddie <paul@boddie.org.uk>, Jonas Karlman <jonas@kwiboo.se>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 3/8] drm/ingenic: Add support for JZ4780 and HDMI output
Date: Sun, 8 Aug 2021 07:04:01 +0200	[thread overview]
Message-ID: <B36D62BF-E4AF-46D5-BBD1-B73F66584F99@goldelico.com> (raw)
In-Reply-To: <CDHDXQ.QTKW3N6WINVB2@crapouillou.net>

Hi Paul,
we have v3 ready and I'll post soon.
Before, here are some feedback to your comments.

> Am 05.08.2021 um 17:22 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus & Paul,
>> +
>> +		if (priv->soc_info->hwdesc_size == sizeof(struct ingenic_dma_hwdesc_ext)) {
> 
> I'd prefer a boolean flag, e.g. "soc_info->use_extended_hwdesc"

Done.

>> +			hwdesc_ext->cpos |= JZ_LCD_CPOS_PREMULTIPLY_LCD |
>> +					    (3 << JZ_LCD_CPOS_COEFFICIENT_OFFSET);
> 
> Where's that magic value '3' coming from?

We have defined a constant.

> 
>> +
>> +			hwdesc_ext->dessize =
>> +				(0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) |
>> +				(((height - 1) & JZ_LCD_DESSIZE_HEIGHT_MASK) <<
>> +						 JZ_LCD_DESSIZE_HEIGHT_OFFSET) |
>> +				(((width - 1) & JZ_LCD_DESSIZE_WIDTH_MASK) <<
>> +						JZ_LCD_DESSIZE_WIDTH_OFFSET);
> 
> Use FIELD_PREP() from <linux/bitfield.h>.

Changed.

> 
>> +		}
>> 		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>> 			fourcc = newstate->fb->format->format;
>> @@ -612,8 +657,12 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
>> 	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
>> 	struct drm_connector *conn = conn_state->connector;
>> 	struct drm_display_info *info = &conn->display_info;
>> +	u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>> 	unsigned int cfg, rgbcfg = 0;
>> +	if (info->num_bus_formats)
>> +		bus_format = info->bus_formats[0];
>> +
> 
> That code is going to change really soon, as I have my own PR ready to convert the ingenic-drm driver to use a top-level bridge for bus format / flags negociation.
> 
> The HDMI driver should therefore implement it as well; see for instance drivers/gpu/drm/bridge/ite-it66121.c for an example of how the bus format is negociated.
> 
> I'll be sure to Cc you as soon as I send it upstream - should be just in a couple of days.

This one is still open.

> 
>> 	priv->panel_is_sharp = info->bus_flags & DRM_BUS_FLAG_SHARP_SIGNALS;
>> 	if (priv->panel_is_sharp) {
>> @@ -623,6 +672,13 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
>> 		    | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE;
>> 	}
>> +	if (priv->soc_info->has_recover)
>> +		cfg |= JZ_LCD_CFG_RECOVER_FIFO_UNDERRUN;
> 
> Seems out of place. Does it not work without?

Yes. We have moved it into a separate patch which enables additional jz4780 features.

> 
>> +
>> +	/* CI20: set use of the 8-word descriptor and OSD foreground usage. */
> 
> Not really CI20-specific though.

CI20 reference removed.

> 
>> +	switch (conn_state->connector->connector_type) {
>> +	case DRM_MODE_CONNECTOR_TV:
>> +	case DRM_MODE_CONNECTOR_HDMIA:
>> +		return 0;
>> +	}
> 
> This switch should move after the check on "num_bus_formats".
> (I understand why you did it, but with proper bus format negociation this won't be needed).

Not yet included since it breaks initialization. I think your proposed series will fix it.

> 
>> +
>> 	if (info->num_bus_formats != 1)
>> 		return -EINVAL;
>> -	drm->mode_config.max_height = 4095;
>> +	drm->mode_config.max_height = soc_info->max_height;
> 
> The drm->mode_config.max_height is different from soc_info->max_height; the former is the maximum framebuffer size, the latter is the maximum size that the SoC can display. The framebuffer can be bigger than what the SoC can display.

Change removed.

> 
>> 	drm->mode_config.funcs = &ingenic_drm_mode_config_funcs;
>> 	drm->mode_config.helper_private = &ingenic_drm_mode_config_helpers;
>> @@ -934,6 +994,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>> 		return PTR_ERR(base);
>> 	}
>> +	ingenic_drm_regmap_config.max_register = soc_info->max_reg;
> 
> Avoid modifying a global variable; instead copy it to a local copy of ingenic_drm_regmap_config, and use this one in the regmap_init_mmio below.

modifies now a local copy on stack in v3.

> 
>> 	priv->map = devm_regmap_init_mmio(dev, base,
>> 					  &ingenic_drm_regmap_config);
>> 	if (IS_ERR(priv->map)) {
>> @@ -966,7 +1027,6 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>> 	if (!priv->dma_hwdescs)
>> 		return -ENOMEM;
>> -
> 
> Cosmetic change - not needed.

removed.

> 
>> 	/* Configure DMA hwdesc for foreground0 plane */
>> 	dma_hwdesc_phys_f0 = priv->dma_hwdescs_phys
>> 		+ offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
>> @@ -1147,7 +1207,26 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>> 	/* Enable OSD if available */
>> 	if (soc_info->has_osd)
>> -		regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
>> +		regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
>> +
>> +	if (soc_info->has_alpha)
>> +		regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_ALPHAEN);
> 
> Do you need alpha blending support between planes, in this patch related to HDMI?

No. We have moved it into a separate patch which enables additional jz4780 features.

> 
>> +
>> +	/* Magic values from the vendor kernel for the priority thresholds. */
>> +	if (soc_info->has_pcfg)
>> +		regmap_write(priv->map, JZ_REG_LCD_PCFG,
>> +			     JZ_LCD_PCFG_PRI_MODE |
>> +			     JZ_LCD_PCFG_HP_BST_16 |
>> +			     (511 << JZ_LCD_PCFG_THRESHOLD2_OFFSET) |
>> +			     (400 << JZ_LCD_PCFG_THRESHOLD1_OFFSET) |
>> +			     (256 << JZ_LCD_PCFG_THRESHOLD0_OFFSET));
> 
> And why is that needed in this patch? Doesn't HDMI work without it?

Yes, works without. We have moved it into a separate patch which enables additional jz4780 features.

> 
>> +
>> +	/* RGB output control may be superfluous. */
>> +	if (soc_info->has_rgbc)
>> +		regmap_write(priv->map, JZ_REG_LCD_RGBC,
>> +			     JZ_LCD_RGBC_RGB_FORMAT_ENABLE |
>> +			     JZ_LCD_RGBC_ODD_LINE_RGB |
>> +			     JZ_LCD_RGBC_EVEN_LINE_RGB);
> 
> The last two macros set the subpixel ordering on the bus for serial (3x8) 24-bit panels - completely unrelated to HDMI.

Yes. We have moved it into a separate patch which enables additional jz4780 features.

> 
>> 	mutex_init(&priv->clk_mutex);
>> 	priv->clock_nb.notifier_call = ingenic_drm_update_pixclk;
>> @@ -1296,41 +1375,75 @@ static const struct jz_soc_info jz4740_soc_info = {
>> 	.needs_dev_clk = true,
>> 	.has_osd = false,
>> 	.map_noncoherent = false,
>> +	.has_pcfg = false,
>> +	.has_recover = false,
>> +	.has_rgbc = false,
>> +	.hwdesc_size = sizeof(struct ingenic_dma_hwdesc),

...

>> +	.has_alpha = true,
>> +	.has_pcfg = true,
>> +	.has_recover = true,
>> +	.has_rgbc = true,
>> +	.hwdesc_size = sizeof(struct ingenic_dma_hwdesc_ext),

We have moved it into a separate patch which enables additional jz4780 features.

>> +	.max_width = 4096,
>> +	.max_height = 4096,
> 
> The PM says that the display is up to 4096x2048-30Hz; so this is wrong.

Changed max_height to 2048.

>> -	return platform_driver_register(&ingenic_drm_driver);
>> +	err = platform_driver_register(&ingenic_drm_driver);
>> +	if (err)
>> +		goto err_ipu_unreg;
> 
> That's actually a change of behaviour - before it would return on error without calling platform_driver_unregister on the IPU.
> 
> It is not necesarily bad, but it belongs in a separate commit.

We have added a separate commit at the beginning of the v3 series to fix IPU unregister.
And then add the hdmi register/unregister.

>> +#define JZ_LCD_RGBC_ODD_LINE_MASK		(0x7 << 4)
>> +#define JZ_LCD_RGBC_ODD_LINE_RGB		(0 << 4)
>> +#define JZ_LCD_RGBC_ODD_LINE_RBG		(1 << 4)
>> +#define JZ_LCD_RGBC_ODD_LINE_GRB		(2 << 4)
>> +#define JZ_LCD_RGBC_ODD_LINE_GBR		(3 << 4)
>> +#define JZ_LCD_RGBC_ODD_LINE_BRG		(4 << 4)
>> +#define JZ_LCD_RGBC_ODD_LINE_BGR		(5 << 4)
>> +#define JZ_LCD_RGBC_EVEN_LINE_MASK		(0x7 << 0)
>> +#define JZ_LCD_RGBC_EVEN_LINE_RGB		0
>> +#define JZ_LCD_RGBC_EVEN_LINE_RBG		1
>> +#define JZ_LCD_RGBC_EVEN_LINE_GRB		2
>> +#define JZ_LCD_RGBC_EVEN_LINE_GBR		3
>> +#define JZ_LCD_RGBC_EVEN_LINE_BRG		4
>> +#define JZ_LCD_RGBC_EVEN_LINE_BGR		5
> 
> We already have these in ingenic-drm.h...
> 
> Please only add the macros that you need and are missing.

removed and made use of JZ_LCD_RGBC_EVEN_RGB etc.

> 
> Cheers,
> -Paul

BR and thanks,
Nikolaus


  parent reply	other threads:[~2021-08-08  5:04 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-05 14:07 [PATCH v2 0/8] MIPS: jz4780 HDMI H. Nikolaus Schaller
2021-08-05 14:07 ` [PATCH v2 1/8] drm/bridge: synopsis: Add mode_fixup and bridge timings support H. Nikolaus Schaller
2021-08-05 14:32   ` Robert Foss
2021-08-05 14:45     ` H. Nikolaus Schaller
2021-08-08  4:50     ` H. Nikolaus Schaller
2021-08-05 14:07 ` [PATCH v2 2/8] drm/ingenic: Add jz4780 Synopsys HDMI driver H. Nikolaus Schaller
2021-08-05 14:07 ` [PATCH v2 3/8] drm/ingenic: Add support for JZ4780 and HDMI output H. Nikolaus Schaller
2021-08-05 15:22   ` Paul Cercueil
2021-08-05 16:01     ` H. Nikolaus Schaller
2021-08-08  5:04     ` H. Nikolaus Schaller [this message]
2021-08-05 14:07 ` [PATCH v2 4/8] dt-bindings: display: Add ingenic-jz4780-hdmi DT Schema H. Nikolaus Schaller
2021-08-05 14:07 ` [PATCH v2 5/8] MIPS: DTS: jz4780: account for Synopsys HDMI driver and LCD controller H. Nikolaus Schaller
2021-08-05 14:07 ` [PATCH v2 6/8] MIPS: DTS: CI20: add HDMI setup H. Nikolaus Schaller
2021-08-05 14:07 ` [PATCH v2 7/8] MIPS: CI20: defconfig: configure for DRM_DW_HDMI_JZ4780 H. Nikolaus Schaller
2021-08-05 14:07 ` [PATCH v2 8/8] [RFC] drm/ingenic: convert to component framework for jz4780 hdmi H. Nikolaus Schaller
2021-08-05 15:04   ` Laurent Pinchart
2021-08-05 16:07     ` H. Nikolaus Schaller
2021-08-05 16:17       ` Paul Cercueil
2021-08-05 16:22         ` H. Nikolaus Schaller
2021-08-10  9:31         ` Daniel Vetter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=B36D62BF-E4AF-46D5-BBD1-B73F66584F99@goldelico.com \
    --to=hns@goldelico.com \
    --cc=Laurent.pinchart@ideasonboard.com \
    --cc=a.hajda@samsung.com \
    --cc=airlied@linux.ie \
    --cc=daniel@ffwll.ch \
    --cc=devicetree@vger.kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=ebiederm@xmission.com \
    --cc=ezequiel@collabora.com \
    --cc=geert+renesas@glider.be \
    --cc=harry.wentland@amd.com \
    --cc=hverkuil-cisco@xs4all.nl \
    --cc=jernej.skrabec@gmail.com \
    --cc=jonas@kwiboo.se \
    --cc=keescook@chromium.org \
    --cc=letux-kernel@openphoenux.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=maxime@cerno.tech \
    --cc=miquel.raynal@bootlin.com \
    --cc=narmstrong@baylibre.com \
    --cc=paul@boddie.org.uk \
    --cc=paul@crapouillou.net \
    --cc=robert.foss@linaro.org \
    --cc=robh+dt@kernel.org \
    --cc=sam@ravnborg.org \
    --cc=tsbogend@alpha.franken.de \
    --subject='Re: [PATCH v2 3/8] drm/ingenic: Add support for JZ4780 and HDMI output' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).