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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 09 Jan 2023, Andrzej Hajda <andrzej.hajda@intel.com> wrote:
> 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@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@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.

Cc: Michal and Mateusz

This has been happening a lot lately.

BR,
Jani.


>
> 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
>>>
>

-- 
Jani Nikula, Intel Open Source Graphics Center