Thank you for the review Hans. My comments are inline. A v2 of this
series is under way.
On 5/29/19 12:39 AM, Hans Verkuil wrote:
> Hi Johan,
>
> Thanks for this patch series!
>
> The previous patches all look good, but this one needs a bit more work:
>
> On 5/28/19 7:19 PM, johan.korsnes@xxxxxxxxx wrote:
>> From: Johan Korsnes <johan.korsnes@xxxxxxxxx>
>>
>> Set/invalidate physical addresses based on the configuration of the
>> display present control. This is relevant not only when the display
>> present control is modified, but also when the Vivid instance EDID is
>> set/cleared.
>>
>> Signed-off-by: Johan Korsnes <johan.korsnes@xxxxxxxxx>
>> ---
>> drivers/media/platform/vivid/vivid-core.c | 16 ++++++------
>> drivers/media/platform/vivid/vivid-ctrls.c | 25 ++++++++++++++++---
>> drivers/media/platform/vivid/vivid-vid-cap.c | 6 ++++-
>> .../media/platform/vivid/vivid-vid-common.c | 2 ++
>> 4 files changed, 36 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c
>> index b2b4ee48cef0..ca46ac24f51e 100644
>> --- a/drivers/media/platform/vivid/vivid-core.c
>> +++ b/drivers/media/platform/vivid/vivid-core.c
>> @@ -1060,14 +1060,6 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>> vivid_update_format_cap(dev, false);
>> vivid_update_format_out(dev);
>>
>> - v4l2_ctrl_handler_setup(&dev->ctrl_hdl_vid_cap);
>> - v4l2_ctrl_handler_setup(&dev->ctrl_hdl_vid_out);
>> - v4l2_ctrl_handler_setup(&dev->ctrl_hdl_vbi_cap);
>> - v4l2_ctrl_handler_setup(&dev->ctrl_hdl_vbi_out);
>> - v4l2_ctrl_handler_setup(&dev->ctrl_hdl_radio_rx);
>> - v4l2_ctrl_handler_setup(&dev->ctrl_hdl_radio_tx);
>> - v4l2_ctrl_handler_setup(&dev->ctrl_hdl_sdr_cap);
>> -
>> /* initialize overlay */
>> dev->fb_cap.fmt.width = dev->src_rect.width;
>> dev->fb_cap.fmt.height = dev->src_rect.height;
>> @@ -1488,6 +1480,14 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>> }
>> #endif
>>
>> + v4l2_ctrl_handler_setup(&dev->ctrl_hdl_vid_cap);
>> + v4l2_ctrl_handler_setup(&dev->ctrl_hdl_vid_out);
>> + v4l2_ctrl_handler_setup(&dev->ctrl_hdl_vbi_cap);
>> + v4l2_ctrl_handler_setup(&dev->ctrl_hdl_vbi_out);
>> + v4l2_ctrl_handler_setup(&dev->ctrl_hdl_radio_rx);
>> + v4l2_ctrl_handler_setup(&dev->ctrl_hdl_radio_tx);
>> + v4l2_ctrl_handler_setup(&dev->ctrl_hdl_sdr_cap);
>> +
>> /* Now that everything is fine, let's add it to device list */
>> vivid_devs[inst] = dev;
>>
>
> This change should be in a patch of its own.
>
> It also needs more work. The reason for this change is that v4l2_ctrl_handler_setup()
> expects that the cec adapters are up and running, so moving these calls until after
> the creation of the adapters fixes that.
>
> However, that now means that controls change after the creation of the video devices,
> which isn't right.
>
> If you look at the probe() function you'll see that things are done in two stages:
> first all the vb2 queues are created, and only if that succeeded are the video devices
> created.
>
> The same should be done for the CEC adapters: they should be allocated (vivid_cec_alloc_adap)
> in the first stage, then call v4l2_ctrl_handler_setup(), and finally create the actual
> device nodes (cec_register_adapter).
>
> That way the controls are configured correctly before any device nodes are created.
I have split up the allocation and registration in v2. Why is this
order important? Some race condition with user space?
>
>> diff --git a/drivers/media/platform/vivid/vivid-ctrls.c b/drivers/media/platform/vivid/vivid-ctrls.c
>> index ae3690fd1b52..e4758a2837e6 100644
>> --- a/drivers/media/platform/vivid/vivid-ctrls.c
>> +++ b/drivers/media/platform/vivid/vivid-ctrls.c
>> @@ -18,6 +18,7 @@
>> #include "vivid-radio-common.h"
>> #include "vivid-osd.h"
>> #include "vivid-ctrls.h"
>> +#include "vivid-cec.h"
>>
>> #define VIVID_CID_CUSTOM_BASE (V4L2_CID_USER_BASE | 0xf000)
>> #define VIVID_CID_BUTTON (VIVID_CID_CUSTOM_BASE + 0)
>> @@ -923,7 +924,7 @@ static int vivid_vid_out_s_ctrl(struct v4l2_ctrl *ctrl)
>> struct vivid_dev *dev = container_of(ctrl->handler, struct vivid_dev, ctrl_hdl_vid_out);
>> struct v4l2_bt_timings *bt = &dev->dv_timings_out.bt;
>> u32 display_present = 0;
>> - unsigned i, j;
>> + unsigned i, j, bus_idx;
>>
>> switch (ctrl->id) {
>> case VIVID_CID_HAS_CROP_OUT:
>> @@ -962,15 +963,31 @@ static int vivid_vid_out_s_ctrl(struct v4l2_ctrl *ctrl)
>> break;
>>
>> dev->display_present[dev->output] = ctrl->val;
>> -
>> for (i = 0, j = 0; i < dev->num_outputs; i++)
>> if (dev->output_type[i] == HDMI)
>> display_present |=
>> dev->display_present[i] << j++;
>>
>> - __v4l2_ctrl_s_ctrl(dev->ctrl_tx_hotplug, display_present);
>> __v4l2_ctrl_s_ctrl(dev->ctrl_tx_rxsense, display_present);
>> - __v4l2_ctrl_s_ctrl(dev->ctrl_tx_edid_present, display_present);
>> +
>> + if (dev->edid_blocks) {
>> + __v4l2_ctrl_s_ctrl(dev->ctrl_tx_edid_present,
>> + display_present);
>> + __v4l2_ctrl_s_ctrl(dev->ctrl_tx_hotplug,
>> + display_present);
>> + }
>> +
>> + if (dev->cec_tx_adap == NULL)
>> + break;
>> +
>> + bus_idx = dev->cec_output2bus_map[dev->output];
>> + if (ctrl->val && dev->edid_blocks)
>> + cec_s_phys_addr(dev->cec_tx_adap[bus_idx],
>> + dev->cec_tx_adap[bus_idx]->phys_addr,
>> + false);
>> + else
>> + cec_phys_addr_invalidate(dev->cec_tx_adap[bus_idx]);
>> +
>> break;
>> }
>> return 0;
>> diff --git a/drivers/media/platform/vivid/vivid-vid-cap.c b/drivers/media/platform/vivid/vivid-vid-cap.c
>> index ca15c13abf6c..920be638da83 100644
>> --- a/drivers/media/platform/vivid/vivid-vid-cap.c
>> +++ b/drivers/media/platform/vivid/vivid-vid-cap.c
>> @@ -1760,6 +1760,7 @@ int vidioc_s_edid(struct file *file, void *_fh,
>> return -EINVAL;
>> if (edid->blocks == 0) {
>> dev->edid_blocks = 0;
>> + v4l2_ctrl_s_ctrl(dev->ctrl_tx_edid_present, false);
>
> false should be 0 (it's a bitmask control).
> The tx_hotplug control should be set to 0 as well.
>
>> phys_addr = CEC_PHYS_ADDR_INVALID;
>> goto set_phys_addr;
>> }
>> @@ -1777,6 +1778,7 @@ int vidioc_s_edid(struct file *file, void *_fh,
>>
>> dev->edid_blocks = edid->blocks;
>> memcpy(dev->edid, edid->edid, edid->blocks * 128);
>> + v4l2_ctrl_s_ctrl(dev->ctrl_tx_edid_present, true);
>
> true should be a display_present mask. And the tx_hotplug control should
> be set as well with the same mask.
>
> It's probably best to just recalculate the display_present mask, just as
> you did in vivid_vid_out_s_ctrl().
Not sure what I was thinking here. Will fix!
>
>>
>> set_phys_addr:
>> /* TODO: a proper hotplug detect cycle should be emulated here */
>> @@ -1784,7 +1786,9 @@ int vidioc_s_edid(struct file *file, void *_fh,
>>
>> for (i = 0; i < MAX_OUTPUTS && dev->cec_tx_adap[i]; i++)
>> cec_s_phys_addr(dev->cec_tx_adap[i],
>> - v4l2_phys_addr_for_input(phys_addr, i + 1),
>> + dev->display_present[i] ?
>> + v4l2_phys_addr_for_input(phys_addr, i + 1) :
>> + CEC_PHYS_ADDR_INVALID,
>> false);
>> return 0;
>> }
>> diff --git a/drivers/media/platform/vivid/vivid-vid-common.c b/drivers/media/platform/vivid/vivid-vid-common.c
>> index b6882426fc12..0addf8ef7dbf 100644
>> --- a/drivers/media/platform/vivid/vivid-vid-common.c
>> +++ b/drivers/media/platform/vivid/vivid-vid-common.c
>> @@ -907,6 +907,8 @@ int vidioc_g_edid(struct file *file, void *_fh,
>> return -EINVAL;
>> if (dev->output_type[edid->pad] != HDMI)
>> return -EINVAL;
>> + if (!dev->display_present[dev->output])
>
> dev->output should be edid->pad. VIDIOC_G_EDID gets the EDID of the specified
> output, not of the current output.
>
Will fix this in v2.
>> + return -ENODATA;
>> bus_idx = dev->cec_output2bus_map[edid->pad];
>> adap = dev->cec_tx_adap[bus_idx];
>> }
>>
>
> Regards,
>
> Hans
>
