[PATCH v1 0/6] drm: kernel-doc stuff

[PATCH v1 0/6] drm: kernel-doc stuff

[Sam Ravnborg]

Lyude Paul wrote a nice intro to vblank in the following mail:
https://lore.kernel.org/dri-devel/faf63d8a9ed23c16af69762f59d0dca6b2bf085f.camel@redhat.com/T/#mce6480be738160e9d07c5d023e88fd78d7a06d27

Reading this I managed to spot a glimmer of hope that I one
day would understand some of the fuzz around vblank.
To let others benefit from the description I went ahead
and added the description to drm_vblank.c.

Lyude - I added a "Co-developed-by: ..."
Can I get your s-o-b on the patch, to document that
you are OK with this to go in.

When checking the output with "make htmldocs" I noticed
several drm related warnings.
I went ahead a fixed most of them resulting in a few extre patches.

There are some warnings in amdgpu land - I have left
them for the AMD people to figure out:
    amdgpu_vm.c:92: warning: Function parameter or member 'vm' not described in 'amdgpu_vm_eviction_lock'
    amdgpu_xgmi.c:1: warning: no structured comments found
    amdgpu_ras.c:1: warning: no structured comments found
    amdgpu_dm.h:305: warning: Function parameter or member 'hdcp_workqueue' not described in 'amdgpu_display_manager'

	Sam


Sam Ravnborg (6):
      drm/vblank: Add intro to documentation
      drm/fb: fix kernel-doc in drm_framebuffer.h
      drm/sched: fix kernel-doc in gpu_scheduler.h
      drm: writeback: document callbacks
      drm/dp_mst: add kernel-doc for drm_dp_mst_port.fec_capable
      drm/bridge: fix kernel-doc warning in panel.c

 drivers/gpu/drm/bridge/panel.c           |  1 +
 drivers/gpu/drm/drm_vblank.c             | 15 +++++++++++++++
 include/drm/drm_dp_mst_helper.h          |  4 ++++
 include/drm/drm_framebuffer.h            |  4 ++--
 include/drm/drm_modeset_helper_vtables.h | 31 +++++++++++++++++++++++++++++++
 include/drm/gpu_scheduler.h              |  1 +
 6 files changed, 54 insertions(+), 2 deletions(-)


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v1 1/6] drm/vblank: Add intro to documentation

[Sam Ravnborg]

Lyude Paul wrote a very good intro to vblank here:
https://lore.kernel.org/dri-devel/faf63d8a9ed23c16af69762f59d0dca6b2bf085f.camel@redhat.com/T/#mce6480be738160e9d07c5d023e88fd78d7a06d27

