Re: [PATCH v3 05/26] drm/i915/writeback: Init writeback connector

["Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>]

On 3/25/2026 4:37 PM, Suraj Kandpal wrote:
> Initialize writeback connector initialising the virtual encoder
> and intel connector. We also allocate memory for drm_writeback_connector
> but not the drm_connector within it due to a constraint
> we need all connectors to be an intel_connector.

Not true anymore.	

> The writeback_format arrays is used to tell the user which
> drm formats are supported by us.
> 
> Bspec: 49275
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>   drivers/gpu/drm/i915/Makefile                 |   1 +
>   .../gpu/drm/i915/display/intel_writeback.c    | 126 ++++++++++++++++++
>   .../gpu/drm/i915/display/intel_writeback.h    |  17 +++
>   3 files changed, 144 insertions(+)
>   create mode 100644 drivers/gpu/drm/i915/display/intel_writeback.c
>   create mode 100644 drivers/gpu/drm/i915/display/intel_writeback.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index b677720a1c2d..1e9140e7713c 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -315,6 +315,7 @@ i915-y += \
>   	display/intel_vblank.o \
>   	display/intel_vga.o \
>   	display/intel_wm.o \
> +	display/intel_writeback.o \
>   	display/skl_prefill.o \
>   	display/skl_scaler.o \
>   	display/skl_universal_plane.o \
> diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
> new file mode 100644
> index 000000000000..73101ee17d74
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_writeback.c
> @@ -0,0 +1,126 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2025 Intel Corporation
> + */
> +
> +#include <linux/slab.h>
> +#include <drm/drm_atomic_state_helper.h>
> +#include <drm/drm_writeback.h>
> +#include <drm/drm_modeset_helper_vtables.h>
> +#include <drm/drm_probe_helper.h>
> +#include <drm/drm_fourcc.h>
> +#include <drm/drm_encoder.h>
> +
> +#include "intel_atomic.h"
> +#include "intel_connector.h"
> +#include "intel_de.h"
> +#include "intel_display_driver.h"
> +#include "intel_display_types.h"
> +#include "intel_writeback.h"
> +
> +struct intel_writeback_connector {
> +	struct intel_connector connector;
> +	struct intel_encoder encoder;
> +	enum transcoder trans;
> +	int frame_num;
> +};

As I understand, this struct sits at the same hierarchy level as struct 
intel_digital_port/struct intel_dsi.

Something like the following might fit better with the existing framework

struct intel_writeback {
	struct intel_encoder base; /* DRM_MODE_ENCODER_VIRTUAL */
	struct intel_connector *attached_connector;
}

I am not sure what is the implication of this for the rest of the 
series* but there are already signs in this patch that the current 
structure does not work well.

For example, kfree(&writeback_conn->encoder) is freeing up an embedded 
member.

*My guess is we should be good as long as we attach the encoder and 
connector properly.

> +
> +static const u32 writeback_formats[] = {
> +	DRM_FORMAT_XYUV8888,
> +	DRM_FORMAT_YUYV,
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_XVYU2101010,
> +	DRM_FORMAT_VYUY,
> +	DRM_FORMAT_XBGR2101010,
> +};
> +
> +static int intel_writeback_connector_init(struct intel_connector *connector)
> +{
> +	struct intel_digital_connector_state *conn_state;
> +
> +	conn_state = kzalloc(sizeof(*conn_state), GFP_KERNEL);
> +	if (!conn_state)
> +		return -ENOMEM;
> +
> +	__drm_atomic_helper_connector_reset(&connector->base,
> +					    &conn_state->base);
> +	return 0;
> +}
> +
> +static int
> +intel_writeback_connector_alloc(struct intel_connector *connector)
> +{
> +	if (intel_writeback_connector_init(connector) < 0) {
> +		kfree(connector);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct drm_encoder_funcs drm_writeback_encoder_funcs = {
> +	.destroy = drm_encoder_cleanup,
> +};
> +
> +const struct drm_connector_funcs conn_funcs = {

To maintain consistency with the style used for other connectors

intel_writeback_connector_funcs

> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.atomic_duplicate_state = intel_digital_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static const struct drm_connector_helper_funcs conn_helper_funcs = {

Same,
intel_writeback_connector_helper_funcs

> +};
> +
> +int intel_writeback_init(struct intel_display *display)
> +{
> +	struct intel_encoder *encoder;
> +	struct intel_writeback_connector *writeback_conn;
> +	struct intel_connector *connector;
> +	int ret;
> +
> +	writeback_conn = kzalloc(sizeof(*writeback_conn), GFP_KERNEL);
> +	if (!writeback_conn)
> +		return -ENOSPC;
> +
> +	encoder = &writeback_conn->encoder;
> +	encoder->base.possible_crtcs = 0xf;
> +	ret = drm_encoder_init(display->drm, &encoder->base,
> +			       &drm_writeback_encoder_funcs,
> +			       DRM_MODE_ENCODER_VIRTUAL, NULL);
> +	if (ret) {
> +		kfree(writeback_conn);
> +		return ret;
> +	}
> +
> +	encoder->type = INTEL_OUTPUT_WRITEBACK;
> +	encoder->pipe_mask = ~0;
> +	encoder->cloneable = 0;
> +
> +	connector = &writeback_conn->connector;
> +	ret = intel_writeback_connector_alloc(connector);
> +	if (ret) {
> +		kfree(writeback_conn);
> +		return ret;
> +	}
> +
> +	connector->base.interlace_allowed = 0;
> +	drm_connector_helper_add(&connector->base, &conn_helper_funcs);
> +	ret = drm_writeback_connector_init(display->drm, &connector->base,
> +					   &conn_funcs, &encoder->base,
> +					   writeback_formats,
> +					   ARRAY_SIZE(writeback_formats));

Probably means that we have to call drm_writeback_connector_cleanup() 
from somewhere, which is missing currently.

> +	if (ret) {
> +		intel_connector_free(connector);
> +		drm_encoder_cleanup(&encoder->base);
> +		kfree(&writeback_conn->encoder);
> +		kfree(writeback_conn);
> +		return ret;
> +	}

would be nice to follow the standard pattern of goto err instead of 
having this code block here.

> +
> +	intel_connector_attach_encoder(connector, encoder);
> +	connector->get_hw_state = intel_connector_get_hw_state;
> +	connector->base.status = connector_status_disconnected;
> +	writeback_conn->frame_num = 1;
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_writeback.h b/drivers/gpu/drm/i915/display/intel_writeback.h
> new file mode 100644
> index 000000000000..5911684cb81a
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_writeback.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2025 Intel Corporation
> + */
> +
> +#ifndef __INTEL_WRITEBACK_H__
> +#define __INTEL_WRITEBACK_H__
> +
> +#include <linux/types.h>
> +
> +struct intel_display;
> +struct intel_writeback_connector;
> +
> +int intel_writeback_init(struct intel_display *display);
> +
> +#endif /* __INTEL_WRITEBACK_H__ */
> +