Hi,
On 30/07/18 17:12, Laurent Pinchart wrote:
> Hi Tomi,
>
> (CC'ing Jacopo Mondi for a comment about bus_formats in bridge drivers)
>
> Thank you for the patch.
Thanks for the review! Due to the length of the mail, I'll cut out all
the easy bits, which I'll address in the next version.
>> +config DRM_TIDSS
>
> Isn't the name TIDSS a bit too generic ? Previous platforms also had a DSS.
Any suggestions? =)
I'm calling the previous generations (DSS2-5) OMAP DSS, as they were
mostly used on OMAP platforms, and the cases they're used on non-OMAP
platforms (AM5 for example), it's still very similar to a version used
on an OMAP platform.
I don't have any good idea on what to call DSS6 and DSS7, so... "tidss".
>> +void tidss_crtc_vblank_irq(struct drm_crtc *crtc)
>> +{
>> + struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
>> +
>> + drm_crtc_handle_vblank(crtc);
>> +
>> + tidss_crtc_finish_page_flip(tcrtc);
>
> Shouldn't page flip be completed at frame done time ? What's the relationship
> between the vblank and frame done IRQs ?
vblank happens at each vertical blank, frame done happens when the
output is disabled, and the ongoing frame has been sent. So, framedone
is essentially the "final" vblank on an output.
>> + return;
>> +
>> + WARN_ON(tidss->dispc_ops->vp_go_busy(tidss->dispc,
>> + tcrtc->hw_videoport));
>
> What will then happen ?
Undefined behavior. Probably the new config doesn't get enabled. Then
again, if we hit the warning, something has already gone wrong earlier.
>> + // I think we always need the event to signal flip done
>> + WARN_ON(!crtc->state->event);
>
> When using drm_atomic_helper_commit() the helper allocates an event internal
> if none is requested by userspace, except for legacy cursor updates if the
> driver implements atomic_°async_check and atomic_async_commit. You will this
> always have an event. I think you can drop this WARN_ON(), the driver will
> crash later when dereferencing the event anyway.
Ok.
>> +
>> + if (crtc->state->gamma_lut) {
>> + lut = (struct drm_color_lut *)
>> + crtc->state->gamma_lut->data;
>> + length = crtc->state->gamma_lut->length /
>> + sizeof(*lut);
>> + }
>> + tidss->dispc_ops->vp_set_gamma(tidss->dispc,
>> + tcrtc->hw_videoport,
>> + lut, length);
>> + }
>> +
>> + WARN_ON(drm_crtc_vblank_get(crtc) != 0);
>
> When can this fail ?
I hope never, but I guess it's good to print something if it does fail.
Perhaps dev_err is better here.
>> +static void tidss_crtc_atomic_enable(struct drm_crtc *crtc,
>> + struct drm_crtc_state *old_state)
>> +{
>> + struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
>> + struct tidss_crtc_state *tcrtc_state = to_tidss_crtc_state(crtc->state);
>> + struct drm_device *ddev = crtc->dev;
>> + struct tidss_device *tidss = ddev->dev_private;
>> + const struct drm_display_mode *mode = &crtc->state->adjusted_mode;
>> + int r;
>> +
>> + dev_dbg(ddev->dev, "%s, event %p\n", __func__, crtc->state->event);
>> +
>> + tidss->dispc_ops->runtime_get(tidss->dispc);
>> +
>> + r = tidss->dispc_ops->vp_set_clk_rate(tidss->dispc, tcrtc->hw_videoport,
>> + mode->clock * 1000);
>> + WARN_ON(r);
>> +
>> + r = tidss->dispc_ops->vp_enable_clk(tidss->dispc, tcrtc->hw_videoport);
>> + WARN_ON(r);
>
> That doesn't seem to be good error handling :-/
No, it doesn't, but what can we do? I guess print an error and return.
I don't know how to handle errors in the atomic commit phase where
failure is not an option anymore.
>> +
>> + /* Turn vertical blanking interrupt reporting on. */
>> + drm_crtc_vblank_on(crtc);
>> +
>> + tcrtc->enabled = true;
>> +
>> + tidss->dispc_ops->vp_enable(tidss->dispc, tcrtc->hw_videoport,
>> + mode,
>> + tcrtc_state->bus_format,
>> + tcrtc_state->bus_flags);
>> +
>> + spin_lock_irq(&ddev->event_lock);
>> +
>> + if (crtc->state->event) {
>> + drm_crtc_send_vblank_event(crtc, crtc->state->event);
>> + crtc->state->event = NULL;
>> + }
>
> Why do you report vblank when enabling the CRTC ?
Hmm, I'm not sure. I think the idea was to report that the fb is taken
into use but thinking about it, it doesn't make sense.
>> +static void tidss_crtc_atomic_disable(struct drm_crtc *crtc,
>> + struct drm_crtc_state *old_state)
>> +{
>> + struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
>> + struct drm_device *ddev = crtc->dev;
>> + struct tidss_device *tidss = ddev->dev_private;
>> +
>> + dev_dbg(ddev->dev, "%s, event %p\n", __func__, crtc->state->event);
>> +
>> + reinit_completion(&tcrtc->framedone_completion);
>> +
>> + tidss->dispc_ops->vp_disable(tidss->dispc, tcrtc->hw_videoport);
>> +
>> + if (!wait_for_completion_timeout(&tcrtc->framedone_completion,
>> + msecs_to_jiffies(500)))
>> + dev_err(tidss->dev, "Timeout waiting for framedone on crtc %d",
>> + tcrtc->hw_videoport);
>
> Without detailed knowledge of the hardware I can't tell for sure, but couldn't
> this be racy ? Could a vblank event be reported right before the
> reinit_completion() call, and ->vp_disable() called before the start of the
> next frame, resulting in no further vblank event being generated ? A
> wake_up()/wait_for_event() with an explicit test on whether a page flip is
> pending (if possible at the hardware level ?) might be better.
This doesn't use vblank, but framedone. There's a single framedone when
the output has finished sending the last frame.
>> + spin_lock_irq(&ddev->event_lock);
>> + if (crtc->state->event) {
>> + drm_crtc_send_vblank_event(crtc, crtc->state->event);
>
> Is this needed only when waiting for completion times out, or is it needed in
> the general case ? I would assume the event to be signaled by the interrupt
> handler.
We don't get a vblank when the output disables, but framedone. At the
moment the framedone handler only completes the above completion,
nothing else.
>> + crtc->state->event = NULL;
>> + }
>> + spin_unlock_irq(&ddev->event_lock);
>> +
>> + tcrtc->enabled = false;
>> +
>> + drm_crtc_vblank_off(crtc);
>> +
>> + tidss->dispc_ops->vp_disable_clk(tidss->dispc, tcrtc->hw_videoport);
>> +
>> + tidss->dispc_ops->runtime_put(tidss->dispc);
>> +
>> + spin_lock_irq(&crtc->dev->event_lock);
>> + if (crtc->state->event) {
>> + drm_crtc_send_vblank_event(crtc, crtc->state->event);
>> + crtc->state->event = NULL;
>
> A second one ?
This must be a mistake...
>> + crtc->state = kzalloc(sizeof(struct tidss_crtc_state), GFP_KERNEL);
>
> Please don't rely on drm_crtc_state being the first field of tidss_crtc_state,
> it's not very error-proof.
I thought that was the style used in DRM all around?
>> +static struct drm_crtc_state *
>> +tidss_crtc_duplicate_state(struct drm_crtc *crtc)
>> +{
>> + struct tidss_crtc_state *state, *current_state;
>> +
>> + if (WARN_ON(!crtc->state))
>> + return NULL;
>
> Can this happen ?
I don't know. drm_atomic_helper_crtc_duplicate_state() has the same code.
>> + u64 (*read_and_clear_irqstatus)(struct dispc_device *dispc);
>> + void (*write_irqenable)(struct dispc_device *dispc, u64 enable);
>> +
>> + int (*runtime_get)(struct dispc_device *dispc);
>> + void (*runtime_put)(struct dispc_device *dispc);
>> +
>> + int (*get_num_planes)(struct dispc_device *dispc);
>> + int (*get_num_vps)(struct dispc_device *dispc);
>> +
>> + const char *(*plane_name)(struct dispc_device *dispc,
>> + u32 hw_plane);
>> + const char *(*vp_name)(struct dispc_device *dispc,
>> + u32 hw_videoport);
>
> Do the hardware IDs need to be exactly and explicitly 32-bit wide ? Can't we
> use unsigned int instead ?
We can, it just makes the lines awfully long =). We could use u8 as well.
>> +#ifdef CONFIG_PM_SLEEP
>> +static int tidss_suspend(struct device *dev)
>> +{
>> + struct tidss_device *tidss = dev_get_drvdata(dev);
>> + struct drm_atomic_state *state;
>> +
>> + drm_kms_helper_poll_disable(tidss->ddev);
>> +
>> + console_lock();
>> + drm_fbdev_cma_set_suspend(tidss->fbdev, 1);
>> + console_unlock();
>> +
>> + state = drm_atomic_helper_suspend(tidss->ddev);
>> +
>> + if (IS_ERR(state)) {
>> + console_lock();
>> + drm_fbdev_cma_set_suspend(tidss->fbdev, 0);
>> + console_unlock();
>> +
>> + drm_kms_helper_poll_enable(tidss->ddev);
>> +
>> + return PTR_ERR(state);
>> + }
>> +
>> + tidss->saved_state = state;
>
> We have drm_mode_config_helper_suspend() and drm_mode_config_helper_resume(),
> could they be used ?
Perhaps. I didn't notice those, as I used v4.14 as a base for the first
versions, and 4.14 didn't have them.
>> +
>> + platform_set_drvdata(pdev, tidss);
>> +
>> + ddev = drm_dev_alloc(&tidss_driver, dev);
>> + if (IS_ERR(ddev))
>> + return PTR_ERR(ddev);
>> +
>> + tidss->ddev = ddev;
>> + ddev->dev_private = tidss;
>> +
>> + pm_runtime_enable(dev);
>> +
>> + ret = tidss->feat->dispc_init(tidss);
>> + if (ret) {
>> + dev_err(dev, "failed to initialize dispc: %d\n", ret);
>> + goto err_disable_pm;
>> + }
>> +
>> +#ifndef CONFIG_PM_SLEEP
>> + /* no PM, so force enable DISPC */
>> + tidss->dispc_ops->runtime_resume(tidss->dispc);
>> +#endif
>
> This should really be hidden somewhere in the PM framework, it's pretty ugly
> otherwise :( Or should we depend on CONFIG_PM_SLEEP ?
Yes, it's ugly, and took some time to figure out why things are not
working (if I recall right, mainline k2g config didn't have PM_SLEEP)...
I don't think we should depend on PM_SLEEP.
>> + ret = tidss_modeset_init(tidss);
>> + if (ret < 0) {
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(dev, "failed to init DRM/KMS (%d)\n", ret);
>> + goto err_runtime_suspend;
>> + }
>> +
>> + irq = platform_get_irq(tidss->pdev, 0);
>> + if (irq < 0) {
>> + ret = irq;
>> + dev_err(dev, "platform_get_irq failed: %d\n", ret);
>> + goto err_modeset_cleanup;
>> + }
>> +
>> + ret = drm_irq_install(ddev, irq);
>> + if (ret) {
>> + dev_err(dev, "drm_irq_install failed: %d\n", ret);
>> + goto err_modeset_cleanup;
>> + }
>> +
>> +#ifdef CONFIG_DRM_FBDEV_EMULATION
>> + if (ddev->mode_config.num_connector) {
>> + struct drm_fbdev_cma *fbdev;
>> +
>> + fbdev = drm_fbdev_cma_init(ddev, 32,
>> + ddev->mode_config.num_connector);
>> + if (IS_ERR(fbdev)) {
>> + dev_err(tidss->dev, "fbdev init failed\n");
>> + ret = PTR_ERR(fbdev);
>> + goto err_irq_uninstall;
>> + }
>
> Should this be a non-fatal error ? We can still use the display even if the
> fbdev emulation isn't available.
Technically yes, but things are probably pretty broken if fbdev setup
fails. We could even proceed without irqs, and use polling =). I think
it's better to just stop if things fail that are not supposed to fail.
>> +struct tidss_device {
>> + struct device *dev; /* Underlying DSS device */
>> + struct platform_device *pdev; /* Underlying DSS platform device */
>
> Do we need to store both ? Can't we store dev only, and cast it to
> platform_device in the only location that needs it ?
Yes, I think I used it just as a shortcut.
>> +static int tidss_encoder_atomic_check(struct drm_encoder *encoder,
>> + struct drm_crtc_state *crtc_state,
>> + struct drm_connector_state *conn_state)
>> +{
>> + struct drm_device *ddev = encoder->dev;
>> + struct tidss_crtc_state *tcrtc_state = to_tidss_crtc_state(crtc_state);
>> + struct drm_display_info *di = &conn_state->connector->display_info;
>> +
>> + dev_dbg(ddev->dev, "%s\n", __func__);
>> +
>> + // XXX any cleaner way to set bus format and flags?
>
> Not that I know of :-/ Jacopo (CC'ed) started working on support for bus
> formats in bridge drivers, which you might be interested in.
>
> This being said, why do you need to store them in the CRTC state, can't you
> access them directly in the two locations where they're used ?
If I recall right, it wasn't trivial to get that info in the crtc side.
>> +static void tidss_irq_fifo_underflow(struct tidss_device *tidss,
>> + u64 irqstatus)
>> +{
>> + static DEFINE_RATELIMIT_STATE(_rs, DEFAULT_RATELIMIT_INTERVAL,
>> + DEFAULT_RATELIMIT_BURST);
>> + unsigned int i;
>> + u64 masked;
>> +
>> + spin_lock(&tidss->wait_lock);
>> + masked = irqstatus & tidss->irq_uf_mask & tidss->irq_mask;
>
> Can you have irq_uf_mask bits that are not set in irq_mask ?
I think it's possible, at least in theory. I'm not sure if we can have
such a case in practice with the current driver. In any case, I'd rather
only look at irq bits that we've explicitly interested in (irq_mask),
instead of trusting that no extra irq bits are present.
>> + ddev->mode_config.min_width = 8;
>> + ddev->mode_config.min_height = 8;
>> + ddev->mode_config.max_width = 8096;
>> + ddev->mode_config.max_height = 8096;
>> + ddev->mode_config.funcs = &mode_config_funcs;
>> + ddev->mode_config.helper_private = &mode_config_helper_funcs;
>> +
>> + ret = tidss->dispc_ops->modeset_init(tidss->dispc);
>
> I'm not too fond of hiding the DRM encoder and DRM bridge instantiation code
> in the dispc layer. Would there be a way to move it here ?
Well... Don't think of it as dispc layer, but as dss6/dss7 helper. The
setup of the crtcs, planes, encoders, connectors and bridges depend on
the HW, so it was much easier to have dss6 and dss7 specific setups,
instead of trying to create common code for them (which I did first).
We could do it here, either as a single common code, or separete
functions similar to what's in dispcs now, but then it would mean
exposing HW details (say, port types, how crtcs and encoders connect to
each other, which planes can be used on which crtc) from the dispc side
only for the purpose of modeset init. Those details are, of course,
available afterwards via standard DRM ways, but until we've done the
modeset init, we have those details only in custom form.
Is there something specific that you dislike with this approach? My
thinking was that the dispc6/7 files are essentially helper functions
for things that are not common between DSS versions. From my omapdrm
experiences, common functions for different DSS versions easily become a
nightmare.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