Add this to the intro chapter in drm_vblank.c so others
can benefit from it too.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Co-developed-by: Lyude Paul <lyude@redhat.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
---
 drivers/gpu/drm/drm_vblank.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index bcf346b3e486..95cac22d59d1 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -41,6 +41,21 @@
 /**
  * DOC: vblank handling
  *
+ * From the perspective of a computer, every time a computer monitor displays
+ * a new frame it's done by "scanning out" the display image from top to
+ * bottom, one row of pixels at a time. which row of pixels we're on is
+ * referred to as the scanline.
+ * Additionally, there's usually a couple of extra scanlines which we
+ * scan out, but aren't actually displayed on the screen (these sometimes
+ * get used by HDMI audio and friends, but that's another story).
+ * The period where we're on these scanlines is referred to as the vblank.
+ *
+ * On a lot of display hardware, programming needs to take effect during the
+ * vertical blanking period so that settings like gamma, what frame we're
+ * scanning out, etc. can be safely changed without showing visual tearing
+ * on the screen. In some unforgiving hardware, some of this programming has
+ * to both start and end in the same vblank - vertical blanking.
+ *
  * Vertical blanking plays a major role in graphics rendering. To achieve
  * tear-free display, users must synchronize page flips and/or rendering to
  * vertical blanking. The DRM API offers ioctls to perform page flips
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 1/6] drm/vblank: Add intro to documentation

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3445 bytes --]

Hi Sam and Lyude,

thanks for improving the documentation. Below are a few points that I'd
found more understandable. I'm no native speaker, though.

Am 28.03.20 um 14:20 schrieb Sam Ravnborg:
> Lyude Paul wrote a very good intro to vblank here:
> https://lore.kernel.org/dri-devel/faf63d8a9ed23c16af69762f59d0dca6b2bf085f.camel@redhat.com/T/#mce6480be738160e9d07c5d023e88fd78d7a06d27
> 
> Add this to the intro chapter in drm_vblank.c so others
> can benefit from it too.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Co-developed-by: Lyude Paul <lyude@redhat.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> ---
>  drivers/gpu/drm/drm_vblank.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index bcf346b3e486..95cac22d59d1 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -41,6 +41,21 @@
>  /**
>   * DOC: vblank handling
>   *
> + * From the perspective of a computer, every time a computer monitor displays

Possibly change from dative case to genitive:

"From the computer's perspective," ...

> + * a new frame it's done by "scanning out" the display image from top to
> + * bottom, one row of pixels at a time. which row of pixels we're on is

s/which/Which

The text changes from third person (the computer) to first person
(we're). Makes it harder to read. I'd remove both, "we" and "computer",
and speak of display hardware or scanout engine.

> + * referred to as the scanline.

I'd say a scanline is any of them. Maybe say "current scanline"?

> + * Additionally, there's usually a couple of extra scanlines which we

"In addition to the display's visible area, there's usually a couple of
extra scanlines that" ...

> + * scan out, but aren't actually displayed on the screen (these sometimes
> + * get used by HDMI audio and friends, but that's another story).
> + * The period where we're on these scanlines is referred to as the vblank.

I'd replace vblank with "vertical blanking period." That term is
required in the next paragraph.

The time when the hardware operates on these invisible scanlines is
referred to as vertical blanking period, or simply vblank.

> + *
> + * On a lot of display hardware, programming needs to take effect during the
> + * vertical blanking period so that settings like gamma, what frame we're

"we" again

> + * scanning out, etc. can be safely changed without showing visual tearing
> + * on the screen. In some unforgiving hardware, some of this programming has
> + * to both start and end in the same vblank - vertical blanking.
> + *
>   * Vertical blanking plays a major role in graphics rendering. To achieve
>   * tear-free display, users must synchronize page flips and/or rendering to
>   * vertical blanking. The DRM API offers ioctls to perform page flips
> 

In any case

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

Best regards
Thomas

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 1/6] drm/vblank: Add intro to documentation

[Sam Ravnborg]

Hi Thomas.

On Mon, Mar 30, 2020 at 01:29:16PM +0200, Thomas Zimmermann wrote:
> Hi Sam and Lyude,
> 
> thanks for improving the documentation. Below are a few points that I'd
> found more understandable. I'm no native speaker, though.
> 
> Am 28.03.20 um 14:20 schrieb Sam Ravnborg:
> > Lyude Paul wrote a very good intro to vblank here:
> > https://lore.kernel.org/dri-devel/faf63d8a9ed23c16af69762f59d0dca6b2bf085f.camel@redhat.com/T/#mce6480be738160e9d07c5d023e88fd78d7a06d27
> > 
> > Add this to the intro chapter in drm_vblank.c so others
> > can benefit from it too.
> > 
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Co-developed-by: Lyude Paul <lyude@redhat.com>
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: David Airlie <airlied@linux.ie>
> > ---
> >  drivers/gpu/drm/drm_vblank.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > index bcf346b3e486..95cac22d59d1 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -41,6 +41,21 @@
> >  /**
> >   * DOC: vblank handling
> >   *
> > + * From the perspective of a computer, every time a computer monitor displays
> 
> Possibly change from dative case to genitive:
> 
> "From the computer's perspective," ...
> 
> > + * a new frame it's done by "scanning out" the display image from top to
> > + * bottom, one row of pixels at a time. which row of pixels we're on is
> 
> s/which/Which
> 
> The text changes from third person (the computer) to first person
> (we're). Makes it harder to read. I'd remove both, "we" and "computer",
> and speak of display hardware or scanout engine.
> 
> > + * referred to as the scanline.
> 
> I'd say a scanline is any of them. Maybe say "current scanline"?
> 
> > + * Additionally, there's usually a couple of extra scanlines which we
> 
> "In addition to the display's visible area, there's usually a couple of
> extra scanlines that" ...
> 
> > + * scan out, but aren't actually displayed on the screen (these sometimes
> > + * get used by HDMI audio and friends, but that's another story).
> > + * The period where we're on these scanlines is referred to as the vblank.
> 
> I'd replace vblank with "vertical blanking period." That term is
> required in the next paragraph.
> 
> The time when the hardware operates on these invisible scanlines is
> referred to as vertical blanking period, or simply vblank.
> 
> > + *
> > + * On a lot of display hardware, programming needs to take effect during the
> > + * vertical blanking period so that settings like gamma, what frame we're
> 
> "we" again
> 
> > + * scanning out, etc. can be safely changed without showing visual tearing
> > + * on the screen. In some unforgiving hardware, some of this programming has
> > + * to both start and end in the same vblank - vertical blanking.
> > + *
> >   * Vertical blanking plays a major role in graphics rendering. To achieve
> >   * tear-free display, users must synchronize page flips and/or rendering to
> >   * vertical blanking. The DRM API offers ioctls to perform page flips
> > 
> 
> In any case
> 
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

Thanks for the detailed feedabck - I have tried to reword it so it
better fits the context and have taking into account your suggetions.
See other mail for the updated patch.

	Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/6] drm/vblank: Add intro to documentation

[Sam Ravnborg]

Lyude Paul wrote a very good intro to vblank here:
https://lore.kernel.org/dri-devel/faf63d8a9ed23c16af69762f59d0dca6b2bf085f.camel@redhat.com/T/#mce6480be738160e9d07c5d023e88fd78d7a06d27

Add this to the intro chapter in drm_vblank.c so others
can benefit from it too.

v2:
  - Reworded to improve readability (Thomas)

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Co-developed-by: Lyude Paul <lyude@redhat.com>
Cc: Lyude Paul <lyude@redhat.com>
Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
---
 drivers/gpu/drm/drm_vblank.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index bcf346b3e486..ec2c2083b186 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -41,6 +41,23 @@
 /**
  * DOC: vblank handling
  *
+ * From the computer's perspective, every time the monitor displays
+ * a new frame the scanout engine have "scanned out" the display image
+ * from top to bottom, one row of pixels at a time.
+ * The current row of pixels is referred to as the current scanline.
+ *
+ * In addition to the display's visible area, there's usually a couple of
+ * extra scanlines which aren't actually displayed on the screen
+ * (the extra scanlines are sometimes used by HDMI audio and friends).
+ * The period where the extra scanlines are "scanned out" is referred
+ * to as the vertical blanking period (vblank for short).
+ *
+ * On a lot of display hardware, programming needs to take effect during the
+ * vertical blanking period so that settings like gamma, what frame being
+ * scanned out, etc. can be safely changed without showing visual tearing
+ * on the screen. In some unforgiving hardware, some of this programming has
+ * to both start and end in the same vblank - vertical blanking period.
+ *
  * Vertical blanking plays a major role in graphics rendering. To achieve
  * tear-free display, users must synchronize page flips and/or rendering to
  * vertical blanking. The DRM API offers ioctls to perform page flips
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/6] drm/vblank: Add intro to documentation

[Lyude Paul]

I am glad that my explanation of vblanks made sense! Some comments below on
things I think we could improve here

On Mon, 2020-03-30 at 20:57 +0200, Sam Ravnborg wrote:
> Lyude Paul wrote a very good intro to vblank here:
> 
https://lore.kernel.org/dri-devel/faf63d8a9ed23c16af69762f59d0dca6b2bf085f.camel@redhat.com/T/#mce6480be738160e9d07c5d023e88fd78d7a06d27
> 
> Add this to the intro chapter in drm_vblank.c so others
> can benefit from it too.
> 
> v2:
>   - Reworded to improve readability (Thomas)
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Co-developed-by: Lyude Paul <lyude@redhat.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> ---
>  drivers/gpu/drm/drm_vblank.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index bcf346b3e486..ec2c2083b186 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -41,6 +41,23 @@
>  /**
>   * DOC: vblank handling
>   *
> + * From the computer's perspective, every time the monitor displays
> + * a new frame the scanout engine have "scanned out" the display image
> + * from top to bottom, one row of pixels at a time.
> + * The current row of pixels is referred to as the current scanline.
> + *
> + * In addition to the display's visible area, there's usually a couple of
> + * extra scanlines which aren't actually displayed on the screen
> + * (the extra scanlines are sometimes used by HDMI audio and friends).
> + * The period where the extra scanlines are "scanned out" is referred
> + * to as the vertical blanking period (vblank for short).

I'd reword this, starting from "(the extra scanlines…" (I'd also remove the
paranthesis):

    These extra scanlines don't contain image data, and are occasionally used
    for features like audio and infoframes. The region made up of these
    scanlines is referred to as the vertical blanking region, or vblank for
    short.

I'd also add a simple ascii-art diagram here, since this might make a lot more
sense to some people if there's a visual reference. Probably something like
this (feel free to get a little creative)

     ⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽
    |                                                |
    |                                                |
    |                   New frame                    |
    |                                                |
    |↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓|
    |~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~| ← Scanline, updates the
    |↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓|   frame as it travels
    |                                                |   down (AKA "scans out")
    |                                                |
    |                                                |
    |                   Old frame                    |
    |                                                |
    |                                                |
    |                                                |
    |                                                |
    |                                                |   physical bottom of
    |⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽| ← display
    ┆xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx┆
    ┆xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx┆ ← vertical blanking
    ┆xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx┆   region
     ------------------------------------------------
> + *
> + * On a lot of display hardware, programming needs to take effect during
> the
> + * vertical blanking period so that settings like gamma, what frame being

s/what frame being/which frame is being/

> + * scanned out, etc. can be safely changed without showing visual tearing
> + * on the screen. In some unforgiving hardware, some of this programming
> has
> + * to both start and end in the same vblank - vertical blanking period.

You can just say vblank here, since we already explained what the vertical
blanking period is up above.

Alex Deucher pointed out to me that it's probably also worth mentioning that not
all hardware actually fires off the vblank interrupt at the start of the
vertical blank, depending on the hardware the interrupt could also happen a few
scanlines after the start of vblank, a few scanlines before the start, somewhere
in the middle, at the end of the vblank, etc.

Other then that, this looks great so far! Feel free to cc me in the respin for
this patch and I'll be happy to give my r-b.

> + *
>   * Vertical blanking plays a major role in graphics rendering. To achieve
>   * tear-free display, users must synchronize page flips and/or rendering to
>   * vertical blanking. The DRM API offers ioctls to perform page flips
-- 
Cheers,
	Lyude Paul (she/her)
	Associate Software Engineer at Red Hat

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/6] drm/vblank: Add intro to documentation

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 6306 bytes --]

Hi,

just a few more nits below.

Am 30.03.20 um 23:51 schrieb Lyude Paul:
> I am glad that my explanation of vblanks made sense! Some comments below on
> things I think we could improve here
> 
> On Mon, 2020-03-30 at 20:57 +0200, Sam Ravnborg wrote:
>> Lyude Paul wrote a very good intro to vblank here:
>>
> https://lore.kernel.org/dri-devel/faf63d8a9ed23c16af69762f59d0dca6b2bf085f.camel@redhat.com/T/#mce6480be738160e9d07c5d023e88fd78d7a06d27
>>
>> Add this to the intro chapter in drm_vblank.c so others
>> can benefit from it too.
>>
>> v2:
>>   - Reworded to improve readability (Thomas)
>>
>> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
>> Co-developed-by: Lyude Paul <lyude@redhat.com>
>> Cc: Lyude Paul <lyude@redhat.com>
>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: David Airlie <airlied@linux.ie>
>> ---
>>  drivers/gpu/drm/drm_vblank.c | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
>> index bcf346b3e486..ec2c2083b186 100644
>> --- a/drivers/gpu/drm/drm_vblank.c
>> +++ b/drivers/gpu/drm/drm_vblank.c
>> @@ -41,6 +41,23 @@
>>  /**
>>   * DOC: vblank handling
>>   *
>> + * From the computer's perspective, every time the monitor displays
>> + * a new frame the scanout engine have "scanned out" the display image
>> + * from top to bottom, one row of pixels at a time.
>> + * The current row of pixels is referred to as the current scanline.
>> + *
>> + * In addition to the display's visible area, there's usually a couple of
>> + * extra scanlines which aren't actually displayed on the screen
>> + * (the extra scanlines are sometimes used by HDMI audio and friends).
>> + * The period where the extra scanlines are "scanned out" is referred
>> + * to as the vertical blanking period (vblank for short).
> 
> I'd reword this, starting from "(the extra scanlines…" (I'd also remove the
> paranthesis):
> 
>     These extra scanlines don't contain image data, and are occasionally used
>     for features like audio and infoframes. The region made up of these
>     scanlines is referred to as the vertical blanking region, or vblank for
>     short.
> 
> I'd also add a simple ascii-art diagram here, since this might make a lot more
> sense to some people if there's a visual reference. Probably something like
> this (feel free to get a little creative)
> 
>      ⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽
>     |                                                |
>     |                                                |
>     |                   New frame                    |
>     |                                                |
>     |↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓|
>     |~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~| ← Scanline, updates the
>     |↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓|   frame as it travels
>     |                                                |   down (AKA "scans out")
>     |                                                |
>     |                                                |
>     |                   Old frame                    |
>     |                                                |
>     |                                                |
>     |                                                |
>     |                                                |
>     |                                                |   physical bottom of
>     |⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽| ← display
>     ┆xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx┆
>     ┆xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx┆ ← vertical blanking
>     ┆xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx┆   region
>      ------------------------------------------------
>> + *
>> + * On a lot of display hardware, programming needs to take effect during
>> the
>> + * vertical blanking period so that settings like gamma, what frame being
> 
> s/what frame being/which frame is being/

I still had read it twice in either variant. Maybe:

s/what frame being scanned out/the displayed image buffer

> 
>> + * scanned out, etc. can be safely changed without showing visual tearing

I think tearing refers specifically to mid-frame buffer flips. Maybe

s/tearing/artifacts

or

s/tearing/distortion

Best regards
Thomas

>> + * on the screen. In some unforgiving hardware, some of this programming
>> has
>> + * to both start and end in the same vblank - vertical blanking period.
> 
> You can just say vblank here, since we already explained what the vertical
> blanking period is up above.
> 
> Alex Deucher pointed out to me that it's probably also worth mentioning that not
> all hardware actually fires off the vblank interrupt at the start of the
> vertical blank, depending on the hardware the interrupt could also happen a few
> scanlines after the start of vblank, a few scanlines before the start, somewhere
> in the middle, at the end of the vblank, etc.
> 
> Other then that, this looks great so far! Feel free to cc me in the respin for
> this patch and I'll be happy to give my r-b.
> 
>> + *
>>   * Vertical blanking plays a major role in graphics rendering. To achieve
>>   * tear-free display, users must synchronize page flips and/or rendering to
>>   * vertical blanking. The DRM API offers ioctls to perform page flips

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/6] drm/vblank: Add intro to documentation

[Daniel Vetter]

On Mon, Mar 30, 2020 at 05:51:26PM -0400, Lyude Paul wrote:
> I am glad that my explanation of vblanks made sense! Some comments below on
> things I think we could improve here
> 
> On Mon, 2020-03-30 at 20:57 +0200, Sam Ravnborg wrote:
> > Lyude Paul wrote a very good intro to vblank here:
> > 
> https://lore.kernel.org/dri-devel/faf63d8a9ed23c16af69762f59d0dca6b2bf085f.camel@redhat.com/T/#mce6480be738160e9d07c5d023e88fd78d7a06d27
> > 
> > Add this to the intro chapter in drm_vblank.c so others
> > can benefit from it too.
> > 
> > v2:
> >   - Reworded to improve readability (Thomas)
> > 
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Co-developed-by: Lyude Paul <lyude@redhat.com>
> > Cc: Lyude Paul <lyude@redhat.com>
> > Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: David Airlie <airlied@linux.ie>
> > ---
> >  drivers/gpu/drm/drm_vblank.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > index bcf346b3e486..ec2c2083b186 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -41,6 +41,23 @@
> >  /**
> >   * DOC: vblank handling
> >   *
> > + * From the computer's perspective, every time the monitor displays
> > + * a new frame the scanout engine have "scanned out" the display image
> > + * from top to bottom, one row of pixels at a time.
> > + * The current row of pixels is referred to as the current scanline.
> > + *
> > + * In addition to the display's visible area, there's usually a couple of
> > + * extra scanlines which aren't actually displayed on the screen
> > + * (the extra scanlines are sometimes used by HDMI audio and friends).
> > + * The period where the extra scanlines are "scanned out" is referred
> > + * to as the vertical blanking period (vblank for short).
> 
> I'd reword this, starting from "(the extra scanlines…" (I'd also remove the
> paranthesis):
> 
>     These extra scanlines don't contain image data, and are occasionally used
>     for features like audio and infoframes. The region made up of these
>     scanlines is referred to as the vertical blanking region, or vblank for
>     short.
> 
> I'd also add a simple ascii-art diagram here, since this might make a lot more
> sense to some people if there's a visual reference. Probably something like
> this (feel free to get a little creative)
> 
>      ⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽
>     |                                                |
>     |                                                |
>     |                   New frame                    |
>     |                                                |
>     |↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓|
>     |~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~| ← Scanline, updates the
>     |↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓|   frame as it travels
>     |                                                |   down (AKA "scans out")
>     |                                                |
>     |                                                |
>     |                   Old frame                    |
>     |                                                |
>     |                                                |
>     |                                                |
>     |                                                |
>     |                                                |   physical bottom of
>     |⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽| ← display
>     ┆xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx┆
>     ┆xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx┆ ← vertical blanking
>     ┆xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx┆   region
>      ------------------------------------------------

Oh if we go with a nice asci-art picture, can we please also make a note
that top of the frame is where the corrected/high-precision timestamp
should match?
-Daniel

> > + *
> > + * On a lot of display hardware, programming needs to take effect during
> > the
> > + * vertical blanking period so that settings like gamma, what frame being
> 
> s/what frame being/which frame is being/
> 
> > + * scanned out, etc. can be safely changed without showing visual tearing
> > + * on the screen. In some unforgiving hardware, some of this programming
> > has
> > + * to both start and end in the same vblank - vertical blanking period.
> 
> You can just say vblank here, since we already explained what the vertical
> blanking period is up above.
> 
> Alex Deucher pointed out to me that it's probably also worth mentioning that not
> all hardware actually fires off the vblank interrupt at the start of the
> vertical blank, depending on the hardware the interrupt could also happen a few
> scanlines after the start of vblank, a few scanlines before the start, somewhere
> in the middle, at the end of the vblank, etc.
> 
> Other then that, this looks great so far! Feel free to cc me in the respin for
> this patch and I'll be happy to give my r-b.
> 
> > + *
> >   * Vertical blanking plays a major role in graphics rendering. To achieve
> >   * tear-free display, users must synchronize page flips and/or rendering to
> >   * vertical blanking. The DRM API offers ioctls to perform page flips
> -- 
> Cheers,
> 	Lyude Paul (she/her)
> 	Associate Software Engineer at Red Hat
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v1 2/6] drm/fb: fix kernel-doc in drm_framebuffer.h

[Sam Ravnborg]

Fix following warnings:
drm_framebuffer.h:342: warning: Function parameter or member 'block_width' not described in 'drm_afbc_framebuffer'
drm_framebuffer.h:342: warning: Function parameter or member 'block_height' not described in 'drm_afbc_framebuffer'

Trivial spelling mistakes.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Fixes: 55f7f72753ab ("drm/core: Add drm_afbc_framebuffer and a corresponding helper")
Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Cc: Emil Velikov <emil.velikov@collabora.com>
Cc: James Qian Wang <james.qian.wang@arm.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 include/drm/drm_framebuffer.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
index e9f1b0e2968d..b53c0332f040 100644
--- a/include/drm/drm_framebuffer.h
+++ b/include/drm/drm_framebuffer.h
@@ -308,11 +308,11 @@ struct drm_afbc_framebuffer {
 	 */
 	struct drm_framebuffer base;
 	/**
-	 * @block_widht: width of a single afbc block
+	 * @block_width: width of a single afbc block
 	 */
 	u32 block_width;
 	/**
-	 * @block_widht: height of a single afbc block
+	 * @block_height: height of a single afbc block
 	 */
 	u32 block_height;
 	/**
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 2/6] drm/fb: fix kernel-doc in drm_framebuffer.h

[Andrzej Pietrasiewicz]

W dniu 28.03.2020 o 14:20, Sam Ravnborg pisze:
> Fix following warnings:
> drm_framebuffer.h:342: warning: Function parameter or member 'block_width' not described in 'drm_afbc_framebuffer'
> drm_framebuffer.h:342: warning: Function parameter or member 'block_height' not described in 'drm_afbc_framebuffer'
> 
> Trivial spelling mistakes.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Fixes: 55f7f72753ab ("drm/core: Add drm_afbc_framebuffer and a corresponding helper")
> Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> Cc: Emil Velikov <emil.velikov@collabora.com>
> Cc: James Qian Wang <james.qian.wang@arm.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>   include/drm/drm_framebuffer.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> index e9f1b0e2968d..b53c0332f040 100644
> --- a/include/drm/drm_framebuffer.h
> +++ b/include/drm/drm_framebuffer.h
> @@ -308,11 +308,11 @@ struct drm_afbc_framebuffer {
>   	 */
>   	struct drm_framebuffer base;
>   	/**
> -	 * @block_widht: width of a single afbc block
> +	 * @block_width: width of a single afbc block
>   	 */
>   	u32 block_width;
>   	/**
> -	 * @block_widht: height of a single afbc block
> +	 * @block_height: height of a single afbc block
>   	 */
>   	u32 block_height;
>   	/**
> 

Acked-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 2/6] drm/fb: fix kernel-doc in drm_framebuffer.h

[Sam Ravnborg]

Hi Andrzej

On Mon, Mar 30, 2020 at 01:35:18PM +0200, Andrzej Pietrasiewicz wrote:
> W dniu 28.03.2020 o 14:20, Sam Ravnborg pisze:
> > Fix following warnings:
> > drm_framebuffer.h:342: warning: Function parameter or member 'block_width' not described in 'drm_afbc_framebuffer'
> > drm_framebuffer.h:342: warning: Function parameter or member 'block_height' not described in 'drm_afbc_framebuffer'
> > 
> > Trivial spelling mistakes.
> > 
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Fixes: 55f7f72753ab ("drm/core: Add drm_afbc_framebuffer and a corresponding helper")
> > Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
...
> 
> Acked-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

Thanks. Added a-b and applied to drm-misc-next.

	Sam

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v1 3/6] drm/sched: fix kernel-doc in gpu_scheduler.h

[Sam Ravnborg]

Fix following warning:
gpu_scheduler.h:103: warning: Function parameter or member 'priority' not described in 'drm_sched_entity'

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Nirmoy Das <nirmoy.das@amd.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 include/drm/gpu_scheduler.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 26b04ff62676..a21b3b92135a 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -56,6 +56,7 @@ enum drm_sched_priority {
  *              Jobs from this entity can be scheduled on any scheduler
  *              on this list.
  * @num_sched_list: number of drm_gpu_schedulers in the sched_list.
+ * @priority: priority of the entity
  * @rq_lock: lock to modify the runqueue to which this entity belongs.
  * @job_queue: the list of jobs of this entity.
  * @fence_seq: a linearly increasing seqno incremented with each
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 3/6] drm/sched: fix kernel-doc in gpu_scheduler.h

[Daniel Vetter]

On Sat, Mar 28, 2020 at 02:20:22PM +0100, Sam Ravnborg wrote:
> Fix following warning:
> gpu_scheduler.h:103: warning: Function parameter or member 'priority' not described in 'drm_sched_entity'
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Nirmoy Das <nirmoy.das@amd.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>

Inline struct comments would be really nice here. Otherwise

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  include/drm/gpu_scheduler.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 26b04ff62676..a21b3b92135a 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -56,6 +56,7 @@ enum drm_sched_priority {
>   *              Jobs from this entity can be scheduled on any scheduler
>   *              on this list.
>   * @num_sched_list: number of drm_gpu_schedulers in the sched_list.
> + * @priority: priority of the entity
>   * @rq_lock: lock to modify the runqueue to which this entity belongs.
>   * @job_queue: the list of jobs of this entity.
>   * @fence_seq: a linearly increasing seqno incremented with each
> -- 
> 2.20.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 3/6] drm/sched: fix kernel-doc in gpu_scheduler.h

[Sam Ravnborg]

On Sat, Mar 28, 2020 at 02:20:22PM +0100, Sam Ravnborg wrote:
> Fix following warning:
> gpu_scheduler.h:103: warning: Function parameter or member 'priority' not described in 'drm_sched_entity'
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Nirmoy Das <nirmoy.das@amd.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  include/drm/gpu_scheduler.h | 1 +
>  1 file changed, 1 insertion(+)

Committed to drm-misc-next

> 
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 26b04ff62676..a21b3b92135a 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -56,6 +56,7 @@ enum drm_sched_priority {
>   *              Jobs from this entity can be scheduled on any scheduler
>   *              on this list.
>   * @num_sched_list: number of drm_gpu_schedulers in the sched_list.
> + * @priority: priority of the entity
>   * @rq_lock: lock to modify the runqueue to which this entity belongs.
>   * @job_queue: the list of jobs of this entity.
>   * @fence_seq: a linearly increasing seqno incremented with each
> -- 
> 2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v1 4/6] drm: writeback: document callbacks

[Sam Ravnborg]

Document the callbacks:
    drm_connector_helper_funcs.prepare_writeback_job
    drm_connector_helper_funcs.cleanup_writeback_job

The documentation was pulled from the changelong introducing the
callbacks, originally written by Laurent.

Addign the missing documentation fixes the following warnings:
drm_modeset_helper_vtables.h:1052: warning: Function parameter or member 'prepare_writeback_job' not described in 'drm_connector_helper_funcs'
drm_modeset_helper_vtables.h:1052: warning: Function parameter or member 'cleanup_writeback_job' not described in 'drm_connector_helper_funcs'

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
---
 include/drm/drm_modeset_helper_vtables.h | 31 ++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 7c20b1c8b6a7..c51bca1ffec7 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -1075,8 +1075,39 @@ struct drm_connector_helper_funcs {
 	void (*atomic_commit)(struct drm_connector *connector,
 			      struct drm_connector_state *state);
 
+	/**
+	 * @prepare_writeback_job:
+	 *
+	 * As writeback jobs contain a framebuffer, drivers may need to
+	 * prepare and cleanup them the same way they can prepare and
+	 * cleanup framebuffers for planes.
+	 * This optional connector operation is used to support the
+	 * preparation of writeback jobs.
+	 * The job prepare operation is called from
+	 * drm_atomic_helper_prepare_planes() to avoid a new atomic commit
+	 * helper that would need to be called by all drivers not using
+	 * drm_atomic_helper_commit().
+	 *
+	 * This hook is optional.
+	 *
+	 * This callback is used by the atomic modeset helpers.
+	 */
 	int (*prepare_writeback_job)(struct drm_writeback_connector *connector,
 				     struct drm_writeback_job *job);
+	/**
+	 * @cleanup_writeback_job:
+	 *
+	 * This optional connector operation is used to support the
+	 * cleanup of writeback jobs.
+	 * The job cleanup operation is called from the existing
+	 * drm_writeback_cleanup_job() function, invoked both when
+	 * destroying the job as part of a aborted commit, or when
+	 * the job completes.
+	 *
+	 * This hook is optional.
+	 *
+	 * This callback is used by the atomic modeset helpers.
+	 */
 	void (*cleanup_writeback_job)(struct drm_writeback_connector *connector,
 				      struct drm_writeback_job *job);
 };
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 4/6] drm: writeback: document callbacks

[Daniel Vetter]

On Sat, Mar 28, 2020 at 02:20:23PM +0100, Sam Ravnborg wrote:
> Document the callbacks:
>     drm_connector_helper_funcs.prepare_writeback_job
>     drm_connector_helper_funcs.cleanup_writeback_job
> 
> The documentation was pulled from the changelong introducing the
> callbacks, originally written by Laurent.
> 
> Addign the missing documentation fixes the following warnings:
> drm_modeset_helper_vtables.h:1052: warning: Function parameter or member 'prepare_writeback_job' not described in 'drm_connector_helper_funcs'
> drm_modeset_helper_vtables.h:1052: warning: Function parameter or member 'cleanup_writeback_job' not described in 'drm_connector_helper_funcs'
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> ---
>  include/drm/drm_modeset_helper_vtables.h | 31 ++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 7c20b1c8b6a7..c51bca1ffec7 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -1075,8 +1075,39 @@ struct drm_connector_helper_funcs {
>  	void (*atomic_commit)(struct drm_connector *connector,
>  			      struct drm_connector_state *state);
>  
> +	/**
> +	 * @prepare_writeback_job:

Formatting looks funny, your linebreaks here won't go into the generated
html and are a bit unusual. I'd remove them and just flow this as a full
paragraph.

> +	 *
> +	 * As writeback jobs contain a framebuffer, drivers may need to
> +	 * prepare and cleanup them the same way they can prepare and
> +	 * cleanup framebuffers for planes.
> +	 * This optional connector operation is used to support the
> +	 * preparation of writeback jobs.
> +	 * The job prepare operation is called from
> +	 * drm_atomic_helper_prepare_planes() to avoid a new atomic commit
> +	 * helper that would need to be called by all drivers not using
> +	 * drm_atomic_helper_commit().

I'd delete "to avoid a new ..." until the end of the sentence. That feels
more like stuff in the commit message/review than kernel docs for driver
writers.

Instead maybe add "... for struct &drm_writeback_connector connectors
only." This gives us a nice link to the writeback docs, and makes it clear
that this isn't some general prep/cleanup thing. Similar addition below.

> +	 *
> +	 * This hook is optional.
> +	 *
> +	 * This callback is used by the atomic modeset helpers.
> +	 */
>  	int (*prepare_writeback_job)(struct drm_writeback_connector *connector,
>  				     struct drm_writeback_job *job);
> +	/**
> +	 * @cleanup_writeback_job:
> +	 *
> +	 * This optional connector operation is used to support the
> +	 * cleanup of writeback jobs.
> +	 * The job cleanup operation is called from the existing
> +	 * drm_writeback_cleanup_job() function, invoked both when
> +	 * destroying the job as part of a aborted commit, or when
> +	 * the job completes.
> +	 *
> +	 * This hook is optional.
> +	 *
> +	 * This callback is used by the atomic modeset helpers.
> +	 */
>  	void (*cleanup_writeback_job)(struct drm_writeback_connector *connector,
>  				      struct drm_writeback_job *job);

With the bikesheds addressed as you see fit:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Also Laurent owes you one, I've been pestering to fill this gap in his
docs since forever ...

Cheers, Daniel

>  };
> -- 
> 2.20.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 4/6] drm: writeback: document callbacks

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Tue, Mar 31, 2020 at 09:53:18AM +0200, Daniel Vetter wrote:
> On Sat, Mar 28, 2020 at 02:20:23PM +0100, Sam Ravnborg wrote:
> > Document the callbacks:
> >     drm_connector_helper_funcs.prepare_writeback_job
> >     drm_connector_helper_funcs.cleanup_writeback_job
> > 
> > The documentation was pulled from the changelong introducing the
> > callbacks, originally written by Laurent.
> > 
> > Addign the missing documentation fixes the following warnings:
> > drm_modeset_helper_vtables.h:1052: warning: Function parameter or member 'prepare_writeback_job' not described in 'drm_connector_helper_funcs'
> > drm_modeset_helper_vtables.h:1052: warning: Function parameter or member 'cleanup_writeback_job' not described in 'drm_connector_helper_funcs'
> > 
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: David Airlie <airlied@linux.ie>
> > ---
> >  include/drm/drm_modeset_helper_vtables.h | 31 ++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> > 
> > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > index 7c20b1c8b6a7..c51bca1ffec7 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -1075,8 +1075,39 @@ struct drm_connector_helper_funcs {
> >  	void (*atomic_commit)(struct drm_connector *connector,
> >  			      struct drm_connector_state *state);
> >  
> > +	/**
> > +	 * @prepare_writeback_job:
> 
> Formatting looks funny, your linebreaks here won't go into the generated
> html and are a bit unusual. I'd remove them and just flow this as a full
> paragraph.
> 
> > +	 *
> > +	 * As writeback jobs contain a framebuffer, drivers may need to
> > +	 * prepare and cleanup them the same way they can prepare and

"cleanup them" or "clean them up" ?

> > +	 * cleanup framebuffers for planes.

This would be "clean up" too.

> > +	 * This optional connector operation is used to support the
> > +	 * preparation of writeback jobs.
> > +	 * The job prepare operation is called from
> > +	 * drm_atomic_helper_prepare_planes() to avoid a new atomic commit
> > +	 * helper that would need to be called by all drivers not using
> > +	 * drm_atomic_helper_commit().
> 
> I'd delete "to avoid a new ..." until the end of the sentence. That feels
> more like stuff in the commit message/review than kernel docs for driver
> writers.
> 
> Instead maybe add "... for struct &drm_writeback_connector connectors
> only." This gives us a nice link to the writeback docs, and makes it clear
> that this isn't some general prep/cleanup thing. Similar addition below.
> 
> > +	 *
> > +	 * This hook is optional.
> > +	 *
> > +	 * This callback is used by the atomic modeset helpers.

"hook" or "callback", you decide, but let's be consistent :-) I'd go for
"operation" personally as that's what is used above. Same for the
cleanup_writeback_job operation below.

> > +	 */
> >  	int (*prepare_writeback_job)(struct drm_writeback_connector *connector,
> >  				     struct drm_writeback_job *job);
> > +	/**
> > +	 * @cleanup_writeback_job:
> > +	 *
> > +	 * This optional connector operation is used to support the
> > +	 * cleanup of writeback jobs.
> > +	 * The job cleanup operation is called from the existing
> > +	 * drm_writeback_cleanup_job() function, invoked both when
> > +	 * destroying the job as part of a aborted commit, or when

s/a aborted/an aborted/

> > +	 * the job completes.
> > +	 *
> > +	 * This hook is optional.
> > +	 *
> > +	 * This callback is used by the atomic modeset helpers.
> > +	 */
> >  	void (*cleanup_writeback_job)(struct drm_writeback_connector *connector,
> >  				      struct drm_writeback_job *job);
> 
> With the bikesheds addressed as you see fit:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Also Laurent owes you one, I've been pestering to fill this gap in his
> docs since forever ...

I do. Sorry for letting you fix it.

> >  };

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v1 5/6] drm/dp_mst: add kernel-doc for drm_dp_mst_port.fec_capable

[Sam Ravnborg]

Fix kernel-doc warnings for drm_dp_mst_port.fec_capable.
This fixed the following warning:
drm_dp_mst_helper.h:162: warning: Function parameter or member 'fec_capable' not described in 'drm_dp_mst_port'

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: David Francis <David.Francis@amd.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Mikita Lipski <mikita.lipski@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 include/drm/drm_dp_mst_helper.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index bf5e65d2303e..d93e628ebc84 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -157,6 +157,10 @@ struct drm_dp_mst_port {
 	 */
 	bool has_audio;
 
+	/**
+	 * @fec_capable: bool indicating if FEC can be supported
+	 * up to that point in the MST network.
+	 */
 	bool fec_capable;
 };
 
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 5/6] drm/dp_mst: add kernel-doc for drm_dp_mst_port.fec_capable

[Lyude Paul]

On Sat, 2020-03-28 at 14:20 +0100, Sam Ravnborg wrote:
> Fix kernel-doc warnings for drm_dp_mst_port.fec_capable.
> This fixed the following warning:
> drm_dp_mst_helper.h:162: warning: Function parameter or member 'fec_capable'
> not described in 'drm_dp_mst_port'
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: David Francis <David.Francis@amd.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Mikita Lipski <mikita.lipski@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  include/drm/drm_dp_mst_helper.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/drm/drm_dp_mst_helper.h
> b/include/drm/drm_dp_mst_helper.h
> index bf5e65d2303e..d93e628ebc84 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -157,6 +157,10 @@ struct drm_dp_mst_port {
>  	 */
>  	bool has_audio;
>  
> +	/**
> +	 * @fec_capable: bool indicating if FEC can be supported
> +	 * up to that point in the MST network.

s/network/topology, but I can just fix that locally and push this in just a
moment. Thanks!

Reviewed-by: Lyude Paul <lyude@redhat.com>

> +	 */
>  	bool fec_capable;
>  };
>  
-- 
Cheers,
	Lyude Paul (she/her)
	Associate Software Engineer at Red Hat

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 5/6] drm/dp_mst: add kernel-doc for drm_dp_mst_port.fec_capable

