Re: [Freedreno] [PATCH v6 4/4] drm: allow real encoder to be passed for drm_writeback_connector
Hi Laurent
On 4/5/2022 10:08 AM, Abhinav Kumar wrote:
Hi Laurent
On 4/5/2022 10:02 AM, Laurent Pinchart wrote:
Hi Abhinav,
On Tue, Apr 05, 2022 at 09:53:57AM -0700, Abhinav Kumar wrote:
On 4/5/2022 9:47 AM, Laurent Pinchart wrote:
On Mon, Apr 04, 2022 at 11:43:37AM -0700, Rob Clark wrote:
On Fri, Apr 1, 2022 at 8:38 AM Laurent Pinchart wrote:
On Thu, Mar 31, 2022 at 05:12:13PM -0700, Abhinav Kumar wrote:
For some vendor driver implementations, display hardware can
be shared between the encoder used for writeback and the physical
display.
In addition resources such as clocks and interrupts can
also be shared between writeback and the real encoder.
To accommodate such vendor drivers and hardware, allow
real encoder to be passed for drm_writeback_connector.
changes in v6:
- assign the encoder inside
drm_writeback_connector_init_with_encoder() for
better readability
- improve some documentation for internal encoder
Co-developed-by: Kandpal Suraj
Signed-off-by: Kandpal Suraj
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/drm_writeback.c | 18 --
drivers/gpu/drm/vc4/vc4_txp.c | 14 --
include/drm/drm_writeback.h | 21 +++--
Please split this in two patches, one for the DRM core and one for
the
VC4 driver. This applies to most patches as a general rule, with the
main exception being API refactoring that requires changing the
implementation and all its users in a single patch.
But this *is* API refactoring ;-)
Partly at least :-) Looking at the API change itself, wouldn't we
minimize the extra changes to vc4 if we moved this patch before 3/4 ?
I can move all the changes done in vc4 except below part to the change
3/4 itself because that way I can show usage of vc4->drm_enc with the
new API. Let me know if that works.
What I meant is moving the API refactoring from 4/4 before the current
3/4, with minimal changes to vc4 there (only to avoid introducing a
bisection breakge), and then move most of the vc4 changes from this
patch to the current 3/4 (which will become 4/4). If that's what you
meant too, it sounds good to me.
The API refactoring part in this patch is tied closely with changing the
wb_connector's encoder to a pointer which breaks vc4.
I have not made any additional refactoring changes here.
So I am not sure how to decouple this more.
Thanks
Abhinav
Looking at this more, even if we split the patch to smaller set of core
changes, we might be able to make them individually compile but
functionality will be broken because the wb_connector->encoder has to be
a valid one and if we break the core functionality further it will break
this.
@@ -238,6 +238,12 @@ int
drm_writeback_connector_init_with_encoder(struct drm_device *dev,
struct drm_mode_config *config = >mode_config;
int ret = create_writeback_properties(dev);
+ /*
+* Assign the encoder passed to this API to the wb_connector's encoder.
+* For drm_writeback_connector_init(), this shall be the
internal_encoder
+*/
+ wb_connector->encoder = enc;
+
if (ret != 0)
return ret;
So to keep both compilation and functionality intact at every change, i
am not sure how to break this up more.
The only part which will remain is the below one:
@@ -523,7 +525,7 @@ static int vc4_txp_bind(struct device *dev, struct
device *master, void *data)
if (ret)
return ret;
- encoder = >connector.encoder;
+ encoder = txp->connector.encoder;
encoder->possible_crtcs = drm_crtc_mask(crtc);
Since i dont know vc4 driver very well, I was not sure of a good way to
decouple this dependency.
Let me know if that works.
3 files changed, 39 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c
b/drivers/gpu/drm/drm_writeback.c
index 797223c..7f72109 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -179,21 +179,21 @@ int drm_writeback_connector_init(struct
drm_device *dev,
{
int ret = 0;
- drm_encoder_helper_add(_connector->encoder,
enc_helper_funcs);
+ drm_encoder_helper_add(_connector->internal_encoder,
enc_helper_funcs);
- wb_connector->encoder.possible_crtcs = possible_crtcs;
+ wb_connector->internal_encoder.possible_crtcs =
possible_crtcs;
- ret = drm_encoder_init(dev, _connector->encoder,
+ ret = drm_encoder_init(dev, _connector->internal_encoder,
_writeback_encoder_funcs,
DRM_MODE_ENCODER_VIRTUAL, NULL);
if (ret)
return ret;
- ret = drm_writeback_connector_init_with_encoder(dev,
wb_connector, _connector->encoder,
- con_funcs, formats, n_formats);
+ ret = drm_writeback_connector_init_with_encoder(dev,
wb_connector,
+ _connector->internal_encoder, con_funcs,
formats, n_formats);
Re: [Freedreno] [PATCH v6 4/4] drm: allow real encoder to be passed for drm_writeback_connector
Hi Laurent
On 4/5/2022 10:02 AM, Laurent Pinchart wrote:
Hi Abhinav,
On Tue, Apr 05, 2022 at 09:53:57AM -0700, Abhinav Kumar wrote:
On 4/5/2022 9:47 AM, Laurent Pinchart wrote:
On Mon, Apr 04, 2022 at 11:43:37AM -0700, Rob Clark wrote:
On Fri, Apr 1, 2022 at 8:38 AM Laurent Pinchart wrote:
On Thu, Mar 31, 2022 at 05:12:13PM -0700, Abhinav Kumar wrote:
For some vendor driver implementations, display hardware can
be shared between the encoder used for writeback and the physical
display.
In addition resources such as clocks and interrupts can
also be shared between writeback and the real encoder.
To accommodate such vendor drivers and hardware, allow
real encoder to be passed for drm_writeback_connector.
changes in v6:
- assign the encoder inside
drm_writeback_connector_init_with_encoder() for
better readability
- improve some documentation for internal encoder
Co-developed-by: Kandpal Suraj
Signed-off-by: Kandpal Suraj
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/drm_writeback.c | 18 --
drivers/gpu/drm/vc4/vc4_txp.c | 14 --
include/drm/drm_writeback.h | 21 +++--
Please split this in two patches, one for the DRM core and one for the
VC4 driver. This applies to most patches as a general rule, with the
main exception being API refactoring that requires changing the
implementation and all its users in a single patch.
But this *is* API refactoring ;-)
Partly at least :-) Looking at the API change itself, wouldn't we
minimize the extra changes to vc4 if we moved this patch before 3/4 ?
I can move all the changes done in vc4 except below part to the change
3/4 itself because that way I can show usage of vc4->drm_enc with the
new API. Let me know if that works.
What I meant is moving the API refactoring from 4/4 before the current
3/4, with minimal changes to vc4 there (only to avoid introducing a
bisection breakge), and then move most of the vc4 changes from this
patch to the current 3/4 (which will become 4/4). If that's what you
meant too, it sounds good to me.
The API refactoring part in this patch is tied closely with changing the
wb_connector's encoder to a pointer which breaks vc4.
I have not made any additional refactoring changes here.
So I am not sure how to decouple this more.
Thanks
Abhinav
The only part which will remain is the below one:
@@ -523,7 +525,7 @@ static int vc4_txp_bind(struct device *dev, struct
device *master, void *data)
if (ret)
return ret;
- encoder = >connector.encoder;
+ encoder = txp->connector.encoder;
encoder->possible_crtcs = drm_crtc_mask(crtc);
Since i dont know vc4 driver very well, I was not sure of a good way to
decouple this dependency.
Let me know if that works.
3 files changed, 39 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index 797223c..7f72109 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -179,21 +179,21 @@ int drm_writeback_connector_init(struct drm_device *dev,
{
int ret = 0;
- drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
+ drm_encoder_helper_add(_connector->internal_encoder, enc_helper_funcs);
- wb_connector->encoder.possible_crtcs = possible_crtcs;
+ wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
- ret = drm_encoder_init(dev, _connector->encoder,
+ ret = drm_encoder_init(dev, _connector->internal_encoder,
_writeback_encoder_funcs,
DRM_MODE_ENCODER_VIRTUAL, NULL);
if (ret)
return ret;
- ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
_connector->encoder,
- con_funcs, formats, n_formats);
+ ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
+ _connector->internal_encoder, con_funcs, formats,
n_formats);
if (ret)
- drm_encoder_cleanup(_connector->encoder);
+ drm_encoder_cleanup(_connector->internal_encoder);
return ret;
}
@@ -238,6 +238,12 @@ int drm_writeback_connector_init_with_encoder(struct
drm_device *dev,
struct drm_mode_config *config = >mode_config;
int ret = create_writeback_properties(dev);
+ /*
+ * Assign the encoder passed to this API to the wb_connector's encoder.
+ * For drm_writeback_connector_init(), this shall be the internal_encoder
+ */
+ wb_connector->encoder = enc;
+
if (ret != 0)
return ret;
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index 5e53f02..a9b4f83 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -151,6 +151,8 @@ struct vc4_txp {
struct platform_device *pdev;
+ struct drm_encoder drm_enc;
+
struct
Re: [Freedreno] [PATCH v6 4/4] drm: allow real encoder to be passed for drm_writeback_connector
Hi Abhinav,
On Tue, Apr 05, 2022 at 09:53:57AM -0700, Abhinav Kumar wrote:
> On 4/5/2022 9:47 AM, Laurent Pinchart wrote:
> > On Mon, Apr 04, 2022 at 11:43:37AM -0700, Rob Clark wrote:
> >> On Fri, Apr 1, 2022 at 8:38 AM Laurent Pinchart wrote:
> >>> On Thu, Mar 31, 2022 at 05:12:13PM -0700, Abhinav Kumar wrote:
> For some vendor driver implementations, display hardware can
> be shared between the encoder used for writeback and the physical
> display.
>
> In addition resources such as clocks and interrupts can
> also be shared between writeback and the real encoder.
>
> To accommodate such vendor drivers and hardware, allow
> real encoder to be passed for drm_writeback_connector.
>
> changes in v6:
> - assign the encoder inside
> drm_writeback_connector_init_with_encoder() for
> better readability
> - improve some documentation for internal encoder
>
> Co-developed-by: Kandpal Suraj
> Signed-off-by: Kandpal Suraj
> Signed-off-by: Abhinav Kumar
> ---
> drivers/gpu/drm/drm_writeback.c | 18 --
> drivers/gpu/drm/vc4/vc4_txp.c | 14 --
> include/drm/drm_writeback.h | 21 +++--
> >>>
> >>> Please split this in two patches, one for the DRM core and one for the
> >>> VC4 driver. This applies to most patches as a general rule, with the
> >>> main exception being API refactoring that requires changing the
> >>> implementation and all its users in a single patch.
> >>
> >> But this *is* API refactoring ;-)
> >
> > Partly at least :-) Looking at the API change itself, wouldn't we
> > minimize the extra changes to vc4 if we moved this patch before 3/4 ?
>
> I can move all the changes done in vc4 except below part to the change
> 3/4 itself because that way I can show usage of vc4->drm_enc with the
> new API. Let me know if that works.
What I meant is moving the API refactoring from 4/4 before the current
3/4, with minimal changes to vc4 there (only to avoid introducing a
bisection breakge), and then move most of the vc4 changes from this
patch to the current 3/4 (which will become 4/4). If that's what you
meant too, it sounds good to me.
> The only part which will remain is the below one:
>
> @@ -523,7 +525,7 @@ static int vc4_txp_bind(struct device *dev, struct
> device *master, void *data)
> if (ret)
> return ret;
>
> - encoder = >connector.encoder;
> + encoder = txp->connector.encoder;
> encoder->possible_crtcs = drm_crtc_mask(crtc);
>
> Since i dont know vc4 driver very well, I was not sure of a good way to
> decouple this dependency.
>
> Let me know if that works.
>
> 3 files changed, 39 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_writeback.c
> b/drivers/gpu/drm/drm_writeback.c
> index 797223c..7f72109 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -179,21 +179,21 @@ int drm_writeback_connector_init(struct drm_device
> *dev,
> {
> int ret = 0;
>
> - drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
> + drm_encoder_helper_add(_connector->internal_encoder,
> enc_helper_funcs);
>
> - wb_connector->encoder.possible_crtcs = possible_crtcs;
> + wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
>
> - ret = drm_encoder_init(dev, _connector->encoder,
> + ret = drm_encoder_init(dev, _connector->internal_encoder,
> _writeback_encoder_funcs,
> DRM_MODE_ENCODER_VIRTUAL, NULL);
> if (ret)
> return ret;
>
> - ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
> _connector->encoder,
> - con_funcs, formats, n_formats);
> + ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
> + _connector->internal_encoder, con_funcs,
> formats, n_formats);
>
> if (ret)
> - drm_encoder_cleanup(_connector->encoder);
> + drm_encoder_cleanup(_connector->internal_encoder);
>
> return ret;
> }
> @@ -238,6 +238,12 @@ int
> drm_writeback_connector_init_with_encoder(struct drm_device *dev,
> struct drm_mode_config *config = >mode_config;
> int ret = create_writeback_properties(dev);
>
> + /*
> + * Assign the encoder passed to this API to the wb_connector's
> encoder.
> + * For drm_writeback_connector_init(), this shall be the
> internal_encoder
> + */
> + wb_connector->encoder = enc;
> +
> if (ret != 0)
> return ret;
>
> diff --git
Re: [Freedreno] [PATCH v6 4/4] drm: allow real encoder to be passed for drm_writeback_connector
Hi Laurent
On 4/5/2022 9:47 AM, Laurent Pinchart wrote:
Hi Rob and Abhinav,
On Mon, Apr 04, 2022 at 11:43:37AM -0700, Rob Clark wrote:
On Fri, Apr 1, 2022 at 8:38 AM Laurent Pinchart wrote:
On Thu, Mar 31, 2022 at 05:12:13PM -0700, Abhinav Kumar wrote:
For some vendor driver implementations, display hardware can
be shared between the encoder used for writeback and the physical
display.
In addition resources such as clocks and interrupts can
also be shared between writeback and the real encoder.
To accommodate such vendor drivers and hardware, allow
real encoder to be passed for drm_writeback_connector.
changes in v6:
- assign the encoder inside
drm_writeback_connector_init_with_encoder() for
better readability
- improve some documentation for internal encoder
Co-developed-by: Kandpal Suraj
Signed-off-by: Kandpal Suraj
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/drm_writeback.c | 18 --
drivers/gpu/drm/vc4/vc4_txp.c | 14 --
include/drm/drm_writeback.h | 21 +++--
Please split this in two patches, one for the DRM core and one for the
VC4 driver. This applies to most patches as a general rule, with the
main exception being API refactoring that requires changing the
implementation and all its users in a single patch.
But this *is* API refactoring ;-)
Partly at least :-) Looking at the API change itself, wouldn't we
minimize the extra changes to vc4 if we moved this patch before 3/4 ?
I can move all the changes done in vc4 except below part to the change
3/4 itself because that way I can show usage of vc4->drm_enc with the
new API. Let me know if that works.
The only part which will remain is the below one:
@@ -523,7 +525,7 @@ static int vc4_txp_bind(struct device *dev, struct
device *master, void *data)
if (ret)
return ret;
- encoder = >connector.encoder;
+ encoder = txp->connector.encoder;
encoder->possible_crtcs = drm_crtc_mask(crtc);
Since i dont know vc4 driver very well, I was not sure of a good way to
decouple this dependency.
Let me know if that works.
Thanks
Abhinav
3 files changed, 39 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index 797223c..7f72109 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -179,21 +179,21 @@ int drm_writeback_connector_init(struct drm_device *dev,
{
int ret = 0;
- drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
+ drm_encoder_helper_add(_connector->internal_encoder, enc_helper_funcs);
- wb_connector->encoder.possible_crtcs = possible_crtcs;
+ wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
- ret = drm_encoder_init(dev, _connector->encoder,
+ ret = drm_encoder_init(dev, _connector->internal_encoder,
_writeback_encoder_funcs,
DRM_MODE_ENCODER_VIRTUAL, NULL);
if (ret)
return ret;
- ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
_connector->encoder,
- con_funcs, formats, n_formats);
+ ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
+ _connector->internal_encoder, con_funcs, formats,
n_formats);
if (ret)
- drm_encoder_cleanup(_connector->encoder);
+ drm_encoder_cleanup(_connector->internal_encoder);
return ret;
}
@@ -238,6 +238,12 @@ int drm_writeback_connector_init_with_encoder(struct
drm_device *dev,
struct drm_mode_config *config = >mode_config;
int ret = create_writeback_properties(dev);
+ /*
+ * Assign the encoder passed to this API to the wb_connector's encoder.
+ * For drm_writeback_connector_init(), this shall be the internal_encoder
+ */
+ wb_connector->encoder = enc;
+
if (ret != 0)
return ret;
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index 5e53f02..a9b4f83 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -151,6 +151,8 @@ struct vc4_txp {
struct platform_device *pdev;
+ struct drm_encoder drm_enc;
+
struct drm_writeback_connector connector;
void __iomem *regs;
@@ -159,7 +161,7 @@ struct vc4_txp {
static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
{
- return container_of(encoder, struct vc4_txp, connector.encoder);
+ return container_of(encoder, struct vc4_txp, drm_enc);
}
static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn)
@@ -499,9 +501,9 @@ static int vc4_txp_bind(struct device *dev, struct device
*master, void *data)
wb_conn = >connector;
- drm_encoder_helper_add(_conn->encoder, _txp_encoder_helper_funcs);
+ drm_encoder_helper_add(>drm_enc, _txp_encoder_helper_funcs);
-
Re: [PATCH v6 4/4] drm: allow real encoder to be passed for drm_writeback_connector
Hi Rob and Abhinav,
On Mon, Apr 04, 2022 at 11:43:37AM -0700, Rob Clark wrote:
> On Fri, Apr 1, 2022 at 8:38 AM Laurent Pinchart wrote:
> > On Thu, Mar 31, 2022 at 05:12:13PM -0700, Abhinav Kumar wrote:
> > > For some vendor driver implementations, display hardware can
> > > be shared between the encoder used for writeback and the physical
> > > display.
> > >
> > > In addition resources such as clocks and interrupts can
> > > also be shared between writeback and the real encoder.
> > >
> > > To accommodate such vendor drivers and hardware, allow
> > > real encoder to be passed for drm_writeback_connector.
> > >
> > > changes in v6:
> > > - assign the encoder inside
> > > drm_writeback_connector_init_with_encoder() for
> > > better readability
> > > - improve some documentation for internal encoder
> > >
> > > Co-developed-by: Kandpal Suraj
> > > Signed-off-by: Kandpal Suraj
> > > Signed-off-by: Abhinav Kumar
> > > ---
> > > drivers/gpu/drm/drm_writeback.c | 18 --
> > > drivers/gpu/drm/vc4/vc4_txp.c | 14 --
> > > include/drm/drm_writeback.h | 21 +++--
> >
> > Please split this in two patches, one for the DRM core and one for the
> > VC4 driver. This applies to most patches as a general rule, with the
> > main exception being API refactoring that requires changing the
> > implementation and all its users in a single patch.
>
> But this *is* API refactoring ;-)
Partly at least :-) Looking at the API change itself, wouldn't we
minimize the extra changes to vc4 if we moved this patch before 3/4 ?
> > > 3 files changed, 39 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_writeback.c
> > > b/drivers/gpu/drm/drm_writeback.c
> > > index 797223c..7f72109 100644
> > > --- a/drivers/gpu/drm/drm_writeback.c
> > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > @@ -179,21 +179,21 @@ int drm_writeback_connector_init(struct drm_device
> > > *dev,
> > > {
> > > int ret = 0;
> > >
> > > - drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
> > > + drm_encoder_helper_add(_connector->internal_encoder,
> > > enc_helper_funcs);
> > >
> > > - wb_connector->encoder.possible_crtcs = possible_crtcs;
> > > + wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
> > >
> > > - ret = drm_encoder_init(dev, _connector->encoder,
> > > + ret = drm_encoder_init(dev, _connector->internal_encoder,
> > > _writeback_encoder_funcs,
> > > DRM_MODE_ENCODER_VIRTUAL, NULL);
> > > if (ret)
> > > return ret;
> > >
> > > - ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
> > > _connector->encoder,
> > > - con_funcs, formats, n_formats);
> > > + ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
> > > + _connector->internal_encoder, con_funcs,
> > > formats, n_formats);
> > >
> > > if (ret)
> > > - drm_encoder_cleanup(_connector->encoder);
> > > + drm_encoder_cleanup(_connector->internal_encoder);
> > >
> > > return ret;
> > > }
> > > @@ -238,6 +238,12 @@ int drm_writeback_connector_init_with_encoder(struct
> > > drm_device *dev,
> > > struct drm_mode_config *config = >mode_config;
> > > int ret = create_writeback_properties(dev);
> > >
> > > + /*
> > > + * Assign the encoder passed to this API to the wb_connector's
> > > encoder.
> > > + * For drm_writeback_connector_init(), this shall be the
> > > internal_encoder
> > > + */
> > > + wb_connector->encoder = enc;
> > > +
> > > if (ret != 0)
> > > return ret;
> > >
> > > diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
> > > index 5e53f02..a9b4f83 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_txp.c
> > > +++ b/drivers/gpu/drm/vc4/vc4_txp.c
> > > @@ -151,6 +151,8 @@ struct vc4_txp {
> > >
> > > struct platform_device *pdev;
> > >
> > > + struct drm_encoder drm_enc;
> > > +
> > > struct drm_writeback_connector connector;
> > >
> > > void __iomem *regs;
> > > @@ -159,7 +161,7 @@ struct vc4_txp {
> > >
> > > static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder
> > > *encoder)
> > > {
> > > - return container_of(encoder, struct vc4_txp, connector.encoder);
> > > + return container_of(encoder, struct vc4_txp, drm_enc);
> > > }
> > >
> > > static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector
> > > *conn)
> > > @@ -499,9 +501,9 @@ static int vc4_txp_bind(struct device *dev, struct
> > > device *master, void *data)
> > >
> > > wb_conn = >connector;
> > >
> > > - drm_encoder_helper_add(_conn->encoder,
> > > _txp_encoder_helper_funcs);
> > > + drm_encoder_helper_add(>drm_enc,
> > > _txp_encoder_helper_funcs);
> > >
> > > - ret = drm_encoder_init(drm, _conn->encoder,
> > > + ret =
Re: [PATCH v6 4/4] drm: allow real encoder to be passed for drm_writeback_connector
On Fri, Apr 1, 2022 at 8:38 AM Laurent Pinchart
wrote:
>
> Hi Abhinav,
>
> Thank you for the patch.
>
> On Thu, Mar 31, 2022 at 05:12:13PM -0700, Abhinav Kumar wrote:
> > For some vendor driver implementations, display hardware can
> > be shared between the encoder used for writeback and the physical
> > display.
> >
> > In addition resources such as clocks and interrupts can
> > also be shared between writeback and the real encoder.
> >
> > To accommodate such vendor drivers and hardware, allow
> > real encoder to be passed for drm_writeback_connector.
> >
> > changes in v6:
> > - assign the encoder inside
> > drm_writeback_connector_init_with_encoder() for
> > better readability
> > - improve some documentation for internal encoder
> >
> > Co-developed-by: Kandpal Suraj
> > Signed-off-by: Kandpal Suraj
> > Signed-off-by: Abhinav Kumar
> > ---
> > drivers/gpu/drm/drm_writeback.c | 18 --
> > drivers/gpu/drm/vc4/vc4_txp.c | 14 --
> > include/drm/drm_writeback.h | 21 +++--
>
> Please split this in two patches, one for the DRM core and one for the
> VC4 driver. This applies to most patches as a general rule, with the
> main exception being API refactoring that requires changing the
> implementation and all its users in a single patch.
But this *is* API refactoring ;-)
BR,
-R
> > 3 files changed, 39 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_writeback.c
> > b/drivers/gpu/drm/drm_writeback.c
> > index 797223c..7f72109 100644
> > --- a/drivers/gpu/drm/drm_writeback.c
> > +++ b/drivers/gpu/drm/drm_writeback.c
> > @@ -179,21 +179,21 @@ int drm_writeback_connector_init(struct drm_device
> > *dev,
> > {
> > int ret = 0;
> >
> > - drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
> > + drm_encoder_helper_add(_connector->internal_encoder,
> > enc_helper_funcs);
> >
> > - wb_connector->encoder.possible_crtcs = possible_crtcs;
> > + wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
> >
> > - ret = drm_encoder_init(dev, _connector->encoder,
> > + ret = drm_encoder_init(dev, _connector->internal_encoder,
> > _writeback_encoder_funcs,
> > DRM_MODE_ENCODER_VIRTUAL, NULL);
> > if (ret)
> > return ret;
> >
> > - ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
> > _connector->encoder,
> > - con_funcs, formats, n_formats);
> > + ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
> > + _connector->internal_encoder, con_funcs, formats,
> > n_formats);
> >
> > if (ret)
> > - drm_encoder_cleanup(_connector->encoder);
> > + drm_encoder_cleanup(_connector->internal_encoder);
> >
> > return ret;
> > }
> > @@ -238,6 +238,12 @@ int drm_writeback_connector_init_with_encoder(struct
> > drm_device *dev,
> > struct drm_mode_config *config = >mode_config;
> > int ret = create_writeback_properties(dev);
> >
> > + /*
> > + * Assign the encoder passed to this API to the wb_connector's
> > encoder.
> > + * For drm_writeback_connector_init(), this shall be the
> > internal_encoder
> > + */
> > + wb_connector->encoder = enc;
> > +
> > if (ret != 0)
> > return ret;
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
> > index 5e53f02..a9b4f83 100644
> > --- a/drivers/gpu/drm/vc4/vc4_txp.c
> > +++ b/drivers/gpu/drm/vc4/vc4_txp.c
> > @@ -151,6 +151,8 @@ struct vc4_txp {
> >
> > struct platform_device *pdev;
> >
> > + struct drm_encoder drm_enc;
> > +
> > struct drm_writeback_connector connector;
> >
> > void __iomem *regs;
> > @@ -159,7 +161,7 @@ struct vc4_txp {
> >
> > static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder
> > *encoder)
> > {
> > - return container_of(encoder, struct vc4_txp, connector.encoder);
> > + return container_of(encoder, struct vc4_txp, drm_enc);
> > }
> >
> > static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector
> > *conn)
> > @@ -499,9 +501,9 @@ static int vc4_txp_bind(struct device *dev, struct
> > device *master, void *data)
> >
> > wb_conn = >connector;
> >
> > - drm_encoder_helper_add(_conn->encoder,
> > _txp_encoder_helper_funcs);
> > + drm_encoder_helper_add(>drm_enc, _txp_encoder_helper_funcs);
> >
> > - ret = drm_encoder_init(drm, _conn->encoder,
> > + ret = drm_encoder_init(drm, >drm_enc,
> > _txp_encoder_funcs,
> > DRM_MODE_ENCODER_VIRTUAL, NULL);
> >
> > @@ -511,10 +513,10 @@ static int vc4_txp_bind(struct device *dev, struct
> > device *master, void *data)
> > drm_connector_helper_add(_conn->base,
> >_txp_connector_helper_funcs);
> >
> > - ret =
Re: [PATCH v6 4/4] drm: allow real encoder to be passed for drm_writeback_connector
Hi Laurent
Thanks for the review.
One question below.
On 4/1/2022 8:38 AM, Laurent Pinchart wrote:
Hi Abhinav,
Thank you for the patch.
On Thu, Mar 31, 2022 at 05:12:13PM -0700, Abhinav Kumar wrote:
For some vendor driver implementations, display hardware can
be shared between the encoder used for writeback and the physical
display.
In addition resources such as clocks and interrupts can
also be shared between writeback and the real encoder.
To accommodate such vendor drivers and hardware, allow
real encoder to be passed for drm_writeback_connector.
changes in v6:
- assign the encoder inside
drm_writeback_connector_init_with_encoder() for
better readability
- improve some documentation for internal encoder
Co-developed-by: Kandpal Suraj
Signed-off-by: Kandpal Suraj
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/drm_writeback.c | 18 --
drivers/gpu/drm/vc4/vc4_txp.c | 14 --
include/drm/drm_writeback.h | 21 +++--
Please split this in two patches, one for the DRM core and one for the
VC4 driver. This applies to most patches as a general rule, with the
main exception being API refactoring that requires changing the
implementation and all its users in a single patch.
I also wanted to do that but that would break compilation of this change
because of vc4 (explained below) so I had to club it in this patch.
If you can provide some suggestion on how to address vc4's dependency I
can break this up.
3 files changed, 39 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index 797223c..7f72109 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -179,21 +179,21 @@ int drm_writeback_connector_init(struct drm_device *dev,
{
int ret = 0;
- drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
+ drm_encoder_helper_add(_connector->internal_encoder,
enc_helper_funcs);
- wb_connector->encoder.possible_crtcs = possible_crtcs;
+ wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
- ret = drm_encoder_init(dev, _connector->encoder,
+ ret = drm_encoder_init(dev, _connector->internal_encoder,
_writeback_encoder_funcs,
DRM_MODE_ENCODER_VIRTUAL, NULL);
if (ret)
return ret;
- ret = drm_writeback_connector_init_with_encoder(dev, wb_connector, _connector->encoder,
- con_funcs, formats, n_formats);
+ ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
+ _connector->internal_encoder, con_funcs, formats,
n_formats);
if (ret)
- drm_encoder_cleanup(_connector->encoder);
+ drm_encoder_cleanup(_connector->internal_encoder);
return ret;
}
@@ -238,6 +238,12 @@ int drm_writeback_connector_init_with_encoder(struct
drm_device *dev,
struct drm_mode_config *config = >mode_config;
int ret = create_writeback_properties(dev);
+ /*
+* Assign the encoder passed to this API to the wb_connector's encoder.
+* For drm_writeback_connector_init(), this shall be the
internal_encoder
+*/
+ wb_connector->encoder = enc;
+
if (ret != 0)
return ret;
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index 5e53f02..a9b4f83 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -151,6 +151,8 @@ struct vc4_txp {
struct platform_device *pdev;
+ struct drm_encoder drm_enc;
+
struct drm_writeback_connector connector;
void __iomem *regs;
@@ -159,7 +161,7 @@ struct vc4_txp {
static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
{
- return container_of(encoder, struct vc4_txp, connector.encoder);
vc4 directly references connector.encoder here.
+ return container_of(encoder, struct vc4_txp, drm_enc);
}
static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn)
@@ -499,9 +501,9 @@ static int vc4_txp_bind(struct device *dev, struct device
*master, void *data)
wb_conn = >connector;
- drm_encoder_helper_add(_conn->encoder, _txp_encoder_helper_funcs);
+ drm_encoder_helper_add(>drm_enc, _txp_encoder_helper_funcs);
- ret = drm_encoder_init(drm, _conn->encoder,
+ ret = drm_encoder_init(drm, >drm_enc,
_txp_encoder_funcs,
DRM_MODE_ENCODER_VIRTUAL, NULL);
@@ -511,10 +513,10 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
drm_connector_helper_add(_conn->base,
_txp_connector_helper_funcs);
- ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, _conn->encoder,
+ ret = drm_writeback_connector_init_with_encoder(drm, wb_conn,
Re: [PATCH v6 4/4] drm: allow real encoder to be passed for drm_writeback_connector
Hi Abhinav,
Thank you for the patch.
On Thu, Mar 31, 2022 at 05:12:13PM -0700, Abhinav Kumar wrote:
> For some vendor driver implementations, display hardware can
> be shared between the encoder used for writeback and the physical
> display.
>
> In addition resources such as clocks and interrupts can
> also be shared between writeback and the real encoder.
>
> To accommodate such vendor drivers and hardware, allow
> real encoder to be passed for drm_writeback_connector.
>
> changes in v6:
> - assign the encoder inside
> drm_writeback_connector_init_with_encoder() for
> better readability
> - improve some documentation for internal encoder
>
> Co-developed-by: Kandpal Suraj
> Signed-off-by: Kandpal Suraj
> Signed-off-by: Abhinav Kumar
> ---
> drivers/gpu/drm/drm_writeback.c | 18 --
> drivers/gpu/drm/vc4/vc4_txp.c | 14 --
> include/drm/drm_writeback.h | 21 +++--
Please split this in two patches, one for the DRM core and one for the
VC4 driver. This applies to most patches as a general rule, with the
main exception being API refactoring that requires changing the
implementation and all its users in a single patch.
> 3 files changed, 39 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index 797223c..7f72109 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -179,21 +179,21 @@ int drm_writeback_connector_init(struct drm_device *dev,
> {
> int ret = 0;
>
> - drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
> + drm_encoder_helper_add(_connector->internal_encoder,
> enc_helper_funcs);
>
> - wb_connector->encoder.possible_crtcs = possible_crtcs;
> + wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
>
> - ret = drm_encoder_init(dev, _connector->encoder,
> + ret = drm_encoder_init(dev, _connector->internal_encoder,
> _writeback_encoder_funcs,
> DRM_MODE_ENCODER_VIRTUAL, NULL);
> if (ret)
> return ret;
>
> - ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
> _connector->encoder,
> - con_funcs, formats, n_formats);
> + ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
> + _connector->internal_encoder, con_funcs, formats,
> n_formats);
>
> if (ret)
> - drm_encoder_cleanup(_connector->encoder);
> + drm_encoder_cleanup(_connector->internal_encoder);
>
> return ret;
> }
> @@ -238,6 +238,12 @@ int drm_writeback_connector_init_with_encoder(struct
> drm_device *dev,
> struct drm_mode_config *config = >mode_config;
> int ret = create_writeback_properties(dev);
>
> + /*
> + * Assign the encoder passed to this API to the wb_connector's encoder.
> + * For drm_writeback_connector_init(), this shall be the
> internal_encoder
> + */
> + wb_connector->encoder = enc;
> +
> if (ret != 0)
> return ret;
>
> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
> index 5e53f02..a9b4f83 100644
> --- a/drivers/gpu/drm/vc4/vc4_txp.c
> +++ b/drivers/gpu/drm/vc4/vc4_txp.c
> @@ -151,6 +151,8 @@ struct vc4_txp {
>
> struct platform_device *pdev;
>
> + struct drm_encoder drm_enc;
> +
> struct drm_writeback_connector connector;
>
> void __iomem *regs;
> @@ -159,7 +161,7 @@ struct vc4_txp {
>
> static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
> {
> - return container_of(encoder, struct vc4_txp, connector.encoder);
> + return container_of(encoder, struct vc4_txp, drm_enc);
> }
>
> static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector
> *conn)
> @@ -499,9 +501,9 @@ static int vc4_txp_bind(struct device *dev, struct device
> *master, void *data)
>
> wb_conn = >connector;
>
> - drm_encoder_helper_add(_conn->encoder,
> _txp_encoder_helper_funcs);
> + drm_encoder_helper_add(>drm_enc, _txp_encoder_helper_funcs);
>
> - ret = drm_encoder_init(drm, _conn->encoder,
> + ret = drm_encoder_init(drm, >drm_enc,
> _txp_encoder_funcs,
> DRM_MODE_ENCODER_VIRTUAL, NULL);
>
> @@ -511,10 +513,10 @@ static int vc4_txp_bind(struct device *dev, struct
> device *master, void *data)
> drm_connector_helper_add(_conn->base,
>_txp_connector_helper_funcs);
>
> - ret = drm_writeback_connector_init_with_encoder(drm, wb_conn,
> _conn->encoder,
> + ret = drm_writeback_connector_init_with_encoder(drm, wb_conn,
> >drm_enc,
> _txp_connector_funcs, drm_fmts,
> ARRAY_SIZE(drm_fmts));
> if (ret) {
> - drm_encoder_cleanup(_conn->encoder);
> + drm_encoder_cleanup(>drm_enc);
>
[PATCH v6 4/4] drm: allow real encoder to be passed for drm_writeback_connector
For some vendor driver implementations, display hardware can
be shared between the encoder used for writeback and the physical
display.
In addition resources such as clocks and interrupts can
also be shared between writeback and the real encoder.
To accommodate such vendor drivers and hardware, allow
real encoder to be passed for drm_writeback_connector.
changes in v6:
- assign the encoder inside
drm_writeback_connector_init_with_encoder() for
better readability
- improve some documentation for internal encoder
Co-developed-by: Kandpal Suraj
Signed-off-by: Kandpal Suraj
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/drm_writeback.c | 18 --
drivers/gpu/drm/vc4/vc4_txp.c | 14 --
include/drm/drm_writeback.h | 21 +++--
3 files changed, 39 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index 797223c..7f72109 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -179,21 +179,21 @@ int drm_writeback_connector_init(struct drm_device *dev,
{
int ret = 0;
- drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
+ drm_encoder_helper_add(_connector->internal_encoder,
enc_helper_funcs);
- wb_connector->encoder.possible_crtcs = possible_crtcs;
+ wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
- ret = drm_encoder_init(dev, _connector->encoder,
+ ret = drm_encoder_init(dev, _connector->internal_encoder,
_writeback_encoder_funcs,
DRM_MODE_ENCODER_VIRTUAL, NULL);
if (ret)
return ret;
- ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
_connector->encoder,
- con_funcs, formats, n_formats);
+ ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
+ _connector->internal_encoder, con_funcs, formats,
n_formats);
if (ret)
- drm_encoder_cleanup(_connector->encoder);
+ drm_encoder_cleanup(_connector->internal_encoder);
return ret;
}
@@ -238,6 +238,12 @@ int drm_writeback_connector_init_with_encoder(struct
drm_device *dev,
struct drm_mode_config *config = >mode_config;
int ret = create_writeback_properties(dev);
+ /*
+* Assign the encoder passed to this API to the wb_connector's encoder.
+* For drm_writeback_connector_init(), this shall be the
internal_encoder
+*/
+ wb_connector->encoder = enc;
+
if (ret != 0)
return ret;
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index 5e53f02..a9b4f83 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -151,6 +151,8 @@ struct vc4_txp {
struct platform_device *pdev;
+ struct drm_encoder drm_enc;
+
struct drm_writeback_connector connector;
void __iomem *regs;
@@ -159,7 +161,7 @@ struct vc4_txp {
static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
{
- return container_of(encoder, struct vc4_txp, connector.encoder);
+ return container_of(encoder, struct vc4_txp, drm_enc);
}
static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn)
@@ -499,9 +501,9 @@ static int vc4_txp_bind(struct device *dev, struct device
*master, void *data)
wb_conn = >connector;
- drm_encoder_helper_add(_conn->encoder,
_txp_encoder_helper_funcs);
+ drm_encoder_helper_add(>drm_enc, _txp_encoder_helper_funcs);
- ret = drm_encoder_init(drm, _conn->encoder,
+ ret = drm_encoder_init(drm, >drm_enc,
_txp_encoder_funcs,
DRM_MODE_ENCODER_VIRTUAL, NULL);
@@ -511,10 +513,10 @@ static int vc4_txp_bind(struct device *dev, struct device
*master, void *data)
drm_connector_helper_add(_conn->base,
_txp_connector_helper_funcs);
- ret = drm_writeback_connector_init_with_encoder(drm, wb_conn,
_conn->encoder,
+ ret = drm_writeback_connector_init_with_encoder(drm, wb_conn,
>drm_enc,
_txp_connector_funcs, drm_fmts,
ARRAY_SIZE(drm_fmts));
if (ret) {
- drm_encoder_cleanup(_conn->encoder);
+ drm_encoder_cleanup(>drm_enc);
return ret;
}
@@ -523,7 +525,7 @@ static int vc4_txp_bind(struct device *dev, struct device
*master, void *data)
if (ret)
return ret;
- encoder = >connector.encoder;
+ encoder = txp->connector.encoder;
encoder->possible_crtcs = drm_crtc_mask(crtc);
ret = devm_request_irq(dev, irq, vc4_txp_interrupt, 0,
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
index 4795024..3f5c330 100644
---
