Re: [PATCH 2/2] drm/atomic: Make atomic iterators less surprising

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Op 27-09-17 om 14:53 schreef Dmitry Osipenko:
> On 27.09.2017 11:35, Maarten Lankhorst wrote:
>> Commit 669c9215afea ("drm/atomic: Make async plane update checks work as
>> intended, v2.") assumed incorrectly that if only 1 plane is matched in
>> the loop, the variables will be set to that plane. In reality we reset
>> them to NULL every time a new plane was iterated. This behavior is
>> surprising, so fix this by making the for loops only assign the
>> variables on a match.
>>
>> Cc: Dmitry Osipenko <digetx@xxxxxxxxx>
>> Fixes: 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.")
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> ---
>>  include/drm/drm_atomic.h | 85 ++++++++++++++++++++++++------------------------
>>  1 file changed, 42 insertions(+), 43 deletions(-)
>>
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index 6fae95f28e10..5afd6e364fb6 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -585,12 +585,12 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
>>   */
>>  #define for_each_oldnew_connector_in_state(__state, connector, old_connector_state, new_connector_state, __i) \
>>  	for ((__i) = 0;								\
>> -	     (__i) < (__state)->num_connector &&				\
>> -	     ((connector) = (__state)->connectors[__i].ptr,			\
>> -	     (old_connector_state) = (__state)->connectors[__i].old_state,	\
>> -	     (new_connector_state) = (__state)->connectors[__i].new_state, 1); 	\
>> -	     (__i)++)							\
>> -		for_each_if (connector)
>> +	     (__i) < (__state)->num_connector;					\
>> +	     (__i)++)								\
>> +		for_each_if ((__state)->connectors[__i].ptr &&			\
>> +			     ((connector) = (__state)->connectors[__i].ptr,	\
>> +			     (old_connector_state) = (__state)->connectors[__i].old_state,	\
>> +			     (new_connector_state) = (__state)->connectors[__i].new_state, 1))
>>  
>>  /**
>>   * for_each_old_connector_in_state - iterate over all connectors in an atomic update
>> @@ -606,11 +606,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
>>   */
>>  #define for_each_old_connector_in_state(__state, connector, old_connector_state, __i) \
>>  	for ((__i) = 0;								\
>> -	     (__i) < (__state)->num_connector &&				\
>> -	     ((connector) = (__state)->connectors[__i].ptr,			\
>> -	     (old_connector_state) = (__state)->connectors[__i].old_state, 1); 	\
>> -	     (__i)++)							\
>> -		for_each_if (connector)
>> +	     (__i) < (__state)->num_connector;					\
>> +	     (__i)++)								\
>> +		for_each_if ((__state)->connectors[__i].ptr &&			\
>> +			     ((connector) = (__state)->connectors[__i].ptr,	\
>> +			     (old_connector_state) = (__state)->connectors[__i].old_state, 1))
>>  
>>  /**
>>   * for_each_new_connector_in_state - iterate over all connectors in an atomic update
>> @@ -626,11 +626,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
>>   */
>>  #define for_each_new_connector_in_state(__state, connector, new_connector_state, __i) \
>>  	for ((__i) = 0;								\
>> -	     (__i) < (__state)->num_connector &&				\
>> -	     ((connector) = (__state)->connectors[__i].ptr,			\
>> -	     (new_connector_state) = (__state)->connectors[__i].new_state, 1); 	\
>> -	     (__i)++)							\
>> -		for_each_if (connector)
>> +	     (__i) < (__state)->num_connector;					\
>> +	     (__i)++)								\
>> +		for_each_if ((__state)->connectors[__i].ptr &&			\
>> +			     ((connector) = (__state)->connectors[__i].ptr,	\
>> +			     (new_connector_state) = (__state)->connectors[__i].new_state, 1))
>>  
>>  /**
>>   * for_each_oldnew_crtc_in_state - iterate over all CRTCs in an atomic update
>> @@ -646,12 +646,12 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
>>   */
>>  #define for_each_oldnew_crtc_in_state(__state, crtc, old_crtc_state, new_crtc_state, __i) \
>>  	for ((__i) = 0;							\
>> -	     (__i) < (__state)->dev->mode_config.num_crtc &&		\
>> -	     ((crtc) = (__state)->crtcs[__i].ptr,			\
>> -	     (old_crtc_state) = (__state)->crtcs[__i].old_state,	\
>> -	     (new_crtc_state) = (__state)->crtcs[__i].new_state, 1);	\
>> +	     (__i) < (__state)->dev->mode_config.num_crtc;		\
>>  	     (__i)++)							\
>> -		for_each_if (crtc)
>> +		for_each_if ((__state)->crtcs[__i].ptr &&		\
>> +			     ((crtc) = (__state)->crtcs[__i].ptr,	\
>> +			     (old_crtc_state) = (__state)->crtcs[__i].old_state, \
>> +			     (new_crtc_state) = (__state)->crtcs[__i].new_state, 1))
>>  
>>  /**
>>   * for_each_old_crtc_in_state - iterate over all CRTCs in an atomic update
>> @@ -666,11 +666,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
>>   */
>>  #define for_each_old_crtc_in_state(__state, crtc, old_crtc_state, __i)	\
>>  	for ((__i) = 0;							\
>> -	     (__i) < (__state)->dev->mode_config.num_crtc &&		\
>> -	     ((crtc) = (__state)->crtcs[__i].ptr,			\
>> -	     (old_crtc_state) = (__state)->crtcs[__i].old_state, 1);	\
>> +	     (__i) < (__state)->dev->mode_config.num_crtc;		\
>>  	     (__i)++)							\
>> -		for_each_if (crtc)
>> +		for_each_if ((__state)->crtcs[__i].ptr &&		\
>> +			     ((crtc) = (__state)->crtcs[__i].ptr,	\
>> +			     (old_crtc_state) = (__state)->crtcs[__i].old_state, 1))
>>  
>>  /**
>>   * for_each_new_crtc_in_state - iterate over all CRTCs in an atomic update
>> @@ -685,11 +685,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
>>   */
>>  #define for_each_new_crtc_in_state(__state, crtc, new_crtc_state, __i)	\
>>  	for ((__i) = 0;							\
>> -	     (__i) < (__state)->dev->mode_config.num_crtc &&		\
>> -	     ((crtc) = (__state)->crtcs[__i].ptr,			\
>> -	     (new_crtc_state) = (__state)->crtcs[__i].new_state, 1);	\
>> +	     (__i) < (__state)->dev->mode_config.num_crtc;		\
>>  	     (__i)++)							\
>> -		for_each_if (crtc)
>> +		for_each_if ((__state)->crtcs[__i].ptr &&		\
>> +			     ((crtc) = (__state)->crtcs[__i].ptr,	\
>> +			     (new_crtc_state) = (__state)->crtcs[__i].new_state, 1))
>>  
>>  /**
>>   * for_each_oldnew_plane_in_state - iterate over all planes in an atomic update
>> @@ -705,12 +705,12 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
>>   */
>>  #define for_each_oldnew_plane_in_state(__state, plane, old_plane_state, new_plane_state, __i) \
>>  	for ((__i) = 0;							\
>> -	     (__i) < (__state)->dev->mode_config.num_total_plane &&	\
>> -	     ((plane) = (__state)->planes[__i].ptr,			\
>> -	     (old_plane_state) = (__state)->planes[__i].old_state,	\
>> -	     (new_plane_state) = (__state)->planes[__i].new_state, 1);	\
>> +	     (__i) < (__state)->dev->mode_config.num_total_plane;	\
>>  	     (__i)++)							\
>> -		for_each_if (plane)
>> +		for_each_if ((__state)->planes[__i].ptr &&		\
>> +			     ((plane) = (__state)->planes[__i].ptr,	\
>> +			      (old_plane_state) = (__state)->planes[__i].old_state,\
>> +			      (new_plane_state) = (__state)->planes[__i].new_state, 1))
>>  
>>  /**
>>   * for_each_old_plane_in_state - iterate over all planes in an atomic update
>> @@ -725,12 +725,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
>>   */
>>  #define for_each_old_plane_in_state(__state, plane, old_plane_state, __i) \
>>  	for ((__i) = 0;							\
>> -	     (__i) < (__state)->dev->mode_config.num_total_plane &&	\
>> -	     ((plane) = (__state)->planes[__i].ptr,			\
>> -	     (old_plane_state) = (__state)->planes[__i].old_state, 1);	\
>> +	     (__i) < (__state)->dev->mode_config.num_total_plane;	\
>>  	     (__i)++)							\
>> -		for_each_if (plane)
>> -
>> +		for_each_if ((__state)->planes[__i].ptr &&		\
>> +			     ((plane) = (__state)->planes[__i].ptr,	\
>> +			      (old_plane_state) = (__state)->planes[__i].old_state, 1))
>>  /**
>>   * for_each_new_plane_in_state - iterate over all planes in an atomic update
>>   * @__state: &struct drm_atomic_state pointer
>> @@ -744,11 +743,11 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
>>   */
>>  #define for_each_new_plane_in_state(__state, plane, new_plane_state, __i) \
>>  	for ((__i) = 0;							\
>> -	     (__i) < (__state)->dev->mode_config.num_total_plane &&	\
>> -	     ((plane) = (__state)->planes[__i].ptr,			\
>> -	     (new_plane_state) = (__state)->planes[__i].new_state, 1);	\
>> +	     (__i) < (__state)->dev->mode_config.num_total_plane;	\
>>  	     (__i)++)							\
>> -		for_each_if (plane)
>> +		for_each_if ((__state)->planes[__i].ptr &&		\
>> +			     ((plane) = (__state)->planes[__i].ptr,	\
>> +			      (new_plane_state) = (__state)->planes[__i].new_state, 1))
>>  
>>  /**
>>   * for_each_oldnew_private_obj_in_state - iterate over all private objects in an atomic update
>>
> This variant also works.
>
> Tested-by: Dmitry Osipenko <digetx@xxxxxxxxx>
>
Forgot to hit push, pushed now. :)

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux