Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1498946 > unrolled thread

[RFC PATCH 00/11] Introduce writeback connectors

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 00/11] Introduce writeback connectors Brian Starkey <brian.starkey@arm.com> - 2016-10-11 17:00 +0200
    [RFC PATCH 10/11] drm: mali-dp: Add support for writeback on DP550/DP650 Brian Starkey <brian.starkey@arm.com> - 2016-10-11 17:00 +0200
    [RFC PATCH 09/11] drm: mali-dp: Add RGB writeback formats for DP550/DP650 Brian Starkey <brian.starkey@arm.com> - 2016-10-11 17:00 +0200
    [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors Brian Starkey <brian.starkey@arm.com> - 2016-10-11 17:00 +0200
      Re: [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors Daniel Vetter <daniel@ffwll.ch> - 2016-10-11 17:50 +0200
        Re: [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors Brian Starkey <brian.starkey@arm.com> - 2016-10-11 18:50 +0200
          Re: [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors Daniel Vetter <daniel@ffwll.ch> - 2016-10-11 19:00 +0200
    [RFC PATCH 05/11] drm: Add fb to connector state Brian Starkey <brian.starkey@arm.com> - 2016-10-11 17:00 +0200
    [RFC PATCH 01/11] drm: Add writeback connector type Brian Starkey <brian.starkey@arm.com> - 2016-10-11 17:00 +0200
    [RFC PATCH 04/11] drm: Add __drm_framebuffer_remove_atomic Brian Starkey <brian.starkey@arm.com> - 2016-10-11 17:00 +0200
      Re: [RFC PATCH 04/11] drm: Add __drm_framebuffer_remove_atomic Daniel Vetter <daniel@ffwll.ch> - 2016-10-11 18:10 +0200
    [RFC PATCH 08/11] drm: mali-dp: Rename malidp_input_format Brian Starkey <brian.starkey@arm.com> - 2016-10-11 17:00 +0200
    [RFC PATCH 11/11] drm: mali-dp: Add writeback connector Brian Starkey <brian.starkey@arm.com> - 2016-10-11 17:00 +0200
    Re: [RFC PATCH 00/11] Introduce writeback connectors Daniel Vetter <daniel@ffwll.ch> - 2016-10-11 17:50 +0200
      Re: [RFC PATCH 00/11] Introduce writeback connectors Brian Starkey <brian.starkey@arm.com> - 2016-10-11 19:00 +0200
        Re: [RFC PATCH 00/11] Introduce writeback connectors Daniel Vetter <daniel@ffwll.ch> - 2016-10-11 19:10 +0200
          Re: [RFC PATCH 00/11] Introduce writeback connectors Brian Starkey <brian.starkey@arm.com> - 2016-10-11 21:50 +0200
            Re: [RFC PATCH 00/11] Introduce writeback connectors Daniel Vetter <daniel@ffwll.ch> - 2016-10-11 22:10 +0200
              Re: [RFC PATCH 00/11] Introduce writeback connectors Brian Starkey <brian.starkey@arm.com> - 2016-10-11 23:30 +0200
                Re: [RFC PATCH 00/11] Introduce writeback connectors Daniel Vetter <daniel@ffwll.ch> - 2016-10-12 09:00 +0200
                  [PATCH] drm: atomic: Clarify documentation around drm_atomic_crtc_needs_modeset Brian Starkey <brian.starkey@arm.com> - 2016-10-13 11:50 +0200
                    Re: [PATCH] drm: atomic: Clarify documentation around drm_atomic_crtc_needs_modeset Alex Deucher <alexdeucher@gmail.com> - 2016-10-13 17:00 +0200
                      Re: [PATCH] drm: atomic: Clarify documentation around  drm_atomic_crtc_needs_modeset Daniel Vetter <daniel@ffwll.ch> - 2016-10-17 08:30 +0200
    Re: [RFC PATCH 00/11] Introduce writeback connectors Daniel Vetter <daniel@ffwll.ch> - 2016-10-11 18:30 +0200
    Re: [RFC PATCH 00/11] Introduce writeback connectors Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-10-11 18:30 +0200
    Re: [RFC PATCH 00/11] Introduce writeback connectors Eric Anholt <eric@anholt.net> - 2016-10-11 21:10 +0200
      Re: [RFC PATCH 00/11] Introduce writeback connectors Brian Starkey <brian.starkey@arm.com> - 2016-10-12 09:40 +0200
        Re: [RFC PATCH 00/11] Introduce writeback connectors Eric Anholt <eric@anholt.net> - 2016-10-13 19:40 +0200
    Re: [RFC PATCH 00/11] Introduce writeback connectors Archit Taneja <architt@codeaurora.org> - 2016-10-14 13:00 +0200
      Re: [RFC PATCH 00/11] Introduce writeback connectors Brian Starkey <brian.starkey@arm.com> - 2016-10-14 14:40 +0200
        Re: [RFC PATCH 00/11] Introduce writeback connectors Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-10-14 15:00 +0200
        Re: [RFC PATCH 00/11] Introduce writeback connectors Daniel Vetter <daniel@ffwll.ch> - 2016-10-14 17:20 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1500128 — [PATCH] drm: atomic: Clarify documentation around drm_atomic_crtc_needs_modeset

Add some additional comments to more explicitly describe the meaning and
usage of the three CRTC modeset detection booleans: mode_changed,
connectors_changed and active_changed.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---

Hi Daniel,

I guess I asked for this one :-), please just check my understanding
is correct.

Thanks,
Brian

 drivers/gpu/drm/drm_atomic_helper.c |    9 +++++----
 include/drm/drm_atomic.h            |   11 ++++++++++-
 include/drm/drm_crtc.h              |    5 +++++
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 78ea735..fb4071a 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -458,10 +458,11 @@ mode_fixup(struct drm_atomic_state *state)
  * removed from the crtc.
  * crtc_state->active_changed is set when crtc_state->active changes,
  * which is used for dpms.
+ * See also: drm_atomic_crtc_needs_modeset()
  *
  * IMPORTANT:
  *
- * Drivers which update ->mode_changed (e.g. in their ->atomic_check hooks if a
+ * Drivers which set ->mode_changed (e.g. in their ->atomic_check hooks if a
  * plane update can't be done without a full modeset) _must_ call this function
  * afterwards after that change. It is permitted to call this function multiple
  * times for the same update, e.g. when the ->atomic_check functions depend upon
@@ -510,9 +511,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 
 	for_each_connector_in_state(state, connector, connector_state, i) {
 		/*
-		 * This only sets crtc->mode_changed for routing changes,
-		 * drivers must set crtc->mode_changed themselves when connector
-		 * properties need to be updated.
+		 * This only sets crtc->connectors_changed for routing changes,
+		 * drivers must set crtc->connectors_changed themselves when
+		 * connector properties need to be updated.
 		 */
 		ret = update_connector_routing(state, connector,
 					       connector_state);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index d9aff06..1ce255f 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -368,8 +368,17 @@ int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state);
  *
  * To give drivers flexibility struct &drm_crtc_state has 3 booleans to track
  * whether the state CRTC changed enough to need a full modeset cycle:
- * connectors_changed, mode_changed and active_change. This helper simply
+ * connectors_changed, mode_changed and active_changed. This helper simply
  * combines these three to compute the overall need for a modeset for @state.
+ *
+ * The atomic helper code sets these booleans, but drivers can and should
+ * change them appropriately to accurately represent whether a modeset is
+ * really needed. In general, drivers should avoid full modesets whenever
+ * possible.
+ *
+ * For example if the CRTC mode has changed, and the hardware is able to enact
+ * the requested mode change without going through a full modeset, the driver
+ * should clear mode_changed during its ->atomic_check.
  */
 static inline bool
 drm_atomic_crtc_needs_modeset(struct drm_crtc_state *state)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c4a3164..1f094d2 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -116,6 +116,11 @@ struct drm_plane_helper_funcs;
  * never return in a failure from the ->atomic_check callback. Userspace assumes
  * that a DPMS On will always succeed. In other words: @enable controls resource
  * assignment, @active controls the actual hardware state.
+ *
+ * The three booleans active_changed, connectors_changed and mode_changed are
+ * intended to indicate whether a full modeset is needed, rather than strictly
+ * describing what has changed in a commit.
+ * See also: drm_atomic_crtc_needs_modeset()
  */
 struct drm_crtc_state {
 	struct drm_crtc *crtc;
-- 
1.7.9.5

[toc] | [prev] | [next] | [standalone]

#1500319 — Re: [PATCH] drm: atomic: Clarify documentation around drm_atomic_crtc_needs_modeset

On Thu, Oct 13, 2016 at 5:47 AM, Brian Starkey <brian.starkey@arm.com> wrote:
> Add some additional comments to more explicitly describe the meaning and
> usage of the three CRTC modeset detection booleans: mode_changed,
> connectors_changed and active_changed.
>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> ---
>
> Hi Daniel,
>
> I guess I asked for this one :-), please just check my understanding
> is correct.
>

The whole thread was very enlightening for me with respect to those
flags as well.  The patch looks good to me.
Acked-by: Alex Deucher <alexander.deucher@amd.com>

> Thanks,
> Brian
>
>  drivers/gpu/drm/drm_atomic_helper.c |    9 +++++----
>  include/drm/drm_atomic.h            |   11 ++++++++++-
>  include/drm/drm_crtc.h              |    5 +++++
>  3 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 78ea735..fb4071a 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -458,10 +458,11 @@ mode_fixup(struct drm_atomic_state *state)
>   * removed from the crtc.
>   * crtc_state->active_changed is set when crtc_state->active changes,
>   * which is used for dpms.
> + * See also: drm_atomic_crtc_needs_modeset()
>   *
>   * IMPORTANT:
>   *
> - * Drivers which update ->mode_changed (e.g. in their ->atomic_check hooks if a
> + * Drivers which set ->mode_changed (e.g. in their ->atomic_check hooks if a
>   * plane update can't be done without a full modeset) _must_ call this function
>   * afterwards after that change. It is permitted to call this function multiple
>   * times for the same update, e.g. when the ->atomic_check functions depend upon
> @@ -510,9 +511,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>
>         for_each_connector_in_state(state, connector, connector_state, i) {
>                 /*
> -                * This only sets crtc->mode_changed for routing changes,
> -                * drivers must set crtc->mode_changed themselves when connector
> -                * properties need to be updated.
> +                * This only sets crtc->connectors_changed for routing changes,
> +                * drivers must set crtc->connectors_changed themselves when
> +                * connector properties need to be updated.
>                  */
>                 ret = update_connector_routing(state, connector,
>                                                connector_state);
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index d9aff06..1ce255f 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -368,8 +368,17 @@ int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state);
>   *
>   * To give drivers flexibility struct &drm_crtc_state has 3 booleans to track
>   * whether the state CRTC changed enough to need a full modeset cycle:
> - * connectors_changed, mode_changed and active_change. This helper simply
> + * connectors_changed, mode_changed and active_changed. This helper simply
>   * combines these three to compute the overall need for a modeset for @state.
> + *
> + * The atomic helper code sets these booleans, but drivers can and should
> + * change them appropriately to accurately represent whether a modeset is
> + * really needed. In general, drivers should avoid full modesets whenever
> + * possible.
> + *
> + * For example if the CRTC mode has changed, and the hardware is able to enact
> + * the requested mode change without going through a full modeset, the driver
> + * should clear mode_changed during its ->atomic_check.
>   */
>  static inline bool
>  drm_atomic_crtc_needs_modeset(struct drm_crtc_state *state)
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index c4a3164..1f094d2 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -116,6 +116,11 @@ struct drm_plane_helper_funcs;
>   * never return in a failure from the ->atomic_check callback. Userspace assumes
>   * that a DPMS On will always succeed. In other words: @enable controls resource
>   * assignment, @active controls the actual hardware state.
> + *
> + * The three booleans active_changed, connectors_changed and mode_changed are
> + * intended to indicate whether a full modeset is needed, rather than strictly
> + * describing what has changed in a commit.
> + * See also: drm_atomic_crtc_needs_modeset()
>   */
>  struct drm_crtc_state {
>         struct drm_crtc *crtc;
> --
> 1.7.9.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[toc] | [prev] | [next] | [standalone]

#1501625 — Re: [PATCH] drm: atomic: Clarify documentation around drm_atomic_crtc_needs_modeset

On Thu, Oct 13, 2016 at 10:54:51AM -0400, Alex Deucher wrote:
> On Thu, Oct 13, 2016 at 5:47 AM, Brian Starkey <brian.starkey@arm.com> wrote:
> > Add some additional comments to more explicitly describe the meaning and
> > usage of the three CRTC modeset detection booleans: mode_changed,
> > connectors_changed and active_changed.
> >
> > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> > ---
> >
> > Hi Daniel,
> >
> > I guess I asked for this one :-), please just check my understanding
> > is correct.
> >
> 
> The whole thread was very enlightening for me with respect to those
> flags as well.  The patch looks good to me.
> Acked-by: Alex Deucher <alexander.deucher@amd.com>

Applied to drm-misc, thanks.
-Daniel

> 
> > Thanks,
> > Brian
> >
> >  drivers/gpu/drm/drm_atomic_helper.c |    9 +++++----
> >  include/drm/drm_atomic.h            |   11 ++++++++++-
> >  include/drm/drm_crtc.h              |    5 +++++
> >  3 files changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 78ea735..fb4071a 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -458,10 +458,11 @@ mode_fixup(struct drm_atomic_state *state)
> >   * removed from the crtc.
> >   * crtc_state->active_changed is set when crtc_state->active changes,
> >   * which is used for dpms.
> > + * See also: drm_atomic_crtc_needs_modeset()
> >   *
> >   * IMPORTANT:
> >   *
> > - * Drivers which update ->mode_changed (e.g. in their ->atomic_check hooks if a
> > + * Drivers which set ->mode_changed (e.g. in their ->atomic_check hooks if a
> >   * plane update can't be done without a full modeset) _must_ call this function
> >   * afterwards after that change. It is permitted to call this function multiple
> >   * times for the same update, e.g. when the ->atomic_check functions depend upon
> > @@ -510,9 +511,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> >
> >         for_each_connector_in_state(state, connector, connector_state, i) {
> >                 /*
> > -                * This only sets crtc->mode_changed for routing changes,
> > -                * drivers must set crtc->mode_changed themselves when connector
> > -                * properties need to be updated.
> > +                * This only sets crtc->connectors_changed for routing changes,
> > +                * drivers must set crtc->connectors_changed themselves when
> > +                * connector properties need to be updated.
> >                  */
> >                 ret = update_connector_routing(state, connector,
> >                                                connector_state);
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index d9aff06..1ce255f 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -368,8 +368,17 @@ int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state);
> >   *
> >   * To give drivers flexibility struct &drm_crtc_state has 3 booleans to track
> >   * whether the state CRTC changed enough to need a full modeset cycle:
> > - * connectors_changed, mode_changed and active_change. This helper simply
> > + * connectors_changed, mode_changed and active_changed. This helper simply
> >   * combines these three to compute the overall need for a modeset for @state.
> > + *
> > + * The atomic helper code sets these booleans, but drivers can and should
> > + * change them appropriately to accurately represent whether a modeset is
> > + * really needed. In general, drivers should avoid full modesets whenever
> > + * possible.
> > + *
> > + * For example if the CRTC mode has changed, and the hardware is able to enact
> > + * the requested mode change without going through a full modeset, the driver
> > + * should clear mode_changed during its ->atomic_check.
> >   */
> >  static inline bool
> >  drm_atomic_crtc_needs_modeset(struct drm_crtc_state *state)
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index c4a3164..1f094d2 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -116,6 +116,11 @@ struct drm_plane_helper_funcs;
> >   * never return in a failure from the ->atomic_check callback. Userspace assumes
> >   * that a DPMS On will always succeed. In other words: @enable controls resource
> >   * assignment, @active controls the actual hardware state.
> > + *
> > + * The three booleans active_changed, connectors_changed and mode_changed are
> > + * intended to indicate whether a full modeset is needed, rather than strictly
> > + * describing what has changed in a commit.
> > + * See also: drm_atomic_crtc_needs_modeset()
> >   */
> >  struct drm_crtc_state {
> >         struct drm_crtc *crtc;
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1499001

On Tue, Oct 11, 2016 at 6:25 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>> Writeback connector usage:
>> --------------------------
>> Due to connector routing changes being treated as "full modeset"
>> operations, any client which wishes to use a writeback connector
>> should include the connector in every modeset. The writeback will not
>> actually become active until a framebuffer is attached.
>>
>> The writeback itself is enabled by attaching a framebuffer to the
>> FB_ID property of the connector. The driver must then ensure that the
>> CRTC content of that atomic commit is written into the framebuffer.
>>
>> The writeback works in a one-shot mode with each atomic commit. This
>> prevents the same content from being written multiple times.
>> In some cases (front-buffer rendering) there might be a desire for
>> continuous operation - I think a property could be added later for
>> this kind of control.
>
> I though people agreed that this sort of thing would go through v4l.
> Continously writing to the same buffer isn't perhaps all that sensible
> anyway, and so we'd need queueing, which is what v4l has already. Well,
> I guess we might add some queueing to atomic eventually?
>
> I guess for front buffer rendering type of thing you might have some
> use for a continuous mode targeting a single fb. Though I think
> peridically triggering a new write could do as well. Of course either
> way would likely tear horribly, and having multiple buffers seems like
> the better option

Yeah, momentarily entirely forgot about v4l. I think making FB_ID
one-shot (perhaps better to call it WRITEBACK_FB_ID to avoid
confusion) is the right thing to do, and then push everything
continuous to some form of drm/v4l integration.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1499003

On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote:
> Hi,
> 
> This RFC series introduces a new connector type:
>  DRM_MODE_CONNECTOR_WRITEBACK
> It is a follow-on from a previous discussion: [1]
> 
> Writeback connectors are used to expose the memory writeback engines
> found in some display controllers, which can write a CRTC's
> composition result to a memory buffer.
> This is useful e.g. for testing, screen-recording, screenshots,
> wireless display, display cloning, memory-to-memory composition.
> 
> Patches 1-7 include the core framework changes required, and patches
> 8-11 implement a writeback connector for the Mali-DP writeback engine.
> The Mali-DP patches depend on this other series: [2].
> 
> The connector is given the FB_ID property for the output framebuffer,
> and two new read-only properties: PIXEL_FORMATS and
> PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel
> formats of the engine.
> 
> The EDID property is not exposed for writeback connectors.
> 
> Writeback connector usage:
> --------------------------
> Due to connector routing changes being treated as "full modeset"
> operations, any client which wishes to use a writeback connector
> should include the connector in every modeset. The writeback will not
> actually become active until a framebuffer is attached.
> 
> The writeback itself is enabled by attaching a framebuffer to the
> FB_ID property of the connector. The driver must then ensure that the
> CRTC content of that atomic commit is written into the framebuffer.
> 
> The writeback works in a one-shot mode with each atomic commit. This
> prevents the same content from being written multiple times.
> In some cases (front-buffer rendering) there might be a desire for
> continuous operation - I think a property could be added later for
> this kind of control.

I though people agreed that this sort of thing would go through v4l.
Continously writing to the same buffer isn't perhaps all that sensible
anyway, and so we'd need queueing, which is what v4l has already. Well,
I guess we might add some queueing to atomic eventually?

I guess for front buffer rendering type of thing you might have some
use for a continuous mode targeting a single fb. Though I think
peridically triggering a new write could do as well. Of course either
way would likely tear horribly, and having multiple buffers seems like
the better option.

> 
> Writeback can be disabled by setting FB_ID to zero.
> 
> Known issues:
> -------------
>  * I'm not sure what "DPMS" should mean for writeback connectors.
>    It could be used to disable writeback (even when a framebuffer is
>    attached), or it could be hidden entirely (which would break the
>    legacy DPMS call for writeback connectors).
>  * With Daniel's recent re-iteration of the userspace API rules, I
>    fully expect to provide some userspace code to support this. The
>    question is what, and where? We want to use writeback for testing,
>    so perhaps some tests in igt is suitable.
>  * Documentation. Probably some portion of this cover letter needs to
>    make it into Documentation/
>  * Synchronisation. Our hardware will finish the writeback by the next
>    vsync. I've not implemented fence support here, but it would be an
>    obvious addition.
> 
> See Also:
> ---------
> [1] https://lists.freedesktop.org/archives/dri-devel/2016-July/113197.html
> [2] https://lists.freedesktop.org/archives/dri-devel/2016-October/120486.html
> 
> I welcome any comments, especially if this approach does/doesn't fit
> well with anyone else's hardware.
> 
> Thanks,
> 
> -Brian
> 
> ---
> 
> Brian Starkey (10):
>   drm: add writeback connector type
>   drm/fb-helper: skip writeback connectors
>   drm: extract CRTC/plane disable from drm_framebuffer_remove
>   drm: add __drm_framebuffer_remove_atomic
>   drm: add fb to connector state
>   drm: expose fb_id property for writeback connectors
>   drm: add writeback-connector pixel format properties
>   drm: mali-dp: rename malidp_input_format
>   drm: mali-dp: add RGB writeback formats for DP550/DP650
>   drm: mali-dp: add writeback connector
> 
> Liviu Dudau (1):
>   drm: mali-dp: Add support for writeback on DP550/DP650
> 
>  drivers/gpu/drm/arm/Makefile        |    1 +
>  drivers/gpu/drm/arm/malidp_crtc.c   |   10 ++
>  drivers/gpu/drm/arm/malidp_drv.c    |   25 +++-
>  drivers/gpu/drm/arm/malidp_drv.h    |    5 +
>  drivers/gpu/drm/arm/malidp_hw.c     |  104 ++++++++++----
>  drivers/gpu/drm/arm/malidp_hw.h     |   27 +++-
>  drivers/gpu/drm/arm/malidp_mw.c     |  268 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/arm/malidp_planes.c |    8 +-
>  drivers/gpu/drm/arm/malidp_regs.h   |   15 ++
>  drivers/gpu/drm/drm_atomic.c        |   40 ++++++
>  drivers/gpu/drm/drm_atomic_helper.c |    4 +
>  drivers/gpu/drm/drm_connector.c     |   79 ++++++++++-
>  drivers/gpu/drm/drm_crtc.c          |   14 +-
>  drivers/gpu/drm/drm_fb_helper.c     |    4 +
>  drivers/gpu/drm/drm_framebuffer.c   |  249 ++++++++++++++++++++++++++++----
>  drivers/gpu/drm/drm_ioctl.c         |    7 +
>  include/drm/drmP.h                  |    2 +
>  include/drm/drm_atomic.h            |    3 +
>  include/drm/drm_connector.h         |   15 ++
>  include/drm/drm_crtc.h              |   12 ++
>  include/uapi/drm/drm.h              |   10 ++
>  include/uapi/drm/drm_mode.h         |    1 +
>  22 files changed, 830 insertions(+), 73 deletions(-)
>  create mode 100644 drivers/gpu/drm/arm/malidp_mw.c
> 
> -- 
> 1.7.9.5

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1499162

[Multipart message — attachments visible in raw view] — view raw

Brian Starkey <brian.starkey@arm.com> writes:

> Hi,
>
> This RFC series introduces a new connector type:
>  DRM_MODE_CONNECTOR_WRITEBACK
> It is a follow-on from a previous discussion: [1]
>
> Writeback connectors are used to expose the memory writeback engines
> found in some display controllers, which can write a CRTC's
> composition result to a memory buffer.
> This is useful e.g. for testing, screen-recording, screenshots,
> wireless display, display cloning, memory-to-memory composition.
>
> Patches 1-7 include the core framework changes required, and patches
> 8-11 implement a writeback connector for the Mali-DP writeback engine.
> The Mali-DP patches depend on this other series: [2].
>
> The connector is given the FB_ID property for the output framebuffer,
> and two new read-only properties: PIXEL_FORMATS and
> PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel
> formats of the engine.
>
> The EDID property is not exposed for writeback connectors.
>
> Writeback connector usage:
> --------------------------
> Due to connector routing changes being treated as "full modeset"
> operations, any client which wishes to use a writeback connector
> should include the connector in every modeset. The writeback will not
> actually become active until a framebuffer is attached.
>
> The writeback itself is enabled by attaching a framebuffer to the
> FB_ID property of the connector. The driver must then ensure that the
> CRTC content of that atomic commit is written into the framebuffer.
>
> The writeback works in a one-shot mode with each atomic commit. This
> prevents the same content from being written multiple times.
> In some cases (front-buffer rendering) there might be a desire for
> continuous operation - I think a property could be added later for
> this kind of control.
>
> Writeback can be disabled by setting FB_ID to zero.

I think this sounds great, and the interface is just right IMO.

I don't really see a use for continuous mode -- a sequence of one-shots
makes a lot more sense because then you can know what data has changed,
which anyone trying to use the writeback buffer would need to know.

> Known issues:
> -------------
>  * I'm not sure what "DPMS" should mean for writeback connectors.
>    It could be used to disable writeback (even when a framebuffer is
>    attached), or it could be hidden entirely (which would break the
>    legacy DPMS call for writeback connectors).
>  * With Daniel's recent re-iteration of the userspace API rules, I
>    fully expect to provide some userspace code to support this. The
>    question is what, and where? We want to use writeback for testing,
>    so perhaps some tests in igt is suitable.
>  * Documentation. Probably some portion of this cover letter needs to
>    make it into Documentation/
>  * Synchronisation. Our hardware will finish the writeback by the next
>    vsync. I've not implemented fence support here, but it would be an
>    obvious addition.

My hardware won't necessarily finish by the next vsync -- it trickles
out at whatever rate it can find memory bandwidth to get the job done,
and fires an interrupt when it's finished.

So I would like some definition for how syncing works.  One answer would
be that these flips don't trigger their pageflip events until the
writeback is done (so I need to collect both the vsync irq and the
writeback irq before sending).  Another would be that manage an
independent fence for the writeback fb, so that you still immediately
know when framebuffers from the previous scanout-only frame are idle.

Also, tests for this in igt, please.  Writeback in igt will give us so
much more ability to cover KMS functionality on non-Intel hardware.

[toc] | [prev] | [next] | [standalone]

#1499404

Hi Eric,

On Tue, Oct 11, 2016 at 12:01:14PM -0700, Eric Anholt wrote:
>Brian Starkey <brian.starkey@arm.com> writes:
>
>> Hi,
>>
>> This RFC series introduces a new connector type:
>>  DRM_MODE_CONNECTOR_WRITEBACK
>> It is a follow-on from a previous discussion: [1]
>>
>> Writeback connectors are used to expose the memory writeback engines
>> found in some display controllers, which can write a CRTC's
>> composition result to a memory buffer.
>> This is useful e.g. for testing, screen-recording, screenshots,
>> wireless display, display cloning, memory-to-memory composition.
>>
>> Patches 1-7 include the core framework changes required, and patches
>> 8-11 implement a writeback connector for the Mali-DP writeback engine.
>> The Mali-DP patches depend on this other series: [2].
>>
>> The connector is given the FB_ID property for the output framebuffer,
>> and two new read-only properties: PIXEL_FORMATS and
>> PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel
>> formats of the engine.
>>
>> The EDID property is not exposed for writeback connectors.
>>
>> Writeback connector usage:
>> --------------------------
>> Due to connector routing changes being treated as "full modeset"
>> operations, any client which wishes to use a writeback connector
>> should include the connector in every modeset. The writeback will not
>> actually become active until a framebuffer is attached.
>>
>> The writeback itself is enabled by attaching a framebuffer to the
>> FB_ID property of the connector. The driver must then ensure that the
>> CRTC content of that atomic commit is written into the framebuffer.
>>
>> The writeback works in a one-shot mode with each atomic commit. This
>> prevents the same content from being written multiple times.
>> In some cases (front-buffer rendering) there might be a desire for
>> continuous operation - I think a property could be added later for
>> this kind of control.
>>
>> Writeback can be disabled by setting FB_ID to zero.
>
>I think this sounds great, and the interface is just right IMO.
>

Thanks, glad you like it! Hopefully you're equally agreeable with the
changes Daniel has been suggesting.

>I don't really see a use for continuous mode -- a sequence of one-shots
>makes a lot more sense because then you can know what data has changed,
>which anyone trying to use the writeback buffer would need to know.
>

Agreed - we've never found a use for it.

>> Known issues:
>> -------------
>>  * I'm not sure what "DPMS" should mean for writeback connectors.
>>    It could be used to disable writeback (even when a framebuffer is
>>    attached), or it could be hidden entirely (which would break the
>>    legacy DPMS call for writeback connectors).
>>  * With Daniel's recent re-iteration of the userspace API rules, I
>>    fully expect to provide some userspace code to support this. The
>>    question is what, and where? We want to use writeback for testing,
>>    so perhaps some tests in igt is suitable.
>>  * Documentation. Probably some portion of this cover letter needs to
>>    make it into Documentation/
>>  * Synchronisation. Our hardware will finish the writeback by the next
>>    vsync. I've not implemented fence support here, but it would be an
>>    obvious addition.
>
>My hardware won't necessarily finish by the next vsync -- it trickles
>out at whatever rate it can find memory bandwidth to get the job done,
>and fires an interrupt when it's finished.
>

Is it bounded? You presumably have to finish the write-out before you
can change any input buffers?

>So I would like some definition for how syncing works.  One answer would
>be that these flips don't trigger their pageflip events until the
>writeback is done (so I need to collect both the vsync irq and the
>writeback irq before sending).  Another would be that manage an
>independent fence for the writeback fb, so that you still immediately
>know when framebuffers from the previous scanout-only frame are idle.
>

I much prefer the sound of the explicit fence approach.

Hopefully we can agree that a new atomic commit can't be completed
whilst there's a writeback ongoing, otherwise managing the fence and
framebuffer lifetime sounds really tricky - they'd need to be decoupled
from the atomic_state and outlive the commit that spawned them.

Cheers,
-Brian

>Also, tests for this in igt, please.  Writeback in igt will give us so
>much more ability to cover KMS functionality on non-Intel hardware.

[toc] | [prev] | [next] | [standalone]

#1500476

[Multipart message — attachments visible in raw view] — view raw

Brian Starkey <brian.starkey@arm.com> writes:

> Hi Eric,
>
> On Tue, Oct 11, 2016 at 12:01:14PM -0700, Eric Anholt wrote:
>>Brian Starkey <brian.starkey@arm.com> writes:
>>
>>> Hi,
>>>
>>> This RFC series introduces a new connector type:
>>>  DRM_MODE_CONNECTOR_WRITEBACK
>>> It is a follow-on from a previous discussion: [1]
>>>
>>> Writeback connectors are used to expose the memory writeback engines
>>> found in some display controllers, which can write a CRTC's
>>> composition result to a memory buffer.
>>> This is useful e.g. for testing, screen-recording, screenshots,
>>> wireless display, display cloning, memory-to-memory composition.
>>>
>>> Patches 1-7 include the core framework changes required, and patches
>>> 8-11 implement a writeback connector for the Mali-DP writeback engine.
>>> The Mali-DP patches depend on this other series: [2].
>>>
>>> The connector is given the FB_ID property for the output framebuffer,
>>> and two new read-only properties: PIXEL_FORMATS and
>>> PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel
>>> formats of the engine.
>>>
>>> The EDID property is not exposed for writeback connectors.
>>>
>>> Writeback connector usage:
>>> --------------------------
>>> Due to connector routing changes being treated as "full modeset"
>>> operations, any client which wishes to use a writeback connector
>>> should include the connector in every modeset. The writeback will not
>>> actually become active until a framebuffer is attached.
>>>
>>> The writeback itself is enabled by attaching a framebuffer to the
>>> FB_ID property of the connector. The driver must then ensure that the
>>> CRTC content of that atomic commit is written into the framebuffer.
>>>
>>> The writeback works in a one-shot mode with each atomic commit. This
>>> prevents the same content from being written multiple times.
>>> In some cases (front-buffer rendering) there might be a desire for
>>> continuous operation - I think a property could be added later for
>>> this kind of control.
>>>
>>> Writeback can be disabled by setting FB_ID to zero.
>>
>>I think this sounds great, and the interface is just right IMO.
>>
>
> Thanks, glad you like it! Hopefully you're equally agreeable with the
> changes Daniel has been suggesting.

Haven't seen anything objectionable there.

>>> Known issues:
>>> -------------
>>>  * I'm not sure what "DPMS" should mean for writeback connectors.
>>>    It could be used to disable writeback (even when a framebuffer is
>>>    attached), or it could be hidden entirely (which would break the
>>>    legacy DPMS call for writeback connectors).
>>>  * With Daniel's recent re-iteration of the userspace API rules, I
>>>    fully expect to provide some userspace code to support this. The
>>>    question is what, and where? We want to use writeback for testing,
>>>    so perhaps some tests in igt is suitable.
>>>  * Documentation. Probably some portion of this cover letter needs to
>>>    make it into Documentation/
>>>  * Synchronisation. Our hardware will finish the writeback by the next
>>>    vsync. I've not implemented fence support here, but it would be an
>>>    obvious addition.
>>
>>My hardware won't necessarily finish by the next vsync -- it trickles
>>out at whatever rate it can find memory bandwidth to get the job done,
>>and fires an interrupt when it's finished.
>>
>
> Is it bounded? You presumably have to finish the write-out before you
> can change any input buffers?

Yeah, I'm not sure what it would mean to try to swap my display list
while write-out was happening.  Each CRTC (each of which can only
support one encoder at a time) has its own display list, though, so it
could avoid blocking other modesets.

>>So I would like some definition for how syncing works.  One answer would
>>be that these flips don't trigger their pageflip events until the
>>writeback is done (so I need to collect both the vsync irq and the
>>writeback irq before sending).  Another would be that manage an
>>independent fence for the writeback fb, so that you still immediately
>>know when framebuffers from the previous scanout-only frame are idle.
>>
>
> I much prefer the sound of the explicit fence approach.
>
> Hopefully we can agree that a new atomic commit can't be completed
> whilst there's a writeback ongoing, otherwise managing the fence and
> framebuffer lifetime sounds really tricky - they'd need to be decoupled
> from the atomic_state and outlive the commit that spawned them.

Oh, good point.

I'm fine with that, but then my anticipated usecases for writeback are
testing (don't care about performance) and fallback plane-squashing when
a complicated modeset exceeds limits (in which case you have no simpler
plane config to modeset to until the writeback is completed, anyway).

[toc] | [prev] | [next] | [standalone]

#1500830

Hi Brian,

On 10/11/2016 08:23 PM, Brian Starkey wrote:
> Hi,
>
> This RFC series introduces a new connector type:
>  DRM_MODE_CONNECTOR_WRITEBACK
> It is a follow-on from a previous discussion: [1]
>
> Writeback connectors are used to expose the memory writeback engines
> found in some display controllers, which can write a CRTC's
> composition result to a memory buffer.
> This is useful e.g. for testing, screen-recording, screenshots,
> wireless display, display cloning, memory-to-memory composition.
>
> Patches 1-7 include the core framework changes required, and patches
> 8-11 implement a writeback connector for the Mali-DP writeback engine.
> The Mali-DP patches depend on this other series: [2].
>
> The connector is given the FB_ID property for the output framebuffer,
> and two new read-only properties: PIXEL_FORMATS and
> PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel
> formats of the engine.
>
> The EDID property is not exposed for writeback connectors.
>
> Writeback connector usage:
> --------------------------
> Due to connector routing changes being treated as "full modeset"
> operations, any client which wishes to use a writeback connector
> should include the connector in every modeset. The writeback will not
> actually become active until a framebuffer is attached.
>
> The writeback itself is enabled by attaching a framebuffer to the
> FB_ID property of the connector. The driver must then ensure that the
> CRTC content of that atomic commit is written into the framebuffer.
>
> The writeback works in a one-shot mode with each atomic commit. This
> prevents the same content from being written multiple times.
> In some cases (front-buffer rendering) there might be a desire for
> continuous operation - I think a property could be added later for
> this kind of control.
>
> Writeback can be disabled by setting FB_ID to zero.
>
> Known issues:
> -------------
>  * I'm not sure what "DPMS" should mean for writeback connectors.
>    It could be used to disable writeback (even when a framebuffer is
>    attached), or it could be hidden entirely (which would break the
>    legacy DPMS call for writeback connectors).
>  * With Daniel's recent re-iteration of the userspace API rules, I
>    fully expect to provide some userspace code to support this. The
>    question is what, and where? We want to use writeback for testing,
>    so perhaps some tests in igt is suitable.
>  * Documentation. Probably some portion of this cover letter needs to
>    make it into Documentation/
>  * Synchronisation. Our hardware will finish the writeback by the next
>    vsync. I've not implemented fence support here, but it would be an
>    obvious addition.
>
> See Also:
> ---------
> [1] https://lists.freedesktop.org/archives/dri-devel/2016-July/113197.html
> [2] https://lists.freedesktop.org/archives/dri-devel/2016-October/120486.html
>
> I welcome any comments, especially if this approach does/doesn't fit
> well with anyone else's hardware.

Thanks for working on this! Some points below.

- Writeback hardware generally allows us to specify the region within
the framebuffer we want to write to. It's analogous to the SRC_X/Y/W/H
plane properties. We could have similar props for the writeback
connectors, and maybe set them to the FB_ID dimensions if they aren't
configured by userspace.

- Besides the above property, writeback hardware can have provisions
for scaling, color space conversion and rotation. This would mean that
we'd eventually add more writeback specific props/params in
drm_connector/drm_connector_state. Would we be okay adding more such
props for connectors?

Thanks,
Archit

>
> Thanks,
>
> -Brian
>
> ---
>
> Brian Starkey (10):
>   drm: add writeback connector type
>   drm/fb-helper: skip writeback connectors
>   drm: extract CRTC/plane disable from drm_framebuffer_remove
>   drm: add __drm_framebuffer_remove_atomic
>   drm: add fb to connector state
>   drm: expose fb_id property for writeback connectors
>   drm: add writeback-connector pixel format properties
>   drm: mali-dp: rename malidp_input_format
>   drm: mali-dp: add RGB writeback formats for DP550/DP650
>   drm: mali-dp: add writeback connector
>
> Liviu Dudau (1):
>   drm: mali-dp: Add support for writeback on DP550/DP650
>
>  drivers/gpu/drm/arm/Makefile        |    1 +
>  drivers/gpu/drm/arm/malidp_crtc.c   |   10 ++
>  drivers/gpu/drm/arm/malidp_drv.c    |   25 +++-
>  drivers/gpu/drm/arm/malidp_drv.h    |    5 +
>  drivers/gpu/drm/arm/malidp_hw.c     |  104 ++++++++++----
>  drivers/gpu/drm/arm/malidp_hw.h     |   27 +++-
>  drivers/gpu/drm/arm/malidp_mw.c     |  268 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/arm/malidp_planes.c |    8 +-
>  drivers/gpu/drm/arm/malidp_regs.h   |   15 ++
>  drivers/gpu/drm/drm_atomic.c        |   40 ++++++
>  drivers/gpu/drm/drm_atomic_helper.c |    4 +
>  drivers/gpu/drm/drm_connector.c     |   79 ++++++++++-
>  drivers/gpu/drm/drm_crtc.c          |   14 +-
>  drivers/gpu/drm/drm_fb_helper.c     |    4 +
>  drivers/gpu/drm/drm_framebuffer.c   |  249 ++++++++++++++++++++++++++++----
>  drivers/gpu/drm/drm_ioctl.c         |    7 +
>  include/drm/drmP.h                  |    2 +
>  include/drm/drm_atomic.h            |    3 +
>  include/drm/drm_connector.h         |   15 ++
>  include/drm/drm_crtc.h              |   12 ++
>  include/uapi/drm/drm.h              |   10 ++
>  include/uapi/drm/drm_mode.h         |    1 +
>  22 files changed, 830 insertions(+), 73 deletions(-)
>  create mode 100644 drivers/gpu/drm/arm/malidp_mw.c
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1500933

Hi Archit,

On Fri, Oct 14, 2016 at 04:20:14PM +0530, Archit Taneja wrote:
>Hi Brian,
>
>On 10/11/2016 08:23 PM, Brian Starkey wrote:
>>Hi,
>>
>>This RFC series introduces a new connector type:
>> DRM_MODE_CONNECTOR_WRITEBACK
>>It is a follow-on from a previous discussion: [1]
>>
>>Writeback connectors are used to expose the memory writeback engines
>>found in some display controllers, which can write a CRTC's
>>composition result to a memory buffer.
>>This is useful e.g. for testing, screen-recording, screenshots,
>>wireless display, display cloning, memory-to-memory composition.
>>
>>Patches 1-7 include the core framework changes required, and patches
>>8-11 implement a writeback connector for the Mali-DP writeback engine.
>>The Mali-DP patches depend on this other series: [2].
>>
>>The connector is given the FB_ID property for the output framebuffer,
>>and two new read-only properties: PIXEL_FORMATS and
>>PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel
>>formats of the engine.
>>
>>The EDID property is not exposed for writeback connectors.
>>
>>Writeback connector usage:
>>--------------------------
>>Due to connector routing changes being treated as "full modeset"
>>operations, any client which wishes to use a writeback connector
>>should include the connector in every modeset. The writeback will not
>>actually become active until a framebuffer is attached.
>>
>>The writeback itself is enabled by attaching a framebuffer to the
>>FB_ID property of the connector. The driver must then ensure that the
>>CRTC content of that atomic commit is written into the framebuffer.
>>
>>The writeback works in a one-shot mode with each atomic commit. This
>>prevents the same content from being written multiple times.
>>In some cases (front-buffer rendering) there might be a desire for
>>continuous operation - I think a property could be added later for
>>this kind of control.
>>
>>Writeback can be disabled by setting FB_ID to zero.
>>
>>Known issues:
>>-------------
>> * I'm not sure what "DPMS" should mean for writeback connectors.
>>   It could be used to disable writeback (even when a framebuffer is
>>   attached), or it could be hidden entirely (which would break the
>>   legacy DPMS call for writeback connectors).
>> * With Daniel's recent re-iteration of the userspace API rules, I
>>   fully expect to provide some userspace code to support this. The
>>   question is what, and where? We want to use writeback for testing,
>>   so perhaps some tests in igt is suitable.
>> * Documentation. Probably some portion of this cover letter needs to
>>   make it into Documentation/
>> * Synchronisation. Our hardware will finish the writeback by the next
>>   vsync. I've not implemented fence support here, but it would be an
>>   obvious addition.
>>
>>See Also:
>>---------
>>[1] https://lists.freedesktop.org/archives/dri-devel/2016-July/113197.html
>>[2] https://lists.freedesktop.org/archives/dri-devel/2016-October/120486.html
>>
>>I welcome any comments, especially if this approach does/doesn't fit
>>well with anyone else's hardware.
>
>Thanks for working on this! Some points below.
>
>- Writeback hardware generally allows us to specify the region within
>the framebuffer we want to write to. It's analogous to the SRC_X/Y/W/H
>plane properties. We could have similar props for the writeback
>connectors, and maybe set them to the FB_ID dimensions if they aren't
>configured by userspace.
>
>- Besides the above property, writeback hardware can have provisions
>for scaling, color space conversion and rotation. This would mean that
>we'd eventually add more writeback specific props/params in
>drm_connector/drm_connector_state. Would we be okay adding more such
>props for connectors?

I've wondered the same thing about bloating non-writeback connectors
with writeback-specific stuff. If it does become significant, maybe
we should subclass drm_connector and add a drm_writeback_state pointer
to drm_connector_state.

Ville touched on scaling support previously, suggesting adding a
fixed_mode property (for all types of connectors) - on writeback this
would represent scaling the framebuffer, and on normal connectors it
could control output scaling (like panel-fitting).

Certainly destination coords, color-space converstion etc. are things
that are worth adding, but IMO I'd rather keep this initial
implementation small so we can enable the basic case right away. For
the most part, the additional things are "just properties" which
should be easily added later without impacting the overall interface.

Cheers,
Brian
>
>Thanks,
>Archit
>
>>
>>Thanks,
>>
>>-Brian
>>
>>---
>>
>>Brian Starkey (10):
>>  drm: add writeback connector type
>>  drm/fb-helper: skip writeback connectors
>>  drm: extract CRTC/plane disable from drm_framebuffer_remove
>>  drm: add __drm_framebuffer_remove_atomic
>>  drm: add fb to connector state
>>  drm: expose fb_id property for writeback connectors
>>  drm: add writeback-connector pixel format properties
>>  drm: mali-dp: rename malidp_input_format
>>  drm: mali-dp: add RGB writeback formats for DP550/DP650
>>  drm: mali-dp: add writeback connector
>>
>>Liviu Dudau (1):
>>  drm: mali-dp: Add support for writeback on DP550/DP650
>>
>> drivers/gpu/drm/arm/Makefile        |    1 +
>> drivers/gpu/drm/arm/malidp_crtc.c   |   10 ++
>> drivers/gpu/drm/arm/malidp_drv.c    |   25 +++-
>> drivers/gpu/drm/arm/malidp_drv.h    |    5 +
>> drivers/gpu/drm/arm/malidp_hw.c     |  104 ++++++++++----
>> drivers/gpu/drm/arm/malidp_hw.h     |   27 +++-
>> drivers/gpu/drm/arm/malidp_mw.c     |  268 +++++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/arm/malidp_planes.c |    8 +-
>> drivers/gpu/drm/arm/malidp_regs.h   |   15 ++
>> drivers/gpu/drm/drm_atomic.c        |   40 ++++++
>> drivers/gpu/drm/drm_atomic_helper.c |    4 +
>> drivers/gpu/drm/drm_connector.c     |   79 ++++++++++-
>> drivers/gpu/drm/drm_crtc.c          |   14 +-
>> drivers/gpu/drm/drm_fb_helper.c     |    4 +
>> drivers/gpu/drm/drm_framebuffer.c   |  249 ++++++++++++++++++++++++++++----
>> drivers/gpu/drm/drm_ioctl.c         |    7 +
>> include/drm/drmP.h                  |    2 +
>> include/drm/drm_atomic.h            |    3 +
>> include/drm/drm_connector.h         |   15 ++
>> include/drm/drm_crtc.h              |   12 ++
>> include/uapi/drm/drm.h              |   10 ++
>> include/uapi/drm/drm_mode.h         |    1 +
>> 22 files changed, 830 insertions(+), 73 deletions(-)
>> create mode 100644 drivers/gpu/drm/arm/malidp_mw.c
>>
>
>-- 
>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>a Linux Foundation Collaborative Project
>--
>To unsubscribe from this list: send the line "unsubscribe linux-media" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[toc] | [prev] | [next] | [standalone]

#1500950

On Fri, Oct 14, 2016 at 01:39:15PM +0100, Brian Starkey wrote:
> Hi Archit,
> 
> On Fri, Oct 14, 2016 at 04:20:14PM +0530, Archit Taneja wrote:
> >Hi Brian,
> >
> >On 10/11/2016 08:23 PM, Brian Starkey wrote:
> >>Hi,
> >>
> >>This RFC series introduces a new connector type:
> >> DRM_MODE_CONNECTOR_WRITEBACK
> >>It is a follow-on from a previous discussion: [1]
> >>
> >>Writeback connectors are used to expose the memory writeback engines
> >>found in some display controllers, which can write a CRTC's
> >>composition result to a memory buffer.
> >>This is useful e.g. for testing, screen-recording, screenshots,
> >>wireless display, display cloning, memory-to-memory composition.
> >>
> >>Patches 1-7 include the core framework changes required, and patches
> >>8-11 implement a writeback connector for the Mali-DP writeback engine.
> >>The Mali-DP patches depend on this other series: [2].
> >>
> >>The connector is given the FB_ID property for the output framebuffer,
> >>and two new read-only properties: PIXEL_FORMATS and
> >>PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel
> >>formats of the engine.
> >>
> >>The EDID property is not exposed for writeback connectors.
> >>
> >>Writeback connector usage:
> >>--------------------------
> >>Due to connector routing changes being treated as "full modeset"
> >>operations, any client which wishes to use a writeback connector
> >>should include the connector in every modeset. The writeback will not
> >>actually become active until a framebuffer is attached.
> >>
> >>The writeback itself is enabled by attaching a framebuffer to the
> >>FB_ID property of the connector. The driver must then ensure that the
> >>CRTC content of that atomic commit is written into the framebuffer.
> >>
> >>The writeback works in a one-shot mode with each atomic commit. This
> >>prevents the same content from being written multiple times.
> >>In some cases (front-buffer rendering) there might be a desire for
> >>continuous operation - I think a property could be added later for
> >>this kind of control.
> >>
> >>Writeback can be disabled by setting FB_ID to zero.
> >>
> >>Known issues:
> >>-------------
> >> * I'm not sure what "DPMS" should mean for writeback connectors.
> >>   It could be used to disable writeback (even when a framebuffer is
> >>   attached), or it could be hidden entirely (which would break the
> >>   legacy DPMS call for writeback connectors).
> >> * With Daniel's recent re-iteration of the userspace API rules, I
> >>   fully expect to provide some userspace code to support this. The
> >>   question is what, and where? We want to use writeback for testing,
> >>   so perhaps some tests in igt is suitable.
> >> * Documentation. Probably some portion of this cover letter needs to
> >>   make it into Documentation/
> >> * Synchronisation. Our hardware will finish the writeback by the next
> >>   vsync. I've not implemented fence support here, but it would be an
> >>   obvious addition.
> >>
> >>See Also:
> >>---------
> >>[1] https://lists.freedesktop.org/archives/dri-devel/2016-July/113197.html
> >>[2] https://lists.freedesktop.org/archives/dri-devel/2016-October/120486.html
> >>
> >>I welcome any comments, especially if this approach does/doesn't fit
> >>well with anyone else's hardware.
> >
> >Thanks for working on this! Some points below.
> >
> >- Writeback hardware generally allows us to specify the region within
> >the framebuffer we want to write to. It's analogous to the SRC_X/Y/W/H
> >plane properties. We could have similar props for the writeback
> >connectors, and maybe set them to the FB_ID dimensions if they aren't
> >configured by userspace.
> >
> >- Besides the above property, writeback hardware can have provisions
> >for scaling, color space conversion and rotation. This would mean that
> >we'd eventually add more writeback specific props/params in
> >drm_connector/drm_connector_state. Would we be okay adding more such
> >props for connectors?
> 
> I've wondered the same thing about bloating non-writeback connectors
> with writeback-specific stuff. If it does become significant, maybe
> we should subclass drm_connector and add a drm_writeback_state pointer
> to drm_connector_state.
> 
> Ville touched on scaling support previously, suggesting adding a
> fixed_mode property (for all types of connectors) - on writeback this
> would represent scaling the framebuffer, and on normal connectors it
> could control output scaling (like panel-fitting).

We got some patches [1] posted for i915 recently that added a bunch of new
properties to control post-blending scaling, but I'm not sure I like the
approach since it seems harder to reconcile with the current way we deal
with scaling for eDP/LVDS/DSI/etc. So I'm still somewhat partial to the
fixed mode idea. Just FYI.

[1] https://lists.freedesktop.org/archives/intel-gfx/2016-August/105557.html

> 
> Certainly destination coords, color-space converstion etc. are things
> that are worth adding, but IMO I'd rather keep this initial
> implementation small so we can enable the basic case right away. For
> the most part, the additional things are "just properties" which
> should be easily added later without impacting the overall interface.
> 
> Cheers,
> Brian
> >
> >Thanks,
> >Archit
> >
> >>
> >>Thanks,
> >>
> >>-Brian
> >>
> >>---
> >>
> >>Brian Starkey (10):
> >>  drm: add writeback connector type
> >>  drm/fb-helper: skip writeback connectors
> >>  drm: extract CRTC/plane disable from drm_framebuffer_remove
> >>  drm: add __drm_framebuffer_remove_atomic
> >>  drm: add fb to connector state
> >>  drm: expose fb_id property for writeback connectors
> >>  drm: add writeback-connector pixel format properties
> >>  drm: mali-dp: rename malidp_input_format
> >>  drm: mali-dp: add RGB writeback formats for DP550/DP650
> >>  drm: mali-dp: add writeback connector
> >>
> >>Liviu Dudau (1):
> >>  drm: mali-dp: Add support for writeback on DP550/DP650
> >>
> >> drivers/gpu/drm/arm/Makefile        |    1 +
> >> drivers/gpu/drm/arm/malidp_crtc.c   |   10 ++
> >> drivers/gpu/drm/arm/malidp_drv.c    |   25 +++-
> >> drivers/gpu/drm/arm/malidp_drv.h    |    5 +
> >> drivers/gpu/drm/arm/malidp_hw.c     |  104 ++++++++++----
> >> drivers/gpu/drm/arm/malidp_hw.h     |   27 +++-
> >> drivers/gpu/drm/arm/malidp_mw.c     |  268 +++++++++++++++++++++++++++++++++++
> >> drivers/gpu/drm/arm/malidp_planes.c |    8 +-
> >> drivers/gpu/drm/arm/malidp_regs.h   |   15 ++
> >> drivers/gpu/drm/drm_atomic.c        |   40 ++++++
> >> drivers/gpu/drm/drm_atomic_helper.c |    4 +
> >> drivers/gpu/drm/drm_connector.c     |   79 ++++++++++-
> >> drivers/gpu/drm/drm_crtc.c          |   14 +-
> >> drivers/gpu/drm/drm_fb_helper.c     |    4 +
> >> drivers/gpu/drm/drm_framebuffer.c   |  249 ++++++++++++++++++++++++++++----
> >> drivers/gpu/drm/drm_ioctl.c         |    7 +
> >> include/drm/drmP.h                  |    2 +
> >> include/drm/drm_atomic.h            |    3 +
> >> include/drm/drm_connector.h         |   15 ++
> >> include/drm/drm_crtc.h              |   12 ++
> >> include/uapi/drm/drm.h              |   10 ++
> >> include/uapi/drm/drm_mode.h         |    1 +
> >> 22 files changed, 830 insertions(+), 73 deletions(-)
> >> create mode 100644 drivers/gpu/drm/arm/malidp_mw.c
> >>
> >
> >-- 
> >Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> >a Linux Foundation Collaborative Project
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-media" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1501051

On Fri, Oct 14, 2016 at 2:39 PM, Brian Starkey <brian.starkey@arm.com> wrote:
>> - Besides the above property, writeback hardware can have provisions
>> for scaling, color space conversion and rotation. This would mean that
>> we'd eventually add more writeback specific props/params in
>> drm_connector/drm_connector_state. Would we be okay adding more such
>> props for connectors?
>
>
> I've wondered the same thing about bloating non-writeback connectors
> with writeback-specific stuff. If it does become significant, maybe
> we should subclass drm_connector and add a drm_writeback_state pointer
> to drm_connector_state.

No pionters needed, just embedded drm_connector_state into
drm_writeback_connector_state as the "base" member. Then we can
provide ready-made atomic_set/get_property functions for the aditional
writeback functionality.

But tbh I'd only start doing that once we have a few more. It's purely
an implementation change, with no effect on userspace. And if you go
with my drm_writeback_connector_init idea, it won't even be an issue
for drivers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web