[Sam Ravnborg]

Hi Lyude.

On Mon, Mar 30, 2020 at 11:01:12AM -0400, Lyude Paul wrote:
> On Sat, 2020-03-28 at 14:20 +0100, Sam Ravnborg wrote:
> > Fix kernel-doc warnings for drm_dp_mst_port.fec_capable.
> > This fixed the following warning:
> > drm_dp_mst_helper.h:162: warning: Function parameter or member 'fec_capable'
> > not described in 'drm_dp_mst_port'
> > 
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: David Francis <David.Francis@amd.com>
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: Mikita Lipski <mikita.lipski@amd.com>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > ---
> >  include/drm/drm_dp_mst_helper.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/include/drm/drm_dp_mst_helper.h
> > b/include/drm/drm_dp_mst_helper.h
> > index bf5e65d2303e..d93e628ebc84 100644
> > --- a/include/drm/drm_dp_mst_helper.h
> > +++ b/include/drm/drm_dp_mst_helper.h
> > @@ -157,6 +157,10 @@ struct drm_dp_mst_port {
> >  	 */
> >  	bool has_audio;
> >  
> > +	/**
> > +	 * @fec_capable: bool indicating if FEC can be supported
> > +	 * up to that point in the MST network.
> 
> s/network/topology, but I can just fix that locally and push this in just a
> moment. Thanks!
> 
> Reviewed-by: Lyude Paul <lyude@redhat.com>

Thanks for fixing and applying!

Can you also take a look at PATCH 1/6 and if OK provide your s-o-b
that should follow the Co-developed-by: ...
I know the text has seen a few changes but the original source came from
you.

	Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v1 6/6] drm/bridge: fix kernel-doc warning in panel.c

[Sam Ravnborg]

Add missing documentation to fix following warning:
panel.c:303: warning: Function parameter or member 'bridge' not described in 'drm_panel_bridge_connector'

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Mihail Atanassov <Mihail.Atanassov@arm.com>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/gpu/drm/bridge/panel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 8461ee8304ba..e933f1c47f5d 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -311,6 +311,7 @@ EXPORT_SYMBOL(devm_drm_panel_bridge_add_typed);
 
 /**
  * drm_panel_bridge_connector - return the connector for the panel bridge
+ * @bridge: The drm_bridge.
  *
  * drm_panel_bridge creates the connector.
  * This function gives external access to the connector.
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 6/6] drm/bridge: fix kernel-doc warning in panel.c

[Daniel Vetter]

On Sat, Mar 28, 2020 at 02:20:25PM +0100, Sam Ravnborg wrote:
> Add missing documentation to fix following warning:
> panel.c:303: warning: Function parameter or member 'bridge' not described in 'drm_panel_bridge_connector'
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> Cc: Mihail Atanassov <Mihail.Atanassov@arm.com>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/bridge/panel.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 8461ee8304ba..e933f1c47f5d 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -311,6 +311,7 @@ EXPORT_SYMBOL(devm_drm_panel_bridge_add_typed);
>  
>  /**
>   * drm_panel_bridge_connector - return the connector for the panel bridge
> + * @bridge: The drm_bridge.
>   *
>   * drm_panel_bridge creates the connector.
>   * This function gives external access to the connector.
> -- 
> 2.20.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 6/6] drm/bridge: fix kernel-doc warning in panel.c

[Sam Ravnborg]

On Sat, Mar 28, 2020 at 02:20:25PM +0100, Sam Ravnborg wrote:
> Add missing documentation to fix following warning:
> panel.c:303: warning: Function parameter or member 'bridge' not described in 'drm_panel_bridge_connector'
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> Cc: Mihail Atanassov <Mihail.Atanassov@arm.com>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/gpu/drm/bridge/panel.c | 1 +
>  1 file changed, 1 insertion(+)

Committed to drm-misc-next

> 
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 8461ee8304ba..e933f1c47f5d 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -311,6 +311,7 @@ EXPORT_SYMBOL(devm_drm_panel_bridge_add_typed);
>  
>  /**
>   * drm_panel_bridge_connector - return the connector for the panel bridge
> + * @bridge: The drm_bridge.
>   *
>   * drm_panel_bridge creates the connector.
>   * This function gives external access to the connector.
> -- 
> 2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel