[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.hajda at 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.vivi at 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
>>