On Wed, Oct 12, 2022 at 05:28:27PM +0300, Jani Nikula wrote:
> On Tue, 11 Oct 2022, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Cc: Kai Vehmanen <kai.vehmanen@xxxxxxxxxxxxxxx>
> > Cc: Takashi Iwai <tiwai@xxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > 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.
--
Ville Syrjälä
Intel
