Re: [Intel-gfx] [PATCH v2 6/9] drm/i915/display/hdmi: use intel_de_rmw if possible

[Andrzej Hajda]

On 06.01.2023 16:35, Rodrigo Vivi wrote:

On Thu, Jan 05, 2023 at 02:10:43PM +0100, Andrzej Hajda wrote:

The helper makes the code more compact and readable.

Signed-off-by: Andrzej Hajda <andrzej.ha...@intel.com>
---
  drivers/gpu/drm/i915/display/g4x_hdmi.c   |  8 ++---
  drivers/gpu/drm/i915/display/intel_hdcp.c | 15 ++++-----
  drivers/gpu/drm/i915/display/intel_hdmi.c | 40 +++++++----------------
  3 files changed, 22 insertions(+), 41 deletions(-)

(...)

diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c 
b/drivers/gpu/drm/i915/display/intel_hdmi.c
index efa2da080f62d4..4b09f17aa4b23b 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -237,15 +237,11 @@ static void g4x_read_infoframe(struct intel_encoder 
*encoder,
                               void *frame, ssize_t len)
  {
        struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-       u32 val, *data = frame;
+       u32 *data = frame;
        int i;

- val = intel_de_read(dev_priv, VIDEO_DIP_CTL);

-
-       val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */

A probably good follow-up clean up would be to define the missing masks
and remove the hardcoded things like this 0xf.

And also something that I had noticed on the previous patches but I forgot
to mention: it would be good as a followup to replace the local value << shift
per FIELD_PREP() helpers and remove the shift definitions...

But really nothing related directly with this patch. For this:

Reviewed-by: Rodrigo Vivi <rodrigo.v...@intel.com>

Oh, and I also noticed that CI didn't return yet for these patches...
https://patchwork.freedesktop.org/series/112438/

a strange delay... I will probably hit the retest if we don't get
anything by the end of the day today.

Thx for reviews.
Apparently CI missed this series, I have just hit retest.

Regards
Andrzej

-       val |= g4x_infoframe_index(type);
-
-       intel_de_write(dev_priv, VIDEO_DIP_CTL, val);
+       intel_de_rmw(dev_priv, VIDEO_DIP_CTL,
+                    VIDEO_DIP_SELECT_MASK | 0xf, g4x_infoframe_index(type));

for (i = 0; i < len; i += 4)

                *data++ = intel_de_read(dev_priv, VIDEO_DIP_DATA);
@@ -313,15 +309,11 @@ static void ibx_read_infoframe(struct intel_encoder 
*encoder,
  {
        struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
        struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
-       u32 val, *data = frame;
+       u32 *data = frame;
        int i;

- val = intel_de_read(dev_priv, TVIDEO_DIP_CTL(crtc->pipe));

-
-       val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
-       val |= g4x_infoframe_index(type);
-
-       intel_de_write(dev_priv, TVIDEO_DIP_CTL(crtc->pipe), val);
+       intel_de_rmw(dev_priv, TVIDEO_DIP_CTL(crtc->pipe),
+                    VIDEO_DIP_SELECT_MASK | 0xf, g4x_infoframe_index(type));

for (i = 0; i < len; i += 4)

                *data++ = intel_de_read(dev_priv, TVIDEO_DIP_DATA(crtc->pipe));
@@ -395,15 +387,11 @@ static void cpt_read_infoframe(struct intel_encoder 
*encoder,
  {
        struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
        struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
-       u32 val, *data = frame;
+       u32 *data = frame;
        int i;

- val = intel_de_read(dev_priv, TVIDEO_DIP_CTL(crtc->pipe));

-
-       val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
-       val |= g4x_infoframe_index(type);
-
-       intel_de_write(dev_priv, TVIDEO_DIP_CTL(crtc->pipe), val);
+       intel_de_rmw(dev_priv, TVIDEO_DIP_CTL(crtc->pipe),
+                    VIDEO_DIP_SELECT_MASK | 0xf, g4x_infoframe_index(type));

for (i = 0; i < len; i += 4)

                *data++ = intel_de_read(dev_priv, TVIDEO_DIP_DATA(crtc->pipe));
@@ -471,15 +459,11 @@ static void vlv_read_infoframe(struct intel_encoder 
*encoder,
  {
        struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
        struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
-       u32 val, *data = frame;
+       u32 *data = frame;
        int i;

- val = intel_de_read(dev_priv, VLV_TVIDEO_DIP_CTL(crtc->pipe));

-
-       val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
-       val |= g4x_infoframe_index(type);
-
-       intel_de_write(dev_priv, VLV_TVIDEO_DIP_CTL(crtc->pipe), val);
+       intel_de_rmw(dev_priv, VLV_TVIDEO_DIP_CTL(crtc->pipe),
+                    VIDEO_DIP_SELECT_MASK | 0xf, g4x_infoframe_index(type));

for (i = 0; i < len; i += 4)

                *data++ = intel_de_read(dev_priv,
--
2.34.1