| Message ID | 20180920185145.1912-17-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Infoframe precompute/check | expand |
On Thu, Sep 20, 2018 at 09:51:43PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Read the HDMI infoframes from the hbuf and unpack them into > the crtc state. > > Well, actually just AVI infoframe for now but let's write the > infoframe readout code in a more generic fashion in case we > expand this later. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Hm, caring about sdvo seems a bit overkill. And afaik we don't have any sdvo (much less hdmi) in CI. I'm leaning towards just adding a PIPE_CONFIG_QUIRK_INFOFRAMES for sdvo, and short-circuiting the checks if that's set. Except if you can somehow convince CI folks to add an sdvo hdmi card to CI :-) > --- > drivers/gpu/drm/i915/intel_sdvo.c | 92 +++++++++++++++++++++++++++++++++++++-- > 1 file changed, 89 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > index d8c78aebaf01..4d787c86df6d 100644 > --- a/drivers/gpu/drm/i915/intel_sdvo.c > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > @@ -981,6 +981,58 @@ static bool intel_sdvo_write_infoframe(struct intel_sdvo *intel_sdvo, > &tx_rate, 1); > } > > +static ssize_t intel_sdvo_read_infoframe(struct intel_sdvo *intel_sdvo, > + unsigned int if_index, > + u8 *data, unsigned int length) > +{ > + u8 set_buf_index[2] = { if_index, 0 }; > + u8 hbuf_size, tx_rate, av_split; > + int i; > + > + if (!intel_sdvo_get_value(intel_sdvo, > + SDVO_CMD_GET_HBUF_AV_SPLIT, > + &av_split, 1)) > + return -ENXIO; > + > + if (av_split < if_index) > + return 0; > + > + if (!intel_sdvo_get_value(intel_sdvo, > + SDVO_CMD_GET_HBUF_TXRATE, > + &tx_rate, 1)) > + return -ENXIO; > + > + if (tx_rate == SDVO_HBUF_TX_DISABLED) > + return 0; > + > + if (!intel_sdvo_set_value(intel_sdvo, > + SDVO_CMD_SET_HBUF_INDEX, > + set_buf_index, 2)) > + return -ENXIO; > + > + if (!intel_sdvo_get_value(intel_sdvo, SDVO_CMD_GET_HBUF_INFO, > + &hbuf_size, 1)) > + return -ENXIO; > + > + /* Buffer size is 0 based, hooray! */ > + hbuf_size++; > + > + DRM_DEBUG_KMS("reading sdvo hbuf: %i, hbuf_size %i, hbuf_size: %i\n", > + if_index, length, hbuf_size); > + > + hbuf_size = min_t(unsigned int, length, hbuf_size); > + > + for (i = 0; i < hbuf_size; i += 8) { > + if (!intel_sdvo_write_cmd(intel_sdvo, SDVO_CMD_GET_HBUF_DATA, NULL, 0)) > + return -ENXIO; > + if (!intel_sdvo_read_response(intel_sdvo, &data[i], > + min_t(unsigned int, 8, hbuf_size - i))) > + return -ENXIO; > + } > + > + return hbuf_size; > +} > + > static bool intel_sdvo_compute_avi_infoframe(struct intel_sdvo *intel_sdvo, > struct intel_crtc_state *crtc_state, > struct drm_connector_state *conn_state) > @@ -1039,6 +1091,37 @@ static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo, > sdvo_data, sizeof(sdvo_data)); > } > > +static bool intel_sdvo_get_avi_infoframe(struct intel_sdvo *intel_sdvo, > + struct intel_crtc_state *crtc_state) > +{ > + u8 sdvo_data[HDMI_INFOFRAME_SIZE(AVI)]; > + union hdmi_infoframe *frame = &crtc_state->infoframes.avi; > + ssize_t len; > + int ret; > + > + if (!crtc_state->has_hdmi_sink) > + return true; > + > + len = intel_sdvo_read_infoframe(intel_sdvo, SDVO_HBUF_INDEX_AVI_IF, > + sdvo_data, sizeof(sdvo_data)); > + if (len < 0) > + return false; > + else if (len == 0) > + return true; > + > + crtc_state->infoframes.enable |= > + intel_hdmi_infoframe_enable(HDMI_INFOFRAME_TYPE_AVI); > + > + ret = hdmi_infoframe_unpack(frame, sdvo_data, sizeof(sdvo_data)); > + if (ret) > + return false; > + > + if (frame->any.type != HDMI_INFOFRAME_TYPE_AVI) > + return false; > + > + return true; > +} > + > static bool intel_sdvo_set_tv_format(struct intel_sdvo *intel_sdvo, > const struct drm_connector_state *conn_state) > { > @@ -1535,6 +1618,10 @@ static void intel_sdvo_get_config(struct intel_encoder *encoder, > } > } > > + WARN(encoder_pixel_multiplier != pipe_config->pixel_multiplier, > + "SDVO pixel multiplier mismatch, port: %i, encoder: %i\n", > + pipe_config->pixel_multiplier, encoder_pixel_multiplier); > + > if (sdvox & HDMI_COLOR_RANGE_16_235) > pipe_config->limited_color_range = true; > > @@ -1547,9 +1634,8 @@ static void intel_sdvo_get_config(struct intel_encoder *encoder, > pipe_config->has_hdmi_sink = true; > } > > - WARN(encoder_pixel_multiplier != pipe_config->pixel_multiplier, > - "SDVO pixel multiplier mismatch, port: %i, encoder: %i\n", > - pipe_config->pixel_multiplier, encoder_pixel_multiplier); > + if (!intel_sdvo_get_avi_infoframe(intel_sdvo, pipe_config)) > + DRM_ERROR("failed to read AVI infoframe\n"); > } > > static void intel_disable_sdvo(struct intel_encoder *encoder, > -- > 2.16.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Sep 24, 2018 at 06:10:14PM +0200, Daniel Vetter wrote: > On Thu, Sep 20, 2018 at 09:51:43PM +0300, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Read the HDMI infoframes from the hbuf and unpack them into > > the crtc state. > > > > Well, actually just AVI infoframe for now but let's write the > > infoframe readout code in a more generic fashion in case we > > expand this later. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Hm, caring about sdvo seems a bit overkill. And afaik we don't have any > sdvo (much less hdmi) in CI. I'm leaning towards just adding a > PIPE_CONFIG_QUIRK_INFOFRAMES for sdvo, and short-circuiting the checks if > that's set. Except if you can somehow convince CI folks to add an sdvo > hdmi card to CI :-) Unfortunately I only have one SDVO HDMI device and it has the chip straight on the motherboard. I can't give mine up for ci :) I guess we could try to find another one of those as that model doesn't even seem super rare. Just the annoying usual problem of getting one from somewhere approved. I think having to maintain a quirk is ~500% more annoying than adding the readout code. > > > --- > > drivers/gpu/drm/i915/intel_sdvo.c | 92 +++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 89 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > > index d8c78aebaf01..4d787c86df6d 100644 > > --- a/drivers/gpu/drm/i915/intel_sdvo.c > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > > @@ -981,6 +981,58 @@ static bool intel_sdvo_write_infoframe(struct intel_sdvo *intel_sdvo, > > &tx_rate, 1); > > } > > > > +static ssize_t intel_sdvo_read_infoframe(struct intel_sdvo *intel_sdvo, > > + unsigned int if_index, > > + u8 *data, unsigned int length) > > +{ > > + u8 set_buf_index[2] = { if_index, 0 }; > > + u8 hbuf_size, tx_rate, av_split; > > + int i; > > + > > + if (!intel_sdvo_get_value(intel_sdvo, > > + SDVO_CMD_GET_HBUF_AV_SPLIT, > > + &av_split, 1)) > > + return -ENXIO; > > + > > + if (av_split < if_index) > > + return 0; > > + > > + if (!intel_sdvo_get_value(intel_sdvo, > > + SDVO_CMD_GET_HBUF_TXRATE, > > + &tx_rate, 1)) > > + return -ENXIO; > > + > > + if (tx_rate == SDVO_HBUF_TX_DISABLED) > > + return 0; > > + > > + if (!intel_sdvo_set_value(intel_sdvo, > > + SDVO_CMD_SET_HBUF_INDEX, > > + set_buf_index, 2)) > > + return -ENXIO; > > + > > + if (!intel_sdvo_get_value(intel_sdvo, SDVO_CMD_GET_HBUF_INFO, > > + &hbuf_size, 1)) > > + return -ENXIO; > > + > > + /* Buffer size is 0 based, hooray! */ > > + hbuf_size++; > > + > > + DRM_DEBUG_KMS("reading sdvo hbuf: %i, hbuf_size %i, hbuf_size: %i\n", > > + if_index, length, hbuf_size); > > + > > + hbuf_size = min_t(unsigned int, length, hbuf_size); > > + > > + for (i = 0; i < hbuf_size; i += 8) { > > + if (!intel_sdvo_write_cmd(intel_sdvo, SDVO_CMD_GET_HBUF_DATA, NULL, 0)) > > + return -ENXIO; > > + if (!intel_sdvo_read_response(intel_sdvo, &data[i], > > + min_t(unsigned int, 8, hbuf_size - i))) > > + return -ENXIO; > > + } > > + > > + return hbuf_size; > > +} > > + > > static bool intel_sdvo_compute_avi_infoframe(struct intel_sdvo *intel_sdvo, > > struct intel_crtc_state *crtc_state, > > struct drm_connector_state *conn_state) > > @@ -1039,6 +1091,37 @@ static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo, > > sdvo_data, sizeof(sdvo_data)); > > } > > > > +static bool intel_sdvo_get_avi_infoframe(struct intel_sdvo *intel_sdvo, > > + struct intel_crtc_state *crtc_state) > > +{ > > + u8 sdvo_data[HDMI_INFOFRAME_SIZE(AVI)]; > > + union hdmi_infoframe *frame = &crtc_state->infoframes.avi; > > + ssize_t len; > > + int ret; > > + > > + if (!crtc_state->has_hdmi_sink) > > + return true; > > + > > + len = intel_sdvo_read_infoframe(intel_sdvo, SDVO_HBUF_INDEX_AVI_IF, > > + sdvo_data, sizeof(sdvo_data)); > > + if (len < 0) > > + return false; > > + else if (len == 0) > > + return true; > > + > > + crtc_state->infoframes.enable |= > > + intel_hdmi_infoframe_enable(HDMI_INFOFRAME_TYPE_AVI); > > + > > + ret = hdmi_infoframe_unpack(frame, sdvo_data, sizeof(sdvo_data)); > > + if (ret) > > + return false; > > + > > + if (frame->any.type != HDMI_INFOFRAME_TYPE_AVI) > > + return false; > > + > > + return true; > > +} > > + > > static bool intel_sdvo_set_tv_format(struct intel_sdvo *intel_sdvo, > > const struct drm_connector_state *conn_state) > > { > > @@ -1535,6 +1618,10 @@ static void intel_sdvo_get_config(struct intel_encoder *encoder, > > } > > } > > > > + WARN(encoder_pixel_multiplier != pipe_config->pixel_multiplier, > > + "SDVO pixel multiplier mismatch, port: %i, encoder: %i\n", > > + pipe_config->pixel_multiplier, encoder_pixel_multiplier); > > + > > if (sdvox & HDMI_COLOR_RANGE_16_235) > > pipe_config->limited_color_range = true; > > > > @@ -1547,9 +1634,8 @@ static void intel_sdvo_get_config(struct intel_encoder *encoder, > > pipe_config->has_hdmi_sink = true; > > } > > > > - WARN(encoder_pixel_multiplier != pipe_config->pixel_multiplier, > > - "SDVO pixel multiplier mismatch, port: %i, encoder: %i\n", > > - pipe_config->pixel_multiplier, encoder_pixel_multiplier); > > + if (!intel_sdvo_get_avi_infoframe(intel_sdvo, pipe_config)) > > + DRM_ERROR("failed to read AVI infoframe\n"); > > } > > > > static void intel_disable_sdvo(struct intel_encoder *encoder, > > -- > > 2.16.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Mon, Sep 24, 2018 at 08:13:08PM +0300, Ville Syrjälä wrote: > On Mon, Sep 24, 2018 at 06:10:14PM +0200, Daniel Vetter wrote: > > On Thu, Sep 20, 2018 at 09:51:43PM +0300, Ville Syrjala wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Read the HDMI infoframes from the hbuf and unpack them into > > > the crtc state. > > > > > > Well, actually just AVI infoframe for now but let's write the > > > infoframe readout code in a more generic fashion in case we > > > expand this later. > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Hm, caring about sdvo seems a bit overkill. And afaik we don't have any > > sdvo (much less hdmi) in CI. I'm leaning towards just adding a > > PIPE_CONFIG_QUIRK_INFOFRAMES for sdvo, and short-circuiting the checks if > > that's set. Except if you can somehow convince CI folks to add an sdvo > > hdmi card to CI :-) > > Unfortunately I only have one SDVO HDMI device and it has the chip > straight on the motherboard. I can't give mine up for ci :) I guess > we could try to find another one of those as that model doesn't > even seem super rare. Just the annoying usual problem of getting > one from somewhere approved. > > I think having to maintain a quirk is ~500% more annoying than > adding the readout code. My experience disagrees, a bunch of the (early?) sdvo chips don't bother to implement all the get stuff. I had a pile of these (but some of them died, and plugging them all into machines is a pain), and just because it works on 1 chip doesn't mean it's actually all that useful. That's why I suggested we do the same thing as with the other stuff we read out from the sdvo chip (as opposed to things we can read out from crtc/sdvo-host-side registers). -Daniel > > > > > > --- > > > drivers/gpu/drm/i915/intel_sdvo.c | 92 +++++++++++++++++++++++++++++++++++++-- > > > 1 file changed, 89 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > > > index d8c78aebaf01..4d787c86df6d 100644 > > > --- a/drivers/gpu/drm/i915/intel_sdvo.c > > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > > > @@ -981,6 +981,58 @@ static bool intel_sdvo_write_infoframe(struct intel_sdvo *intel_sdvo, > > > &tx_rate, 1); > > > } > > > > > > +static ssize_t intel_sdvo_read_infoframe(struct intel_sdvo *intel_sdvo, > > > + unsigned int if_index, > > > + u8 *data, unsigned int length) > > > +{ > > > + u8 set_buf_index[2] = { if_index, 0 }; > > > + u8 hbuf_size, tx_rate, av_split; > > > + int i; > > > + > > > + if (!intel_sdvo_get_value(intel_sdvo, > > > + SDVO_CMD_GET_HBUF_AV_SPLIT, > > > + &av_split, 1)) > > > + return -ENXIO; > > > + > > > + if (av_split < if_index) > > > + return 0; > > > + > > > + if (!intel_sdvo_get_value(intel_sdvo, > > > + SDVO_CMD_GET_HBUF_TXRATE, > > > + &tx_rate, 1)) > > > + return -ENXIO; > > > + > > > + if (tx_rate == SDVO_HBUF_TX_DISABLED) > > > + return 0; > > > + > > > + if (!intel_sdvo_set_value(intel_sdvo, > > > + SDVO_CMD_SET_HBUF_INDEX, > > > + set_buf_index, 2)) > > > + return -ENXIO; > > > + > > > + if (!intel_sdvo_get_value(intel_sdvo, SDVO_CMD_GET_HBUF_INFO, > > > + &hbuf_size, 1)) > > > + return -ENXIO; > > > + > > > + /* Buffer size is 0 based, hooray! */ > > > + hbuf_size++; > > > + > > > + DRM_DEBUG_KMS("reading sdvo hbuf: %i, hbuf_size %i, hbuf_size: %i\n", > > > + if_index, length, hbuf_size); > > > + > > > + hbuf_size = min_t(unsigned int, length, hbuf_size); > > > + > > > + for (i = 0; i < hbuf_size; i += 8) { > > > + if (!intel_sdvo_write_cmd(intel_sdvo, SDVO_CMD_GET_HBUF_DATA, NULL, 0)) > > > + return -ENXIO; > > > + if (!intel_sdvo_read_response(intel_sdvo, &data[i], > > > + min_t(unsigned int, 8, hbuf_size - i))) > > > + return -ENXIO; > > > + } > > > + > > > + return hbuf_size; > > > +} > > > + > > > static bool intel_sdvo_compute_avi_infoframe(struct intel_sdvo *intel_sdvo, > > > struct intel_crtc_state *crtc_state, > > > struct drm_connector_state *conn_state) > > > @@ -1039,6 +1091,37 @@ static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo, > > > sdvo_data, sizeof(sdvo_data)); > > > } > > > > > > +static bool intel_sdvo_get_avi_infoframe(struct intel_sdvo *intel_sdvo, > > > + struct intel_crtc_state *crtc_state) > > > +{ > > > + u8 sdvo_data[HDMI_INFOFRAME_SIZE(AVI)]; > > > + union hdmi_infoframe *frame = &crtc_state->infoframes.avi; > > > + ssize_t len; > > > + int ret; > > > + > > > + if (!crtc_state->has_hdmi_sink) > > > + return true; > > > + > > > + len = intel_sdvo_read_infoframe(intel_sdvo, SDVO_HBUF_INDEX_AVI_IF, > > > + sdvo_data, sizeof(sdvo_data)); > > > + if (len < 0) > > > + return false; > > > + else if (len == 0) > > > + return true; > > > + > > > + crtc_state->infoframes.enable |= > > > + intel_hdmi_infoframe_enable(HDMI_INFOFRAME_TYPE_AVI); > > > + > > > + ret = hdmi_infoframe_unpack(frame, sdvo_data, sizeof(sdvo_data)); > > > + if (ret) > > > + return false; > > > + > > > + if (frame->any.type != HDMI_INFOFRAME_TYPE_AVI) > > > + return false; > > > + > > > + return true; > > > +} > > > + > > > static bool intel_sdvo_set_tv_format(struct intel_sdvo *intel_sdvo, > > > const struct drm_connector_state *conn_state) > > > { > > > @@ -1535,6 +1618,10 @@ static void intel_sdvo_get_config(struct intel_encoder *encoder, > > > } > > > } > > > > > > + WARN(encoder_pixel_multiplier != pipe_config->pixel_multiplier, > > > + "SDVO pixel multiplier mismatch, port: %i, encoder: %i\n", > > > + pipe_config->pixel_multiplier, encoder_pixel_multiplier); > > > + > > > if (sdvox & HDMI_COLOR_RANGE_16_235) > > > pipe_config->limited_color_range = true; > > > > > > @@ -1547,9 +1634,8 @@ static void intel_sdvo_get_config(struct intel_encoder *encoder, > > > pipe_config->has_hdmi_sink = true; > > > } > > > > > > - WARN(encoder_pixel_multiplier != pipe_config->pixel_multiplier, > > > - "SDVO pixel multiplier mismatch, port: %i, encoder: %i\n", > > > - pipe_config->pixel_multiplier, encoder_pixel_multiplier); > > > + if (!intel_sdvo_get_avi_infoframe(intel_sdvo, pipe_config)) > > > + DRM_ERROR("failed to read AVI infoframe\n"); > > > } > > > > > > static void intel_disable_sdvo(struct intel_encoder *encoder, > > > -- > > > 2.16.4 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > -- > Ville Syrjälä > Intel
On Mon, Oct 01, 2018 at 08:59:41AM +0200, Daniel Vetter wrote: > On Mon, Sep 24, 2018 at 08:13:08PM +0300, Ville Syrjälä wrote: > > On Mon, Sep 24, 2018 at 06:10:14PM +0200, Daniel Vetter wrote: > > > On Thu, Sep 20, 2018 at 09:51:43PM +0300, Ville Syrjala wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > Read the HDMI infoframes from the hbuf and unpack them into > > > > the crtc state. > > > > > > > > Well, actually just AVI infoframe for now but let's write the > > > > infoframe readout code in a more generic fashion in case we > > > > expand this later. > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Hm, caring about sdvo seems a bit overkill. And afaik we don't have any > > > sdvo (much less hdmi) in CI. I'm leaning towards just adding a > > > PIPE_CONFIG_QUIRK_INFOFRAMES for sdvo, and short-circuiting the checks if > > > that's set. Except if you can somehow convince CI folks to add an sdvo > > > hdmi card to CI :-) > > > > Unfortunately I only have one SDVO HDMI device and it has the chip > > straight on the motherboard. I can't give mine up for ci :) I guess > > we could try to find another one of those as that model doesn't > > even seem super rare. Just the annoying usual problem of getting > > one from somewhere approved. > > > > I think having to maintain a quirk is ~500% more annoying than > > adding the readout code. > > My experience disagrees, a bunch of the (early?) sdvo chips don't bother > to implement all the get stuff. I had a pile of these (but some of them > died, and plugging them all into machines is a pain), and just because it > works on 1 chip doesn't mean it's actually all that useful. That's why I > suggested we do the same thing as with the other stuff we read out from > the sdvo chip (as opposed to things we can read out from > crtc/sdvo-host-side registers). We do read out the hdmi encoding and pixel multipler from the sdvo chip already. If the chips don't implement the hdmi stuff we treat them as dvi. I have some (supposedly) hdmi add2 cards like that. So I don't think those pose any kind of real problem.
On Mon, Oct 01, 2018 at 04:35:50PM +0300, Ville Syrjälä wrote: > On Mon, Oct 01, 2018 at 08:59:41AM +0200, Daniel Vetter wrote: > > On Mon, Sep 24, 2018 at 08:13:08PM +0300, Ville Syrjälä wrote: > > > On Mon, Sep 24, 2018 at 06:10:14PM +0200, Daniel Vetter wrote: > > > > On Thu, Sep 20, 2018 at 09:51:43PM +0300, Ville Syrjala wrote: > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > Read the HDMI infoframes from the hbuf and unpack them into > > > > > the crtc state. > > > > > > > > > > Well, actually just AVI infoframe for now but let's write the > > > > > infoframe readout code in a more generic fashion in case we > > > > > expand this later. > > > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > Hm, caring about sdvo seems a bit overkill. And afaik we don't have any > > > > sdvo (much less hdmi) in CI. I'm leaning towards just adding a > > > > PIPE_CONFIG_QUIRK_INFOFRAMES for sdvo, and short-circuiting the checks if > > > > that's set. Except if you can somehow convince CI folks to add an sdvo > > > > hdmi card to CI :-) > > > > > > Unfortunately I only have one SDVO HDMI device and it has the chip > > > straight on the motherboard. I can't give mine up for ci :) I guess > > > we could try to find another one of those as that model doesn't > > > even seem super rare. Just the annoying usual problem of getting > > > one from somewhere approved. > > > > > > I think having to maintain a quirk is ~500% more annoying than > > > adding the readout code. > > > > My experience disagrees, a bunch of the (early?) sdvo chips don't bother > > to implement all the get stuff. I had a pile of these (but some of them > > died, and plugging them all into machines is a pain), and just because it > > works on 1 chip doesn't mean it's actually all that useful. That's why I > > suggested we do the same thing as with the other stuff we read out from > > the sdvo chip (as opposed to things we can read out from > > crtc/sdvo-host-side registers). > > We do read out the hdmi encoding and pixel multipler from > the sdvo chip already. If the chips don't implement the hdmi > stuff we treat them as dvi. I have some (supposedly) hdmi > add2 cards like that. So I don't think those pose any kind > of real problem. Hm ok, but you get to keep the pieces if there's an sdvo regression report :-) I.e. in that case I'd vote for a revert + quirk flag, and call it a day. But I guess for now we could assume that hdmi sdvo cards are less garbage, and implement the spec better. On both this and the previous patch: Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> (Since I'm too lazy to dig out the hdmi sdvo spec and check all the details, imo not quite worth it). It would be good to capture the gist of this discussion here in the commit message, for future reference. Maybe stuff it into the readout patch. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index d8c78aebaf01..4d787c86df6d 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -981,6 +981,58 @@ static bool intel_sdvo_write_infoframe(struct intel_sdvo *intel_sdvo, &tx_rate, 1); } +static ssize_t intel_sdvo_read_infoframe(struct intel_sdvo *intel_sdvo, + unsigned int if_index, + u8 *data, unsigned int length) +{ + u8 set_buf_index[2] = { if_index, 0 }; + u8 hbuf_size, tx_rate, av_split; + int i; + + if (!intel_sdvo_get_value(intel_sdvo, + SDVO_CMD_GET_HBUF_AV_SPLIT, + &av_split, 1)) + return -ENXIO; + + if (av_split < if_index) + return 0; + + if (!intel_sdvo_get_value(intel_sdvo, + SDVO_CMD_GET_HBUF_TXRATE, + &tx_rate, 1)) + return -ENXIO; + + if (tx_rate == SDVO_HBUF_TX_DISABLED) + return 0; + + if (!intel_sdvo_set_value(intel_sdvo, + SDVO_CMD_SET_HBUF_INDEX, + set_buf_index, 2)) + return -ENXIO; + + if (!intel_sdvo_get_value(intel_sdvo, SDVO_CMD_GET_HBUF_INFO, + &hbuf_size, 1)) + return -ENXIO; + + /* Buffer size is 0 based, hooray! */ + hbuf_size++; + + DRM_DEBUG_KMS("reading sdvo hbuf: %i, hbuf_size %i, hbuf_size: %i\n", + if_index, length, hbuf_size); + + hbuf_size = min_t(unsigned int, length, hbuf_size); + + for (i = 0; i < hbuf_size; i += 8) { + if (!intel_sdvo_write_cmd(intel_sdvo, SDVO_CMD_GET_HBUF_DATA, NULL, 0)) + return -ENXIO; + if (!intel_sdvo_read_response(intel_sdvo, &data[i], + min_t(unsigned int, 8, hbuf_size - i))) + return -ENXIO; + } + + return hbuf_size; +} + static bool intel_sdvo_compute_avi_infoframe(struct intel_sdvo *intel_sdvo, struct intel_crtc_state *crtc_state, struct drm_connector_state *conn_state) @@ -1039,6 +1091,37 @@ static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo, sdvo_data, sizeof(sdvo_data)); } +static bool intel_sdvo_get_avi_infoframe(struct intel_sdvo *intel_sdvo, + struct intel_crtc_state *crtc_state) +{ + u8 sdvo_data[HDMI_INFOFRAME_SIZE(AVI)]; + union hdmi_infoframe *frame = &crtc_state->infoframes.avi; + ssize_t len; + int ret; + + if (!crtc_state->has_hdmi_sink) + return true; + + len = intel_sdvo_read_infoframe(intel_sdvo, SDVO_HBUF_INDEX_AVI_IF, + sdvo_data, sizeof(sdvo_data)); + if (len < 0) + return false; + else if (len == 0) + return true; + + crtc_state->infoframes.enable |= + intel_hdmi_infoframe_enable(HDMI_INFOFRAME_TYPE_AVI); + + ret = hdmi_infoframe_unpack(frame, sdvo_data, sizeof(sdvo_data)); + if (ret) + return false; + + if (frame->any.type != HDMI_INFOFRAME_TYPE_AVI) + return false; + + return true; +} + static bool intel_sdvo_set_tv_format(struct intel_sdvo *intel_sdvo, const struct drm_connector_state *conn_state) { @@ -1535,6 +1618,10 @@ static void intel_sdvo_get_config(struct intel_encoder *encoder, } } + WARN(encoder_pixel_multiplier != pipe_config->pixel_multiplier, + "SDVO pixel multiplier mismatch, port: %i, encoder: %i\n", + pipe_config->pixel_multiplier, encoder_pixel_multiplier); + if (sdvox & HDMI_COLOR_RANGE_16_235) pipe_config->limited_color_range = true; @@ -1547,9 +1634,8 @@ static void intel_sdvo_get_config(struct intel_encoder *encoder, pipe_config->has_hdmi_sink = true; } - WARN(encoder_pixel_multiplier != pipe_config->pixel_multiplier, - "SDVO pixel multiplier mismatch, port: %i, encoder: %i\n", - pipe_config->pixel_multiplier, encoder_pixel_multiplier); + if (!intel_sdvo_get_avi_infoframe(intel_sdvo, pipe_config)) + DRM_ERROR("failed to read AVI infoframe\n"); } static void intel_disable_sdvo(struct intel_encoder *encoder,