| Message ID | 20221011170011.17198-11-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: ELD precompute and readout | expand |
On Tue, 11 Oct 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Currently we only write as many dwords into the hardware > ELD buffers as drm_eld_size() tells us. That could mean the > remainder of the hardware buffer is left with whatever > stale garbage it had before, which doesn't seem entirely > great. Let's zero out the remainder of the buffer in case > the provided ELD doesn't fill it fully. > > We can also sanity check out idea of the hardware ELD buffer's > size by making sure the address wrapped back to zero once > we wrote the entire buffer. > > Cc: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> > Cc: Kai Vehmanen <kai.vehmanen@linux.intel.com> > Cc: Takashi Iwai <tiwai@suse.de> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_audio.c | 34 ++++++++++++++++------ > 1 file changed, 25 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c > index abca5f23673a..d2f9c4c29061 100644 > --- a/drivers/gpu/drm/i915/display/intel_audio.c > +++ b/drivers/gpu/drm/i915/display/intel_audio.c > @@ -333,19 +333,24 @@ static void g4x_audio_codec_enable(struct intel_encoder *encoder, > struct drm_i915_private *i915 = to_i915(encoder->base.dev); > struct drm_connector *connector = conn_state->connector; > const u8 *eld = connector->eld; > + int eld_buffer_size, len, i; > u32 tmp; > - int len, i; > > tmp = intel_de_read(i915, G4X_AUD_CNTL_ST); > tmp &= ~(G4X_ELD_VALID | G4X_ELD_ADDRESS_MASK); > intel_de_write(i915, G4X_AUD_CNTL_ST, tmp); > > - len = g4x_eld_buffer_size(i915); > - len = min(drm_eld_size(eld) / 4, len); > + eld_buffer_size = g4x_eld_buffer_size(i915); > + len = min(drm_eld_size(eld) / 4, eld_buffer_size); > > for (i = 0; i < len; i++) > intel_de_write(i915, G4X_HDMIW_HDMIEDID, > *((const u32 *)eld + i)); > + for (; i < eld_buffer_size; i++) > + intel_de_write(i915, G4X_HDMIW_HDMIEDID, 0); I think I'd personally write this along the lines of: eld_buffer_size = g4x_eld_buffer_size(i915); len = drm_eld_size(eld) / 4; for (i = 0; i < eld_buffer_size; i++) { u32 val = i < len ? *((const u32 *)eld + i)) : 0; intel_de_write(i915, G4X_HDMIW_HDMIEDID, val); } Get rid of two loops, the loop variable "leaking" from one to the next, the min() around len calculation, and multiple reg writes. Seems cleaner to me. Anyway, the patch does what it says on the box, Reviewed-by: Jani Nikula <jani.nikula@intel.com> > + > + drm_WARN_ON(&i915->drm, > + (intel_de_read(i915, G4X_AUD_CNTL_ST) & G4X_ELD_ADDRESS_MASK) != 0); > > tmp = intel_de_read(i915, G4X_AUD_CNTL_ST); > tmp |= G4X_ELD_VALID; > @@ -608,8 +613,8 @@ static void hsw_audio_codec_enable(struct intel_encoder *encoder, > struct drm_connector *connector = conn_state->connector; > enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > const u8 *eld = connector->eld; > + int eld_buffer_size, len, i; > u32 tmp; > - int len, i; > > mutex_lock(&i915->display.audio.mutex); > > @@ -635,12 +640,18 @@ static void hsw_audio_codec_enable(struct intel_encoder *encoder, > tmp &= ~IBX_ELD_ADDRESS_MASK; > intel_de_write(i915, HSW_AUD_DIP_ELD_CTRL(cpu_transcoder), tmp); > > - len = hsw_eld_buffer_size(i915, cpu_transcoder); > - len = min(drm_eld_size(eld) / 4, len); > + eld_buffer_size = hsw_eld_buffer_size(i915, cpu_transcoder); > + len = min(drm_eld_size(eld) / 4, eld_buffer_size); > > for (i = 0; i < len; i++) > intel_de_write(i915, HSW_AUD_EDID_DATA(cpu_transcoder), > *((const u32 *)eld + i)); > + for (; i < eld_buffer_size; i++) > + intel_de_write(i915, HSW_AUD_EDID_DATA(cpu_transcoder), 0); > + > + drm_WARN_ON(&i915->drm, > + (intel_de_read(i915, HSW_AUD_DIP_ELD_CTRL(cpu_transcoder)) & > + IBX_ELD_ADDRESS_MASK) != 0); > > /* ELD valid */ > tmp = intel_de_read(i915, HSW_AUD_PIN_ELD_CP_VLD); > @@ -738,8 +749,8 @@ static void ilk_audio_codec_enable(struct intel_encoder *encoder, > enum pipe pipe = crtc->pipe; > enum port port = encoder->port; > const u8 *eld = connector->eld; > + int eld_buffer_size, len, i; > struct ilk_audio_regs regs; > - int len, i; > u32 tmp; > > if (drm_WARN_ON(&i915->drm, port == PORT_A)) > @@ -766,12 +777,17 @@ static void ilk_audio_codec_enable(struct intel_encoder *encoder, > tmp &= ~IBX_ELD_ADDRESS_MASK; > intel_de_write(i915, regs.aud_cntl_st, tmp); > > - len = ilk_eld_buffer_size(i915, pipe); > - len = min(drm_eld_size(eld) / 4, len); > + eld_buffer_size = ilk_eld_buffer_size(i915, pipe); > + len = min(drm_eld_size(eld) / 4, eld_buffer_size); > > for (i = 0; i < len; i++) > intel_de_write(i915, regs.hdmiw_hdmiedid, > *((const u32 *)eld + i)); > + for (; i < eld_buffer_size; i++) > + intel_de_write(i915, regs.hdmiw_hdmiedid, 0); > + > + drm_WARN_ON(&i915->drm, > + (intel_de_read(i915, regs.aud_cntl_st) & IBX_ELD_ADDRESS_MASK) != 0); > > /* ELD valid */ > tmp = intel_de_read(i915, regs.aud_cntrl_st2);
On Wed, Oct 12, 2022 at 05:28:27PM +0300, Jani Nikula wrote: > On Tue, 11 Oct 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Currently we only write as many dwords into the hardware > > ELD buffers as drm_eld_size() tells us. That could mean the > > remainder of the hardware buffer is left with whatever > > stale garbage it had before, which doesn't seem entirely > > great. Let's zero out the remainder of the buffer in case > > the provided ELD doesn't fill it fully. > > > > We can also sanity check out idea of the hardware ELD buffer's > > size by making sure the address wrapped back to zero once > > we wrote the entire buffer. > > > > Cc: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> > > Cc: Kai Vehmanen <kai.vehmanen@linux.intel.com> > > Cc: Takashi Iwai <tiwai@suse.de> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_audio.c | 34 ++++++++++++++++------ > > 1 file changed, 25 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c > > index abca5f23673a..d2f9c4c29061 100644 > > --- a/drivers/gpu/drm/i915/display/intel_audio.c > > +++ b/drivers/gpu/drm/i915/display/intel_audio.c > > @@ -333,19 +333,24 @@ static void g4x_audio_codec_enable(struct intel_encoder *encoder, > > struct drm_i915_private *i915 = to_i915(encoder->base.dev); > > struct drm_connector *connector = conn_state->connector; > > const u8 *eld = connector->eld; > > + int eld_buffer_size, len, i; > > u32 tmp; > > - int len, i; > > > > tmp = intel_de_read(i915, G4X_AUD_CNTL_ST); > > tmp &= ~(G4X_ELD_VALID | G4X_ELD_ADDRESS_MASK); > > intel_de_write(i915, G4X_AUD_CNTL_ST, tmp); > > > > - len = g4x_eld_buffer_size(i915); > > - len = min(drm_eld_size(eld) / 4, len); > > + eld_buffer_size = g4x_eld_buffer_size(i915); > > + len = min(drm_eld_size(eld) / 4, eld_buffer_size); > > > > for (i = 0; i < len; i++) > > intel_de_write(i915, G4X_HDMIW_HDMIEDID, > > *((const u32 *)eld + i)); > > + for (; i < eld_buffer_size; i++) > > + intel_de_write(i915, G4X_HDMIW_HDMIEDID, 0); > > I think I'd personally write this along the lines of: > > eld_buffer_size = g4x_eld_buffer_size(i915); > len = drm_eld_size(eld) / 4; > > for (i = 0; i < eld_buffer_size; i++) { > u32 val = i < len ? *((const u32 *)eld + i)) : 0; > intel_de_write(i915, G4X_HDMIW_HDMIEDID, val); > } > > Get rid of two loops, the loop variable "leaking" from one to the next, > the min() around len calculation, and multiple reg writes. Seems cleaner > to me. I suppose. Though the double loop is what we already use in the infoframe code. So should probably change all of it at once if we decide that a single loop is the way to go.
On Wed, 12 Oct 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Wed, Oct 12, 2022 at 05:28:27PM +0300, Jani Nikula wrote: >> On Tue, 11 Oct 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > >> > Currently we only write as many dwords into the hardware >> > ELD buffers as drm_eld_size() tells us. That could mean the >> > remainder of the hardware buffer is left with whatever >> > stale garbage it had before, which doesn't seem entirely >> > great. Let's zero out the remainder of the buffer in case >> > the provided ELD doesn't fill it fully. >> > >> > We can also sanity check out idea of the hardware ELD buffer's >> > size by making sure the address wrapped back to zero once >> > we wrote the entire buffer. >> > >> > Cc: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> >> > Cc: Kai Vehmanen <kai.vehmanen@linux.intel.com> >> > Cc: Takashi Iwai <tiwai@suse.de> >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > --- >> > drivers/gpu/drm/i915/display/intel_audio.c | 34 ++++++++++++++++------ >> > 1 file changed, 25 insertions(+), 9 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c >> > index abca5f23673a..d2f9c4c29061 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_audio.c >> > +++ b/drivers/gpu/drm/i915/display/intel_audio.c >> > @@ -333,19 +333,24 @@ static void g4x_audio_codec_enable(struct intel_encoder *encoder, >> > struct drm_i915_private *i915 = to_i915(encoder->base.dev); >> > struct drm_connector *connector = conn_state->connector; >> > const u8 *eld = connector->eld; >> > + int eld_buffer_size, len, i; >> > u32 tmp; >> > - int len, i; >> > >> > tmp = intel_de_read(i915, G4X_AUD_CNTL_ST); >> > tmp &= ~(G4X_ELD_VALID | G4X_ELD_ADDRESS_MASK); >> > intel_de_write(i915, G4X_AUD_CNTL_ST, tmp); >> > >> > - len = g4x_eld_buffer_size(i915); >> > - len = min(drm_eld_size(eld) / 4, len); >> > + eld_buffer_size = g4x_eld_buffer_size(i915); >> > + len = min(drm_eld_size(eld) / 4, eld_buffer_size); >> > >> > for (i = 0; i < len; i++) >> > intel_de_write(i915, G4X_HDMIW_HDMIEDID, >> > *((const u32 *)eld + i)); >> > + for (; i < eld_buffer_size; i++) >> > + intel_de_write(i915, G4X_HDMIW_HDMIEDID, 0); >> >> I think I'd personally write this along the lines of: >> >> eld_buffer_size = g4x_eld_buffer_size(i915); >> len = drm_eld_size(eld) / 4; >> >> for (i = 0; i < eld_buffer_size; i++) { >> u32 val = i < len ? *((const u32 *)eld + i)) : 0; >> intel_de_write(i915, G4X_HDMIW_HDMIEDID, val); >> } >> >> Get rid of two loops, the loop variable "leaking" from one to the next, >> the min() around len calculation, and multiple reg writes. Seems cleaner >> to me. > > I suppose. Though the double loop is what we already use > in the infoframe code. So should probably change all of it > at once if we decide that a single loop is the way to go. IMO it would be nicer, but it's not a super important thing. Can go with this now, and decide later. As said, R-b on this as-is. BR, Jani.
diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c index abca5f23673a..d2f9c4c29061 100644 --- a/drivers/gpu/drm/i915/display/intel_audio.c +++ b/drivers/gpu/drm/i915/display/intel_audio.c @@ -333,19 +333,24 @@ static void g4x_audio_codec_enable(struct intel_encoder *encoder, struct drm_i915_private *i915 = to_i915(encoder->base.dev); struct drm_connector *connector = conn_state->connector; const u8 *eld = connector->eld; + int eld_buffer_size, len, i; u32 tmp; - int len, i; tmp = intel_de_read(i915, G4X_AUD_CNTL_ST); tmp &= ~(G4X_ELD_VALID | G4X_ELD_ADDRESS_MASK); intel_de_write(i915, G4X_AUD_CNTL_ST, tmp); - len = g4x_eld_buffer_size(i915); - len = min(drm_eld_size(eld) / 4, len); + eld_buffer_size = g4x_eld_buffer_size(i915); + len = min(drm_eld_size(eld) / 4, eld_buffer_size); for (i = 0; i < len; i++) intel_de_write(i915, G4X_HDMIW_HDMIEDID, *((const u32 *)eld + i)); + for (; i < eld_buffer_size; i++) + intel_de_write(i915, G4X_HDMIW_HDMIEDID, 0); + + drm_WARN_ON(&i915->drm, + (intel_de_read(i915, G4X_AUD_CNTL_ST) & G4X_ELD_ADDRESS_MASK) != 0); tmp = intel_de_read(i915, G4X_AUD_CNTL_ST); tmp |= G4X_ELD_VALID; @@ -608,8 +613,8 @@ static void hsw_audio_codec_enable(struct intel_encoder *encoder, struct drm_connector *connector = conn_state->connector; enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; const u8 *eld = connector->eld; + int eld_buffer_size, len, i; u32 tmp; - int len, i; mutex_lock(&i915->display.audio.mutex); @@ -635,12 +640,18 @@ static void hsw_audio_codec_enable(struct intel_encoder *encoder, tmp &= ~IBX_ELD_ADDRESS_MASK; intel_de_write(i915, HSW_AUD_DIP_ELD_CTRL(cpu_transcoder), tmp); - len = hsw_eld_buffer_size(i915, cpu_transcoder); - len = min(drm_eld_size(eld) / 4, len); + eld_buffer_size = hsw_eld_buffer_size(i915, cpu_transcoder); + len = min(drm_eld_size(eld) / 4, eld_buffer_size); for (i = 0; i < len; i++) intel_de_write(i915, HSW_AUD_EDID_DATA(cpu_transcoder), *((const u32 *)eld + i)); + for (; i < eld_buffer_size; i++) + intel_de_write(i915, HSW_AUD_EDID_DATA(cpu_transcoder), 0); + + drm_WARN_ON(&i915->drm, + (intel_de_read(i915, HSW_AUD_DIP_ELD_CTRL(cpu_transcoder)) & + IBX_ELD_ADDRESS_MASK) != 0); /* ELD valid */ tmp = intel_de_read(i915, HSW_AUD_PIN_ELD_CP_VLD); @@ -738,8 +749,8 @@ static void ilk_audio_codec_enable(struct intel_encoder *encoder, enum pipe pipe = crtc->pipe; enum port port = encoder->port; const u8 *eld = connector->eld; + int eld_buffer_size, len, i; struct ilk_audio_regs regs; - int len, i; u32 tmp; if (drm_WARN_ON(&i915->drm, port == PORT_A)) @@ -766,12 +777,17 @@ static void ilk_audio_codec_enable(struct intel_encoder *encoder, tmp &= ~IBX_ELD_ADDRESS_MASK; intel_de_write(i915, regs.aud_cntl_st, tmp); - len = ilk_eld_buffer_size(i915, pipe); - len = min(drm_eld_size(eld) / 4, len); + eld_buffer_size = ilk_eld_buffer_size(i915, pipe); + len = min(drm_eld_size(eld) / 4, eld_buffer_size); for (i = 0; i < len; i++) intel_de_write(i915, regs.hdmiw_hdmiedid, *((const u32 *)eld + i)); + for (; i < eld_buffer_size; i++) + intel_de_write(i915, regs.hdmiw_hdmiedid, 0); + + drm_WARN_ON(&i915->drm, + (intel_de_read(i915, regs.aud_cntl_st) & IBX_ELD_ADDRESS_MASK) != 0); /* ELD valid */ tmp = intel_de_read(i915, regs.aud_cntrl_st2);