Re: [PATCH v3 08/26] drm/i915/writeback: Define encoder->get_hw_state

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Mar 25, 2026 at 04:37:26PM +0530, Suraj Kandpal wrote:
> Define the get_hw_state function for encoder which
> get's the encoder state, pipe config.
> 
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_writeback.c    | 49 +++++++++++++++++++
>  .../drm/i915/display/intel_writeback_reg.h    |  3 ++
>  2 files changed, 52 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
> index 765f62fa38f8..64769609aefe 100644
> --- a/drivers/gpu/drm/i915/display/intel_writeback.c
> +++ b/drivers/gpu/drm/i915/display/intel_writeback.c
> @@ -17,7 +17,9 @@
>  #include "intel_de.h"
>  #include "intel_display_driver.h"
>  #include "intel_display_types.h"
> +#include "intel_display_utils.h"
>  #include "intel_writeback.h"
> +#include "intel_writeback_reg.h"
>  
>  struct intel_writeback_connector {
>  	struct intel_connector connector;
> @@ -98,6 +100,52 @@ static const struct drm_connector_helper_funcs conn_helper_funcs = {
>  	.mode_valid = intel_writeback_mode_valid,
>  };
>  
> +static bool
> +intel_writeback_get_hw_state(struct intel_encoder *encoder,
> +			     enum pipe *pipe)
> +{
> +	struct intel_display *display = to_intel_display(encoder);
> +	u8 pipe_mask = 0;
> +	u32 tmp;
> +
> +	/* TODO need to be done for both the wd transcoder */

The encoder should know its transcoder.

> +	tmp = intel_de_read(display,
> +			    TRANSCONF_WD(TRANSCODER_WD_0));
> +	if (!(tmp & WD_TRANS_ENABLE))
> +		return false;

Is this register really different enough from the norm to
warrant its own register definitions?

> +
> +	tmp = intel_de_read(display,
> +			    WD_TRANS_FUNC_CTL(TRANSCODER_WD_0));
> +
> +	if (!(tmp & TRANS_WD_FUNC_ENABLE))
> +		return false;
> +
> +	switch (tmp & WD_INPUT_SELECT_MASK) {
> +	case WD_INPUT_PIPE_A:
> +		pipe_mask |= BIT(PIPE_A);
> +		break;
> +	case WD_INPUT_PIPE_B:
> +		pipe_mask |= BIT(PIPE_B);
> +		break;
> +	case WD_INPUT_PIPE_C:
> +		pipe_mask |= BIT(PIPE_C);
> +		break;
> +	case WD_INPUT_PIPE_D:
> +		pipe_mask |= BIT(PIPE_D);
> +		break;
> +	default:
> +		MISSING_CASE(tmp & WD_INPUT_SELECT_MASK);
> +		fallthrough;
> +	}
> +
> +	if (pipe_mask == 0)
> +		return false;
> +
> +	*pipe = ffs(pipe_mask) - 1;

What's the point of this pipe_mask?

> +
> +	return true;
> +}
> +
>  int intel_writeback_init(struct intel_display *display)
>  {
>  	struct intel_encoder *encoder;
> @@ -122,6 +170,7 @@ int intel_writeback_init(struct intel_display *display)
>  	encoder->type = INTEL_OUTPUT_WRITEBACK;
>  	encoder->pipe_mask = ~0;
>  	encoder->cloneable = 0;
> +	encoder->get_hw_state = intel_writeback_get_hw_state;
>  
>  	connector = &writeback_conn->connector;
>  	ret = intel_writeback_connector_alloc(connector);
> diff --git a/drivers/gpu/drm/i915/display/intel_writeback_reg.h b/drivers/gpu/drm/i915/display/intel_writeback_reg.h
> index ffe302ef3dd9..5e7c6c99d191 100644
> --- a/drivers/gpu/drm/i915/display/intel_writeback_reg.h
> +++ b/drivers/gpu/drm/i915/display/intel_writeback_reg.h
> @@ -19,6 +19,9 @@
>  /* Gen12 WD */
>  #define _MMIO_WD(tc, wd0, wd1)	_MMIO_TRANS((tc) - TRANSCODER_WD_0, wd0, wd1)
>  
> +#define TRANSCONF_WD(tc)	_MMIO_WD(tc,\
> +				PIPE_WD0_OFFSET,\
> +				PIPE_WD1_OFFSET)
>  #define WD_TRANS_ENABLE		REG_BIT(31)
>  #define WD_TRANS_STATE		REG_BIT(30)
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel