Re: [Intel-gfx] [PATCH 10/12] drm/i915: Eliminate bigjoiner boolean

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 2/16/2022 4:34 PM, Ville Syrjälä wrote:
> On Wed, Feb 16, 2022 at 04:27:49PM +0530, Nautiyal, Ankit K wrote:
>> On 2/16/2022 12:02 AM, Ville Syrjala wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Since we now have the bigjoiner_pipes bitmask the boolean
>>> is redundant. Get rid of it.
>>>
>>> Also, populating bigjoiner_pipes already during
>>> encoder->compute_config() allows us to use it much earlier
>>> during the state calculation as well. The initial aim is
>>> to use it in intel_crtc_compute_config().
>>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/display/intel_cursor.c   |  2 +-
>>>    drivers/gpu/drm/i915/display/intel_display.c  | 50 ++++++++-----------
>>>    .../drm/i915/display/intel_display_debugfs.c  |  2 +-
>>>    .../drm/i915/display/intel_display_types.h    |  3 --
>>>    drivers/gpu/drm/i915/display/intel_dp.c       | 13 ++---
>>>    drivers/gpu/drm/i915/display/intel_vdsc.c     |  8 +--
>>>    .../drm/i915/display/skl_universal_plane.c    |  2 +-
>>>    7 files changed, 36 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
>>> index 1f448f4e9aaf..da6cf0515164 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
>>> @@ -640,7 +640,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>>>    	 * FIXME bigjoiner fastpath would be good
>>>    	 */
>>>    	if (!crtc_state->hw.active || intel_crtc_needs_modeset(crtc_state) ||
>>> -	    crtc_state->update_pipe || crtc_state->bigjoiner)
>>> +	    crtc_state->update_pipe || crtc_state->bigjoiner_pipes)
>>>    		goto slow;
>>>    
>>>    	/*
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>> index 47b5d8cc16fd..192474163edb 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>> @@ -1926,7 +1926,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>>>    	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
>>>    		return;
>>>    
>>> -	if (!new_crtc_state->bigjoiner) {
>>> +	if (!new_crtc_state->bigjoiner_pipes) {
>>>    		intel_encoders_pre_pll_enable(state, crtc);
>>>    
>>>    		if (new_crtc_state->shared_dpll)
>>> @@ -2727,7 +2727,7 @@ static void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state)
>>>    static void intel_bigjoiner_adjust_timings(const struct intel_crtc_state *crtc_state,
>>>    					   struct drm_display_mode *mode)
>>>    {
>>> -	if (!crtc_state->bigjoiner)
>>> +	if (!crtc_state->bigjoiner_pipes)
>>>    		return;
>>>    
>>>    	mode->crtc_clock /= 2;
>>> @@ -2811,7 +2811,7 @@ static void intel_bigjoiner_compute_pipe_src(struct intel_crtc_state *crtc_state
>>>    {
>>>    	int width, height;
>>>    
>>> -	if (!crtc_state->bigjoiner)
>>> +	if (!crtc_state->bigjoiner_pipes)
>>>    		return;
>>>    
>>>    	width = drm_rect_width(&crtc_state->pipe_src);
>>> @@ -4218,7 +4218,6 @@ static void intel_bigjoiner_get_config(struct intel_crtc_state *crtc_state)
>>>    	if (((master_pipes | slave_pipes) & BIT(pipe)) == 0)
>>>    		return;
>>>    
>>> -	crtc_state->bigjoiner = true;
>>>    	crtc_state->bigjoiner_pipes =
>>>    		BIT(get_bigjoiner_master_pipe(pipe, master_pipes, slave_pipes)) |
>>>    		get_bigjoiner_slave_pipes(pipe, master_pipes, slave_pipes);
>> Although not part of this patch, do we need to check if
>> get_bigjoiner_master_pipe() does not give PIPE_INVALID?
>>
>> Perhaps in a case where master_pipe is read as 0 but some garbage for
>> slave_pipes during readout?
>>
>> Should there be a check for INVALID_PIPE, before feeding into BIT() macro?
> I think if we want to do more thourough validation against totally bogus
> hardware programming then we should just do it once at the start.
> enabled_bigjoiner_pipes() does have something, but it's only good for
> the two joined pipes cases. Also it just warns and doesn't do anything
> more than that atm. The simple option might be to make it just zero out
> the masks entirely if they look totally bogus. The readout would then
> be skipped for all slave pipes.

Yes you are right, enabled_bigjoiner_pipes does have a check in the end 
and that will prevent bogus value to

a certain extent. Given case would not occur, atleast for two joined 
pipes case.

Anyways, the patch seems to be straight forward and looks good to me.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>