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 <p...@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
