diff mbox series

[v2,01/22] drm: add clone mode check for CRTC

Message ID 20240924-concurrent-wb-v2-1-7849f900e863@quicinc.com (mailing list archive)
State New, archived
Headers show
Series drm/msm/dpu: Add Concurrent Writeback Support for DPU 10.x+ | expand

Commit Message

Jessica Zhang Sept. 24, 2024, 10:59 p.m. UTC
Add helper to check if the given CRTC state is in clone mode

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 include/drm/drm_crtc.h | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Dmitry Baryshkov Sept. 24, 2024, 11:06 p.m. UTC | #1
On Tue, Sep 24, 2024 at 03:59:17PM GMT, Jessica Zhang wrote:
> Add helper to check if the given CRTC state is in clone mode
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  include/drm/drm_crtc.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 8b48a1974da3..ecb93e2c4afc 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1323,5 +1323,12 @@ static inline struct drm_crtc *drm_crtc_find(struct drm_device *dev,
>  
>  int drm_crtc_create_scaling_filter_property(struct drm_crtc *crtc,
>  					    unsigned int supported_filters);

Missing kerneldoc

> +static inline bool drm_crtc_in_clone_mode(struct drm_crtc_state *crtc_state)
> +{
> +	if (!crtc_state)
> +		return false;
> +
> +	return hweight32(crtc_state->encoder_mask) > 1;
> +}
>  
>  #endif /* __DRM_CRTC_H__ */
> 
> -- 
> 2.34.1
>
Maxime Ripard Sept. 25, 2024, 7:22 a.m. UTC | #2
On Wed, Sep 25, 2024 at 02:06:35AM GMT, Dmitry Baryshkov wrote:
> On Tue, Sep 24, 2024 at 03:59:17PM GMT, Jessica Zhang wrote:
> > Add helper to check if the given CRTC state is in clone mode
> > 
> > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> > ---
> >  include/drm/drm_crtc.h | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 8b48a1974da3..ecb93e2c4afc 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -1323,5 +1323,12 @@ static inline struct drm_crtc *drm_crtc_find(struct drm_device *dev,
> >  
> >  int drm_crtc_create_scaling_filter_property(struct drm_crtc *crtc,
> >  					    unsigned int supported_filters);
> 
> Missing kerneldoc

Also, a proper commit log to describe what this will be used for, and
ideally some kunit tests.

Maxime
Jani Nikula Sept. 25, 2024, 8:12 a.m. UTC | #3
On Tue, 24 Sep 2024, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> Add helper to check if the given CRTC state is in clone mode
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  include/drm/drm_crtc.h | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 8b48a1974da3..ecb93e2c4afc 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1323,5 +1323,12 @@ static inline struct drm_crtc *drm_crtc_find(struct drm_device *dev,
>  
>  int drm_crtc_create_scaling_filter_property(struct drm_crtc *crtc,
>  					    unsigned int supported_filters);
> +static inline bool drm_crtc_in_clone_mode(struct drm_crtc_state *crtc_state)
> +{
> +	if (!crtc_state)
> +		return false;
> +
> +	return hweight32(crtc_state->encoder_mask) > 1;
> +}

What's the benefit of this being static inline?

You're implicitly depending on hweight32() being available, basically
<linux/bitops.h> being included. Maybe it already is, but it's the
accumulation of small and innocent looking things like this that then
explode the header dependencies, and make them harder to reduce.

BR,
Jani.

>  
>  #endif /* __DRM_CRTC_H__ */
Jessica Zhang Sept. 25, 2024, 8:47 p.m. UTC | #4
On 9/25/2024 1:12 AM, Jani Nikula wrote:
> On Tue, 24 Sep 2024, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>> Add helper to check if the given CRTC state is in clone mode
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   include/drm/drm_crtc.h | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index 8b48a1974da3..ecb93e2c4afc 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -1323,5 +1323,12 @@ static inline struct drm_crtc *drm_crtc_find(struct drm_device *dev,
>>   
>>   int drm_crtc_create_scaling_filter_property(struct drm_crtc *crtc,
>>   					    unsigned int supported_filters);
>> +static inline bool drm_crtc_in_clone_mode(struct drm_crtc_state *crtc_state)
>> +{
>> +	if (!crtc_state)
>> +		return false;
>> +
>> +	return hweight32(crtc_state->encoder_mask) > 1;
>> +}
> 
> What's the benefit of this being static inline?
> 
> You're implicitly depending on hweight32() being available, basically
> <linux/bitops.h> being included. Maybe it already is, but it's the
> accumulation of small and innocent looking things like this that then
> explode the header dependencies, and make them harder to reduce.

Hi Jani,

Good point, I'll move the implementation to drm_crtc.c.

Thanks,

Jessica Zhang

> 
> BR,
> Jani.
> 
>>   
>>   #endif /* __DRM_CRTC_H__ */
> 
> -- 
> Jani Nikula, Intel
diff mbox series

Patch

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8b48a1974da3..ecb93e2c4afc 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1323,5 +1323,12 @@  static inline struct drm_crtc *drm_crtc_find(struct drm_device *dev,
 
 int drm_crtc_create_scaling_filter_property(struct drm_crtc *crtc,
 					    unsigned int supported_filters);
+static inline bool drm_crtc_in_clone_mode(struct drm_crtc_state *crtc_state)
+{
+	if (!crtc_state)
+		return false;
+
+	return hweight32(crtc_state->encoder_mask) > 1;
+}
 
 #endif /* __DRM_CRTC_H__ */