Re: [PATCH] drm/atomic_helper: Add drm_atomic_helper_disable_planes_on_crtc()

[Jyri Sarha <jsarha@ti.com>]

On 11/23/15 18:09, Daniel Vetter wrote:
> On Mon, Nov 23, 2015 at 12:44:35PM +0200, Jyri Sarha wrote:
>> Add drm_atomic_helper_disable_planes_on_crtc() for disabling all
>> planes associated with the given CRTC. This can be used for instance
>> in the CRTC helper disable callback to disable all planes before
>> shutting down the display pipeline.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
>> This a follow version to "[PATCH RFC] drm/crtc_helper: Add
>> drm_crtc_helper_disable_planes()"-patch [1], with Daniel Vetter's
>> review comments [2] implemented. Hope I got everything right this
>> time. Thanks a lot for the prompt review.
>
> When resending a patch please add a revision log to remind reviewers of
> what you changed. Otherwise they have to dig out the old thread again to
> figure that out. E.g.
>
> v2 per Daniel's feedback:
> - Improve kerneldoc.
> - WARN_ON when atomic_disable is missing.
> - Also use crtc_funcs->atomic_begin/atomic_flush optionally.

I usually do that, but not when I drop RFC off, but I'll do it next time 
even then.

>>
>> Best regards,
>> Jyri
>>
>> [1] http://www.spinics.net/lists/dri-devel/msg94720.html
>> [2] http://www.spinics.net/lists/dri-devel/msg94722.html
>>
>>   drivers/gpu/drm/drm_atomic_helper.c | 52 +++++++++++++++++++++++++++++++++++++
>>   include/drm/drm_atomic_helper.h     |  2 ++
>>   2 files changed, 54 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index a53ed7a..229c810 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1281,6 +1281,58 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>>   EXPORT_SYMBOL(drm_atomic_helper_commit_planes);
>>
>>   /**
>> + * drm_atomic_helper_disable_planes_on_crtc - helper to disable CRTC's planes
>> + * @crtc: CRTC
>> + * @atomic: if set, synchronize with CRTC's atomic_begin/flush hooks
>> + *
>> + * Disables all planes associated with the given CRTC. This can be
>> + * used for instance in the CRTC helper disable callback to disable
>> + * all planes before shutting down the display pipeline.
>> + *
>> + * If there are planes to disable and the atomic-parameter is set the
>> + * function calls the CRTC's atomic_begin hook before and atomic_flush
>> + * hook after disabling the planes.
>> + *
>> + * It is a bug to call this function without having implemented the
>> + * ->atomic_disable() plane hook.
>> + */
>> +void drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc *crtc,
>> +					      bool atomic)
>> +{
>> +	const struct drm_crtc_helper_funcs *crtc_funcs =
>> +		crtc->helper_private;
>> +	struct drm_plane *plane;
>> +	bool flush = false;
>> +
>> +	if (!crtc_funcs || !crtc_funcs->atomic_begin)
>> +		atomic = false;
>> +
>> +	drm_for_each_plane(plane, crtc->dev) {
>> +		const struct drm_plane_helper_funcs *plane_funcs =
>> +			plane->helper_private;
>> +
>> +		if (plane->state->crtc != crtc || !plane_funcs)
>> +			continue;
>> +
>> +		if (atomic && !flush) {
>> +			crtc_funcs->atomic_begin(crtc, NULL);
>> +			flush = true;
>
> I think it's clearer if you do that upfront with a simple
>
> 	if (crtc_funcs->atomic_begin && atomic)
> 		crtc_funcs->atomic_begin();
>

That would cause ->atomic_begin() and ->atomic_flush() cycle even when 
there is no planes to disable. At least with omapdrm that causes a write 
HW and crtc_disable() is called once at probe time when the piece of HW 
not yet enabled.

I am not that familiar with either drm or omapdrm yet to tell if that is 
wrong, but in any case, calling ->atomic_begin() and ->atomic_flush() 
for nothing makes no sense to me.

Would you like it better if there would be two drm_for_each_plane() 
loops, one for just checking if there is anything to do and the second 
doing actual job and ->atomic_begin() there in between?


>> +		}
>> +
>> +		WARN_ON(!plane_funcs->atomic_disable);
>> +		if (plane_funcs->atomic_disable)
>> +			plane_funcs->atomic_disable(plane, NULL);
>> +	}
>> +
>> +	if (flush) {
>> +		WARN_ON(!crtc_funcs->atomic_flush);
>> +		if (crtc_funcs->atomic_flush)
>> +			crtc_funcs->atomic_flush(crtc, NULL);
>> +	}
>
> Same here below. Imo the tracking you do in flush isn't required, since
> drivers can opt to not implement either atomic_begin or atomic_flush (on
> some hw you only need atomic_flush, and your code would break such a
> driver here).
>

Ok, thanks I did not know that. I'll fix that.

> In short the code flow for atomic_begin/flush should look exactly like in
> drm_atomic_helper_commit_planes_on_crtc, except for the added && atomic
> check.
>
> Otherwise looks good.
>
> Cheers, Daniel
>

Thanks,
Jyri

>> +}
>> +EXPORT_SYMBOL(drm_atomic_helper_disable_planes_on_crtc);
>> +
>> +/**
>>    * drm_atomic_helper_commit_planes_on_crtc - commit plane state for a crtc
>>    * @old_crtc_state: atomic state object with the old crtc state
>>    *
>> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
>> index 8cba54a..b7d4237 100644
>> --- a/include/drm/drm_atomic_helper.h
>> +++ b/include/drm/drm_atomic_helper.h
>> @@ -62,6 +62,8 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>>   void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
>>   				      struct drm_atomic_state *old_state);
>>   void drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state);
>> +void drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc *crtc,
>> +					      bool atomic);
>>
>>   void drm_atomic_helper_swap_state(struct drm_device *dev,
>>   				  struct drm_atomic_state *state);
>> --
>> 1.9.1
>>
>

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