[PATCH 1/3] drm/todo: Create a TODO item for additional HDMI work

[PATCH 1/3] drm/todo: Create a TODO item for additional HDMI work

[Maxime Ripard]

We recently added some infrastructure to deal with HDMI but we're still
lacking a couple of things. Add a TODO entry with the remaining items.

Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 Documentation/gpu/todo.rst | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 2ea6ffc9b22b..52fd8672fb6d 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -633,10 +633,39 @@ long as that supports DMA. Otherwise importing can still needlessly fail.
 
 Contact: Thomas Zimmermann <tzimmermann@suse.de>, Daniel Vetter
 
 Level: Advanced
 
+Improve HDMI Infrastructure
+---------------------------
+
+We have a bunch of helpers to handle HDMI and reduce the boilerplate in
+drivers. Support so far includes HDMI 1.4 support, but we need to extend
+it with:
+
+  - CEC handling support. CEC requires a bit of integration into every
+    HDMI driver to set the device physical address according to the EDID
+    in `.get_modes`, and to clear/reset it in the hotplug detection
+    path. We should create the ``drm_atomic_helper_connector_hdmi_get_modes()``
+    and ``drm_atomic_helper_connector_hdmi_handle_hotplug()`` helpers to handle
+    this properly, and convert drivers to use them.
+
+  - In order to support HDMI 2.0 properly, the scrambler parameters need
+    to be moved into the state. This includes figuring out in
+    drm_atomic_helper_connector_hdmi_check() if the scrambler and TMDS ratio
+    need to be changed, and make the
+    ``drm_atomic_helper_connector_hdmi_handle_hotplug()`` helper reset the
+    scrambler status when the device is plugged and unplugged.
+
+  - We need to support YUV420 too.
+
+The `vc4` driver is a good example for all this.
+
+Contact: Maxime Ripard <mripard@kernel.org>
+
+Level: Intermediate
+
 
 Better Testing
 ==============
 
 Add unit tests using the Kernel Unit Testing (KUnit) framework
-- 
2.45.2

[PATCH 2/3] drm/todo: Create a TODO item for MIPI-DSI rework

[Maxime Ripard]

The MIPI-DSI infrastructure and setup we have so far is limited and
doesn't provide a good way to address some corner-cases.

Add a TODO entry to document what is needed.

Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 Documentation/gpu/todo.rst | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 52fd8672fb6d..94139c652663 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -633,10 +633,30 @@ long as that supports DMA. Otherwise importing can still needlessly fail.
 
 Contact: Thomas Zimmermann <tzimmermann@suse.de>, Daniel Vetter
 
 Level: Advanced
 
+Improve MIPI-DSI infrastructure
+-------------------------------
+
+The MIPI-DSI infrastructure in KMS supports MIPI-DSI devices in part
+through a dedicated bus (`drm_mipi_dsi.c`) and in part through ad-hoc
+calls in drivers, especially to handle the power state of the bus.
+
+This is an oddity as far as the device model is concerned, but is also
+an issue when multiple devices are chained (like bridges) and have
+different requirements.
+
+It would be a good idea to move all the power state handling out of KMS
+and into the bus support itself.
+
+See `[RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state <https://lore.kernel.org/dri-devel/20231016165355.1327217-4-dmitry.baryshkov@linaro.org/>`_
+
+Contact: Maxime Ripard <mripard@kernel.org>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
+
+Level: Advanced
+
 Improve HDMI Infrastructure
 ---------------------------
 
 We have a bunch of helpers to handle HDMI and reduce the boilerplate in
 drivers. Support so far includes HDMI 1.4 support, but we need to extend
-- 
2.45.2

[PATCH 3/3] drm/todo: Add TODO entry for "lints"

[Maxime Ripard]

Having lints would prove beneficial to prevent the same dark patterns
from reoccuring over and over again in drivers.

Add a TODO entry for that.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 Documentation/gpu/todo.rst | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 94139c652663..16a9a24b33a6 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -717,10 +717,38 @@ doesn't or document it if it does would be of great help.
 
 Contact: Maxime Ripard <mripard@kernel.org>
 
 Level: Intermediate
 
+Create lints for KMS drivers
+----------------------------
+
+Over time, we've accumulated a list of dark patterns in KMS drivers that
+should be avoided. However, none of them are published anywhere, and not
+all reviewers are aware of them. It creates a situation where we have
+more drivers with problematic / deprecated code, even though we know
+that they shouldn't.
+
+We should create a set of coccinelle scripts that match these patterns,
+and make new drivers run that list. And possibly integrate them in CI.
+
+These patterns include:
+
+  - Drivers using kzalloc() or devm_kzalloc() to allocate their memory,
+    instead of drmm_kzalloc().
+
+  - Drivers not protecting their device resources (MMIO, clocks,
+    regulators, etc.) by drm_dev_enter() and drm_dev_exit().
+
+  - Drivers using drm_dev_unregister() instead of drm_dev_unplug().
+
+  - Drivers not calling drm_atomic_helper_shutdown() at shutdown
+
+Contact: Maxime Ripard <mripard@kernel.org>
+
+Level: Intermediate
+
 Enable trinity for DRM
 ----------------------
 
 And fix up the fallout. Should be really interesting ...
 
-- 
2.45.2

Re: [PATCH 1/3] drm/todo: Create a TODO item for additional HDMI work

[Dmitry Baryshkov]

On Wed, 12 Jun 2024 at 17:35, Maxime Ripard <mripard@kernel.org> wrote:
>
> We recently added some infrastructure to deal with HDMI but we're still
> lacking a couple of things. Add a TODO entry with the remaining items.
>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  Documentation/gpu/todo.rst | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 2ea6ffc9b22b..52fd8672fb6d 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -633,10 +633,39 @@ long as that supports DMA. Otherwise importing can still needlessly fail.
>
>  Contact: Thomas Zimmermann <tzimmermann@suse.de>, Daniel Vetter
>
>  Level: Advanced
>
> +Improve HDMI Infrastructure
> +---------------------------
> +
> +We have a bunch of helpers to handle HDMI and reduce the boilerplate in
> +drivers. Support so far includes HDMI 1.4 support, but we need to extend
> +it with:
> +
> +  - CEC handling support. CEC requires a bit of integration into every
> +    HDMI driver to set the device physical address according to the EDID
> +    in `.get_modes`, and to clear/reset it in the hotplug detection
> +    path. We should create the ``drm_atomic_helper_connector_hdmi_get_modes()``
> +    and ``drm_atomic_helper_connector_hdmi_handle_hotplug()`` helpers to handle
> +    this properly, and convert drivers to use them.
> +
> +  - In order to support HDMI 2.0 properly, the scrambler parameters need
> +    to be moved into the state. This includes figuring out in
> +    drm_atomic_helper_connector_hdmi_check() if the scrambler and TMDS ratio
> +    need to be changed, and make the
> +    ``drm_atomic_helper_connector_hdmi_handle_hotplug()`` helper reset the
> +    scrambler status when the device is plugged and unplugged.
> +
> +  - We need to support YUV420 too.

- HDMI audio improvements:
  - Implement the get_eld() and possibly hook_plugged_cb() functions
for hdmi_codec_ops in a generic way. Maybe implementing generic HDMI
hdmi_codec_ops and then providing device-specific hooks to prepare /
shutdown / mute stream.
  - provide helpers to determine ACR / N / CTS params

A different, but closely related topic is HDCP support in a generic
way. https://lore.kernel.org/dri-devel/20230419154321.1993419-1-markyacoub@google.com/

> +
> +The `vc4` driver is a good example for all this.
> +
> +Contact: Maxime Ripard <mripard@kernel.org>
> +
> +Level: Intermediate
> +
>
>  Better Testing
>  ==============
>
>  Add unit tests using the Kernel Unit Testing (KUnit) framework
> --
> 2.45.2
>


-- 
With best wishes
Dmitry

Re: [PATCH 3/3] drm/todo: Add TODO entry for "lints"

[Dmitry Baryshkov]

On Wed, Jun 12, 2024 at 04:35:53PM +0200, Maxime Ripard wrote:
> Having lints would prove beneficial to prevent the same dark patterns
> from reoccuring over and over again in drivers.
> 
> Add a TODO entry for that.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  Documentation/gpu/todo.rst | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 94139c652663..16a9a24b33a6 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -717,10 +717,38 @@ doesn't or document it if it does would be of great help.
>  
>  Contact: Maxime Ripard <mripard@kernel.org>
>  
>  Level: Intermediate
>  
> +Create lints for KMS drivers
> +----------------------------
> +
> +Over time, we've accumulated a list of dark patterns in KMS drivers that
> +should be avoided. However, none of them are published anywhere, and not
> +all reviewers are aware of them. It creates a situation where we have
> +more drivers with problematic / deprecated code, even though we know
> +that they shouldn't.
> +
> +We should create a set of coccinelle scripts that match these patterns,
> +and make new drivers run that list. And possibly integrate them in CI.
> +
> +These patterns include:
> +
> +  - Drivers using kzalloc() or devm_kzalloc() to allocate their memory,
> +    instead of drmm_kzalloc().
> +
> +  - Drivers not protecting their device resources (MMIO, clocks,
> +    regulators, etc.) by drm_dev_enter() and drm_dev_exit().
> +
> +  - Drivers using drm_dev_unregister() instead of drm_dev_unplug().
> +
> +  - Drivers not calling drm_atomic_helper_shutdown() at shutdown

- Drivers setting state->allow_modeset manually.

> +
> +Contact: Maxime Ripard <mripard@kernel.org>
> +
> +Level: Intermediate
> +
>  Enable trinity for DRM
>  ----------------------
>  
>  And fix up the fallout. Should be really interesting ...
>  
> -- 
> 2.45.2
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/3] drm/todo: Create a TODO item for additional HDMI work

[Jani Nikula]

On Wed, 12 Jun 2024, Maxime Ripard <mripard@kernel.org> wrote:
> We recently added some infrastructure to deal with HDMI but we're still
> lacking a couple of things. Add a TODO entry with the remaining items.
>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  Documentation/gpu/todo.rst | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 2ea6ffc9b22b..52fd8672fb6d 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -633,10 +633,39 @@ long as that supports DMA. Otherwise importing can still needlessly fail.
>  
>  Contact: Thomas Zimmermann <tzimmermann@suse.de>, Daniel Vetter
>  
>  Level: Advanced
>  
> +Improve HDMI Infrastructure
> +---------------------------
> +
> +We have a bunch of helpers to handle HDMI and reduce the boilerplate in
> +drivers. Support so far includes HDMI 1.4 support, but we need to extend
> +it with:
> +
> +  - CEC handling support. CEC requires a bit of integration into every
> +    HDMI driver to set the device physical address according to the EDID
> +    in `.get_modes`, and to clear/reset it in the hotplug detection
> +    path. We should create the ``drm_atomic_helper_connector_hdmi_get_modes()``
> +    and ``drm_atomic_helper_connector_hdmi_handle_hotplug()`` helpers to handle
> +    this properly, and convert drivers to use them.

Furthermore, we should stop passing EDID to the CEC functions, and
instead use the source physical address we've parsed ourselves and
stored to connector->display_info.source_physical_address.

I.e. stop using

- drm_dp_cec_set_edid()
- cec_s_phys_addr_from_edid()
- cec_get_edid_phys_addr()

And instead use .source_physical_address and

- drm_dp_cec_attach()
- cec_s_phys_addr()

The main rationale is to avoid using a separate EDID parser that's
outside of the drm subsystem and unaware of struct drm_edid and frankly
cdoes not look very robust.


BR,
Jani.


> +
> +  - In order to support HDMI 2.0 properly, the scrambler parameters need
> +    to be moved into the state. This includes figuring out in
> +    drm_atomic_helper_connector_hdmi_check() if the scrambler and TMDS ratio
> +    need to be changed, and make the
> +    ``drm_atomic_helper_connector_hdmi_handle_hotplug()`` helper reset the
> +    scrambler status when the device is plugged and unplugged.
> +
> +  - We need to support YUV420 too.
> +
> +The `vc4` driver is a good example for all this.
> +
> +Contact: Maxime Ripard <mripard@kernel.org>
> +
> +Level: Intermediate
> +
>  
>  Better Testing
>  ==============
>  
>  Add unit tests using the Kernel Unit Testing (KUnit) framework

-- 
Jani Nikula, Intel

Re: [PATCH 1/3] drm/todo: Create a TODO item for additional HDMI work

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2408 bytes --]

Hi Jani,

On Thu, Jun 13, 2024 at 10:44:42AM GMT, Jani Nikula wrote:
> On Wed, 12 Jun 2024, Maxime Ripard <mripard@kernel.org> wrote:
> > We recently added some infrastructure to deal with HDMI but we're still
> > lacking a couple of things. Add a TODO entry with the remaining items.
> >
> > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >  Documentation/gpu/todo.rst | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >
> > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> > index 2ea6ffc9b22b..52fd8672fb6d 100644
> > --- a/Documentation/gpu/todo.rst
> > +++ b/Documentation/gpu/todo.rst
> > @@ -633,10 +633,39 @@ long as that supports DMA. Otherwise importing can still needlessly fail.
> >  
> >  Contact: Thomas Zimmermann <tzimmermann@suse.de>, Daniel Vetter
> >  
> >  Level: Advanced
> >  
> > +Improve HDMI Infrastructure
> > +---------------------------
> > +
> > +We have a bunch of helpers to handle HDMI and reduce the boilerplate in
> > +drivers. Support so far includes HDMI 1.4 support, but we need to extend
> > +it with:
> > +
> > +  - CEC handling support. CEC requires a bit of integration into every
> > +    HDMI driver to set the device physical address according to the EDID
> > +    in `.get_modes`, and to clear/reset it in the hotplug detection
> > +    path. We should create the ``drm_atomic_helper_connector_hdmi_get_modes()``
> > +    and ``drm_atomic_helper_connector_hdmi_handle_hotplug()`` helpers to handle
> > +    this properly, and convert drivers to use them.
> 
> Furthermore, we should stop passing EDID to the CEC functions, and
> instead use the source physical address we've parsed ourselves and
> stored to connector->display_info.source_physical_address.
> 
> I.e. stop using
> 
> - drm_dp_cec_set_edid()
> - cec_s_phys_addr_from_edid()
> - cec_get_edid_phys_addr()
> 
> And instead use .source_physical_address and
> 
> - drm_dp_cec_attach()
> - cec_s_phys_addr()
> 
> The main rationale is to avoid using a separate EDID parser that's
> outside of the drm subsystem and unaware of struct drm_edid and frankly
> cdoes not look very robust.

It's a good idea, but imo do not belong in that entry. I've added it to
the last patch of this series which fits better to me.

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]