Re: [PATCH] drm/exynos: use atomic helper commit

[Inki Dae <inki.dae@samsung.com>]

Hi Laurent,

2017년 01월 17일 18:17에 Laurent Pinchart 이(가) 쓴 글:
> Hi Inki,
> 
> Thank you for the patch.
> 
> On Monday 16 Jan 2017 18:13:22 Inki Dae wrote:
>> This patch relpaces specific atomic commit function
>> with atomic helper commit one, which also includes
>> atomic_commit_tail callback for Exynos SoC becasue
>> crtc devices on Exynos SoC uses power domain device
>> so drm_atomic_helper_commit_planes should be called
>> after drm_atomic_helper_commit_modeset_enables.
>>
>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c |   9 ++-
>>  drivers/gpu/drm/exynos/exynos_drm_drv.c  | 110 +---------------------------
>>  drivers/gpu/drm/exynos/exynos_drm_fb.c   |  25 ++++++-
>>  3 files changed, 33 insertions(+), 111 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index 2530bf5..47da612 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> @@ -39,6 +39,14 @@ static void exynos_drm_crtc_disable(struct drm_crtc
>> *crtc)
>>
>>  	if (exynos_crtc->ops->disable)
>>  		exynos_crtc->ops->disable(exynos_crtc);
>> +
>> +	if (crtc->state->event && !crtc->state->active) {
>> +		spin_lock_irq(&crtc->dev->event_lock);
>> +		drm_crtc_send_vblank_event(crtc, crtc->state->event);
>> +		spin_unlock_irq(&crtc->dev->event_lock);
>> +
>> +		crtc->state->event = NULL;
>> +	}
> 
> You also need to handle events for exynos_drm_crtc_enable(). I'm not too

Is there any corner case? I think there should be no event which is not handled when exynos_drm_crtc_enable() is called.
 
> familiar with the exynos drm driver, is that taken care of by event handling 
> in exynos_crtc_atomic_flush() ?

Yes, exynos_crtc_atomic_flush() handles the event - making crtc->state->event to NULL and handing the event.

However, I think no need to handle the event at this funtion so relevant code in exynos_crtc_ctomic_flush() is not clear to me excepting setting crtc->state->event to NULL - without this code, hw_done function will say warning.
This patch would be a first step to clean up atomic interfaces of Exynos DRM driver.

> 
> You could also split this change into a separate patch with a clear 
> explanation of why it's needed in the commit message.
> 
>>  }
> 
> [snip]
> 
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 3ec0535..9d0df00 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> 
> [snip]
> 
>> -static void exynos_atomic_commit_complete(struct exynos_atomic_commit
>> *commit)
>> -{
>> -	struct drm_device *dev = commit->dev;
>> -	struct exynos_drm_private *priv = dev->dev_private;
>> -	struct drm_atomic_state *state = commit->state;
>> -
>> -	drm_atomic_helper_commit_modeset_disables(dev, state);
>> -
>> -	drm_atomic_helper_commit_modeset_enables(dev, state);
>> -
>> -	/*
>> -	 * Exynos can't update planes with CRTCs and encoders disabled,
>> -	 * its updates routines, specially for FIMD, requires the clocks
>> -	 * to be enabled. So it is necessary to handle the modeset operations
>> -	 * *before* the commit_planes() step, this way it will always
>> -	 * have the relevant clocks enabled to perform the update.
>> -	 */
> 
> There's a value in this comment, I would copy it to 
> exynos_drm_atomic_commit_tail()

As I already left the comment at other email thread, I will update description of this patch instead because atomic kms helper already described why atomic_commit_tail callback is required - the driver using runtime PM should have different call order.
But.. ok, this is what Gustavo wanted also. Seems better to leave above comment because the more explanation, the better. 

> 
>> -	drm_atomic_helper_commit_planes(dev, state, 0);
>> -
>> -	drm_atomic_helper_wait_for_vblanks(dev, state);
>> -
>> -	drm_atomic_helper_cleanup_planes(dev, state);
>> -
>> -	drm_atomic_state_put(state);
>> -
>> -	spin_lock(&priv->lock);
>> -	priv->pending &= ~commit->crtcs;
>> -	spin_unlock(&priv->lock);
>> -
>> -	wake_up_all(&priv->wait);
>> -
>> -	kfree(commit);
>> -}
> 
> [snip]
> 
>> @@ -313,6 +204,7 @@ static void exynos_drm_preclose(struct drm_device *dev,
>>
>>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
>>  		exynos_drm_crtc_cancel_page_flip(crtc, file);
>> +
> 
> This isn't needed.
> 
>>  }
>>
>>  static void exynos_drm_postclose(struct drm_device *dev, struct drm_file
>> *file) diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c
>> b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 68d4142..1e10b73 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
>> @@ -187,11 +187,33 @@ dma_addr_t exynos_drm_fb_dma_addr(struct
>> drm_framebuffer *fb, int index) return exynos_fb->dma_addr[index];
>>  }
>>
>> +static void exynos_drm_atomic_commit_tail(struct drm_atomic_state *state)
>> +{
>> +	struct drm_device *dev = state->dev;
>> +
>> +	drm_atomic_helper_commit_modeset_disables(dev, state);
>> +
>> +	drm_atomic_helper_commit_modeset_enables(dev, state);
>> +
>> +	drm_atomic_helper_commit_planes(dev, state,
>> +					DRM_PLANE_COMMIT_ACTIVE_ONLY);
> 
> The DRM_PLANE_COMMIT_ACTIVE_ONLY flag wasn't set before, I think this change 
> should go in a separate patch (or at least be documented in the commit 
> message).

Agree. I will update description of this patch about why DRM_PLANE_COMMIT_ACTIVE_ONLY flag is used.

Thanks.

> 
>> +	drm_atomic_helper_commit_hw_done(state);
>> +
>> +	drm_atomic_helper_wait_for_vblanks(dev, state);
>> +
>> +	drm_atomic_helper_cleanup_planes(dev, state);
>> +}
> 
> [snip]
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel