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

[Andrzej Hajda <andrzej.hajda@xxxxxxxxx>]

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.hajda@xxxxxxxxx>
> > ---
> >   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.vivi@xxxxxxxxx>
>
> 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