On Wed, 12 Oct 2022, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> 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.
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.
--
Jani Nikula, Intel Open Source Graphics Center
